Re: [RFC PATCH stable-4.4] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping

[Michal Hocko]

On Thu 06-06-19 19:42:20, Ajay Kaher wrote:
> 
> > From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> >
> > Upstream 04f5866e41fb70690e28397487d8bd8eea7d712a commit.
> >
> >
> > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> > ---
> > Hi,
> > this is based on the backport I have done for out 4.4 based distribution
> > kernel. Please double check that I haven't missed anything before
> > applying to the stable tree. I have also CCed Joel for the binder part
> > which is not in the current upstream anymore but I believe it needs the
> > check as well.
> >
> > Review feedback welcome.
> >
> > drivers/android/binder.c |  6 ++++++
> > fs/proc/task_mmu.c       | 18 ++++++++++++++++++
> > fs/userfaultfd.c         | 10 ++++++++--
> > include/linux/mm.h       | 21 +++++++++++++++++++++
> > mm/huge_memory.c         |  2 +-
> > mm/mmap.c                |  7 ++++++-
> > 6 files changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 260ce0e60187..1fb1cddbd19a 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -570,6 +570,12 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
> > 
> > 	if (mm) {
> > 		down_write(&mm->mmap_sem);
> > +		if (!mmget_still_valid(mm)) {
> > +			if (allocate == 0)
> > +				goto free_range;
> 
> Please cross check, free_range: should not end-up with modifications in vma.

A review from a binder expert is definitely due but this function
clearly modifies the vma. Maybe the mapping is not really that important
because the coredump would simply not see the new mapping and therefore
"only" generate an incomplete/corrupted dump rather than leak an
information. I went with a "just to be sure" approach and add the check
to all locations which might be operating on a remote mm and modify the
address space.

-- 
Michal Hocko
SUSE Labs