All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
	Ingo Molnar <mingo@kernel.org>,
	Konstantin Khlebnikov <koct9i@gmail.com>,
	Michal Hocko <mhocko@suse.com>, Oleg Nesterov <oleg@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	stable@vger.kernel.org, Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH] fork: fix incorrect fput of ->exe_file causing use-after-free
Date: Fri, 25 Aug 2017 11:49:42 +0100	[thread overview]
Message-ID: <20170825104941.GC3127@leverpostej> (raw)
In-Reply-To: <20170824150110.GA29665@leverpostej>

On Thu, Aug 24, 2017 at 04:02:49PM +0100, Mark Rutland wrote:
> On Wed, Aug 23, 2017 at 02:14:08PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Commit 7c051267931a ("mm, fork: make dup_mmap wait for mmap_sem for
> > write killable") made it possible to kill a forking task while it is
> > waiting to acquire its ->mmap_sem for write, in dup_mmap().  However, it
> > was overlooked that this introduced an new error path before a reference
> > is taken on the mm_struct's ->exe_file.  Since the ->exe_file of the new
> > mm_struct was already set to the old ->exe_file by the memcpy() in
> > dup_mm(), it was possible for the mmput() in the error path of dup_mm()
> > to drop a reference to ->exe_file which was never taken.  This caused
> > the struct file to later be freed prematurely.
> > 
> > Fix it by updating mm_init() to NULL out the ->exe_file, in the same
> > place it clears other things like the list of mmaps.
> 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index e075b7780421..cbbea277b3fb 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -806,6 +806,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
> >  	mm_init_cpumask(mm);
> >  	mm_init_aio(mm);
> >  	mm_init_owner(mm, p);
> > +	RCU_INIT_POINTER(mm->exe_file, NULL);
> >  	mmu_notifier_mm_init(mm);
> >  	init_tlb_flush_pending(mm);
> >  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
> 
> I've been seeing similar issues on arm64 with use-after-free of a file
> and other memory corruption [1].
> 
> This patch seems to fix that; a test that normally fired in a few
> minutes has been happily running for hours with this applied.

Those haven't triggered after 24 hours, and in 16+ hours of fuzzing with
this applied, I haven't seen new issues. FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
	Ingo Molnar <mingo@kernel.org>,
	Konstantin Khlebnikov <koct9i@gmail.com>,
	Michal Hocko <mhocko@suse.com>, Oleg Nesterov <oleg@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	stable@vger.kernel.org, Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH] fork: fix incorrect fput of ->exe_file causing use-after-free
Date: Fri, 25 Aug 2017 11:49:42 +0100	[thread overview]
Message-ID: <20170825104941.GC3127@leverpostej> (raw)
In-Reply-To: <20170824150110.GA29665@leverpostej>

On Thu, Aug 24, 2017 at 04:02:49PM +0100, Mark Rutland wrote:
> On Wed, Aug 23, 2017 at 02:14:08PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Commit 7c051267931a ("mm, fork: make dup_mmap wait for mmap_sem for
> > write killable") made it possible to kill a forking task while it is
> > waiting to acquire its ->mmap_sem for write, in dup_mmap().  However, it
> > was overlooked that this introduced an new error path before a reference
> > is taken on the mm_struct's ->exe_file.  Since the ->exe_file of the new
> > mm_struct was already set to the old ->exe_file by the memcpy() in
> > dup_mm(), it was possible for the mmput() in the error path of dup_mm()
> > to drop a reference to ->exe_file which was never taken.  This caused
> > the struct file to later be freed prematurely.
> > 
> > Fix it by updating mm_init() to NULL out the ->exe_file, in the same
> > place it clears other things like the list of mmaps.
> 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index e075b7780421..cbbea277b3fb 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -806,6 +806,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
> >  	mm_init_cpumask(mm);
> >  	mm_init_aio(mm);
> >  	mm_init_owner(mm, p);
> > +	RCU_INIT_POINTER(mm->exe_file, NULL);
> >  	mmu_notifier_mm_init(mm);
> >  	init_tlb_flush_pending(mm);
> >  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
> 
> I've been seeing similar issues on arm64 with use-after-free of a file
> and other memory corruption [1].
> 
> This patch seems to fix that; a test that normally fired in a few
> minutes has been happily running for hours with this applied.

Those haven't triggered after 24 hours, and in 16+ hours of fuzzing with
this applied, I haven't seen new issues. FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

  reply	other threads:[~2017-08-25 10:50 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-23 21:14 [PATCH] fork: fix incorrect fput of ->exe_file causing use-after-free Eric Biggers
2017-08-23 21:14 ` Eric Biggers
2017-08-24 13:20 ` Oleg Nesterov
2017-08-24 13:20   ` Oleg Nesterov
2017-08-24 16:59   ` Eric Biggers
2017-08-24 16:59     ` Eric Biggers
2017-08-25 14:40     ` Oleg Nesterov
2017-08-25 14:40       ` Oleg Nesterov
2017-08-28 15:55       ` Eric Biggers
2017-08-28 15:55         ` Eric Biggers
2017-08-24 15:02 ` Mark Rutland
2017-08-24 15:02   ` Mark Rutland
2017-08-25 10:49   ` Mark Rutland [this message]
2017-08-25 10:49     ` Mark Rutland
2017-11-16 22:24 ` Jason A. Donenfeld
2017-11-16 22:24   ` Jason A. Donenfeld

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170825104941.GC3127@leverpostej \
    --to=mark.rutland@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=dvyukov@google.com \
    --cc=ebiggers3@gmail.com \
    --cc=ebiggers@google.com \
    --cc=koct9i@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=stable@vger.kernel.org \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.