Re: kernel BUG at fs/userfaultfd.c:LINE!

Re: kernel BUG at fs/userfaultfd.c:LINE!

[Eric Biggers]

[+Cc aarcange@redhat.com, linux-mm@kvack.org]

On Fri, Dec 22, 2017 at 01:37:01PM -0800, syzbot wrote:
> Hello,
> 
> syzkaller hit the following crash on
> 6084b576dca2e898f5c101baef151f7bfdbb606d
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
> 
> 
> ------------[ cut here ]------------
> kernel BUG at fs/userfaultfd.c:142!
> invalid opcode: 0000 [#1] SMP
> Dumping ftrace buffer:
>    (ftrace buffer empty)
> Modules linked in:
> CPU: 0 PID: 3118 Comm: syzkaller879466 Not tainted 4.15.0-rc3-next-20171214+
> #67
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> RIP: 0010:userfaultfd_ctx_get+0x6d/0x70 fs/userfaultfd.c:142
> RSP: 0000:ffffc900012f7c30 EFLAGS: 00010293
> RAX: ffff8802134420c0 RBX: 0000000000000000 RCX: ffffffff8147a98d
> RDX: 0000000000000000 RSI: 0000000000000200 RDI: ffff880213659c40
> RBP: ffffc900012f7c48 R08: 0000000000000000 R09: 0000000000000004
> R10: ffffc900012f7cc0 R11: 0000000000000004 R12: ffff880213659c40
> R13: ffff880214ed6000 R14: 0000000000000200 R15: 0000000000000000
> FS:  00007fdf76164700(0000) GS:ffff88021fc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020687000 CR3: 0000000211d48006 CR4: 00000000001606f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  handle_userfault+0xd3/0xa00 fs/userfaultfd.c:445
>  do_huge_pmd_anonymous_page+0x571/0x850 mm/huge_memory.c:707
>  create_huge_pmd mm/memory.c:3838 [inline]
>  __handle_mm_fault+0xc37/0x1930 mm/memory.c:4042
>  handle_mm_fault+0x215/0x450 mm/memory.c:4108
>  __do_page_fault+0x337/0x6b0 arch/x86/mm/fault.c:1429
>  do_page_fault+0x52/0x330 arch/x86/mm/fault.c:1504
>  page_fault+0x4c/0x60 arch/x86/entry/entry_64.S:1243
> RIP: 0033:0x4453e5
> RSP: 002b:0000000020687000 EFLAGS: 00010217
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00000000004453d9
> RDX: 0000000020b4c000 RSI: 0000000020687000 RDI: 0000000000000600
> RBP: 0000000000000000 R08: 00000000207a4f71 R09: 00007fdf76164700
> R10: 0000000020552ffc R11: 0000000000000202 R12: 0000000000000000
> R13: 00007ffc6b2b2c2f R14: 00007fdf761649c0 R15: 0000000000000000
> Code: 5b 41 5c 41 5d 5d c3 e8 d2 f9 e3 ff 85 db 74 16 e8 c9 f9 e3 ff 8d 53
> 01 89 d8 f0 41 0f b1 55 00 89 c3 74 d7 eb e1 e8 b3 f9 e3 ff <0f> 0b 90 55 48
> 89 e5 53 48 89 fb e8 a3 f9 e3 ff 48 83 3d 73 bb
> RIP: userfaultfd_ctx_get+0x6d/0x70 fs/userfaultfd.c:142 RSP:
> ffffc900012f7c30
> ---[ end trace c25da3c687899c5a ]---
> Kernel panic - not syncing: Fatal exception
> Dumping ftrace buffer:
>    (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
> 
> 

Possibly a duplicate of "KASAN: use-after-free Read in handle_userfault":

https://groups.google.com/d/msg/syzkaller-bugs/sS99S-Z-9No/O4dwVMtVAQAJ

(which *really* needs to be fixed, by the way.  Who is maintaining the
"userfaultfd" feature?)

Eric

--
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>

[PATCH 0/1] Re: kernel BUG at fs/userfaultfd.c:LINE!

[Andrea Arcangeli]

Hello,

Thanks for the CC, I'm temporarily very busy so if there's something
urgent, safer to CC.

This passed both testcases, the hard part was already done. I'm glad
there was nothing wrong in the previous fix that had to be redone.

Simply we forgot to undo the vma->vm_userfaultfd_ctx = NULL after
aborting the new child uffd ctx, the original code of course didn't do
that either.

Having just seen this issue, this isn't very well tested.

Thank you,
Andrea

Andrea Arcangeli (1):
  userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK
    fails

 fs/userfaultfd.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

--
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>

[PATCH 1/1] userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails

[Andrea Arcangeli]

The previous fix 384632e67e0829deb8015ee6ad916b180049d252 corrected
the refcounting in case of UFFD_EVENT_FORK failure for the fork
userfault paths. That still didn't clear the vma->vm_userfaultfd_ctx
of the vmas that were set to point to the aborted new uffd ctx earlier
in dup_userfaultfd.

Cc: stable@vger.kernel.org
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/userfaultfd.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 896f810b6a06..1a88916455bd 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -591,11 +591,14 @@ int handle_userfault(struct vm_fault *vmf, unsigned long reason)
 static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 					      struct userfaultfd_wait_queue *ewq)
 {
+	struct userfaultfd_ctx *release_new_ctx;
+
 	if (WARN_ON_ONCE(current->flags & PF_EXITING))
 		goto out;
 
 	ewq->ctx = ctx;
 	init_waitqueue_entry(&ewq->wq, current);
+	release_new_ctx = NULL;
 
 	spin_lock(&ctx->event_wqh.lock);
 	/*
@@ -622,8 +625,7 @@ static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 				new = (struct userfaultfd_ctx *)
 					(unsigned long)
 					ewq->msg.arg.reserved.reserved1;
-
-				userfaultfd_ctx_put(new);
+				release_new_ctx = new;
 			}
 			break;
 		}
@@ -638,6 +640,20 @@ static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 	__set_current_state(TASK_RUNNING);
 	spin_unlock(&ctx->event_wqh.lock);
 
+	if (release_new_ctx) {
+		struct vm_area_struct *vma;
+		struct mm_struct *mm = release_new_ctx->mm;
+
+		/* the various vma->vm_userfaultfd_ctx still points to it */
+		down_write(&mm->mmap_sem);
+		for (vma = mm->mmap; vma; vma = vma->vm_next)
+			if (vma->vm_userfaultfd_ctx.ctx == release_new_ctx)
+				vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
+		up_write(&mm->mmap_sem);
+
+		userfaultfd_ctx_put(release_new_ctx);
+	}
+
 	/*
 	 * ctx may go away after this if the userfault pseudo fd is
 	 * already released.

--
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>

Re: [PATCH 0/1] Re: kernel BUG at fs/userfaultfd.c:LINE!

[Dmitry Vyukov]

On Sat, Dec 23, 2017 at 1:25 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> Hello,
>
> Thanks for the CC, I'm temporarily very busy so if there's something
> urgent, safer to CC.

Hi,

syzbot uses get_maintainer.pl and for fs/userfaultfd.c you are not
there, so if you want to be CCed please add yourself to MAINTAINERS.


> This passed both testcases, the hard part was already done. I'm glad
> there was nothing wrong in the previous fix that had to be redone.
>
> Simply we forgot to undo the vma->vm_userfaultfd_ctx = NULL after
> aborting the new child uffd ctx, the original code of course didn't do
> that either.
>
> Having just seen this issue, this isn't very well tested.
>
> Thank you,
> Andrea
>
> Andrea Arcangeli (1):
>   userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK
>     fails
>
>  fs/userfaultfd.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)

The original report footer was stripped, so:

Please credit me with: Reported-by: syzbot <syzkaller@googlegroups.com>

and we also need to tell syzbot about the fix with:

#syz fix:
userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails

--
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>

Re: [PATCH 1/1] userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails

[Mike Rapoport]

On Sat, Dec 23, 2017 at 01:25:05AM +0100, Andrea Arcangeli wrote:
> The previous fix 384632e67e0829deb8015ee6ad916b180049d252 corrected
> the refcounting in case of UFFD_EVENT_FORK failure for the fork
> userfault paths. That still didn't clear the vma->vm_userfaultfd_ctx
> of the vmas that were set to point to the aborted new uffd ctx earlier
> in dup_userfaultfd.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>

Reviewed-by: Mike Rapoport <rppt@linux.vnet.ibm.com>

> ---
>  fs/userfaultfd.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 896f810b6a06..1a88916455bd 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -591,11 +591,14 @@ int handle_userfault(struct vm_fault *vmf, unsigned long reason)
>  static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
>  					      struct userfaultfd_wait_queue *ewq)
>  {
> +	struct userfaultfd_ctx *release_new_ctx;

Nit: we could have set release_new_ctx to NULL here...
> +
>  	if (WARN_ON_ONCE(current->flags & PF_EXITING))
>  		goto out;
> 
>  	ewq->ctx = ctx;
>  	init_waitqueue_entry(&ewq->wq, current);
> +	release_new_ctx = NULL;
> 
>  	spin_lock(&ctx->event_wqh.lock);
>  	/*
> @@ -622,8 +625,7 @@ static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
>  				new = (struct userfaultfd_ctx *)
>  					(unsigned long)
>  					ewq->msg.arg.reserved.reserved1;
> -
> -				userfaultfd_ctx_put(new);
> +				release_new_ctx = new;
>  			}
>  			break;
>  		}
> @@ -638,6 +640,20 @@ static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
>  	__set_current_state(TASK_RUNNING);
>  	spin_unlock(&ctx->event_wqh.lock);
> 
> +	if (release_new_ctx) {
> +		struct vm_area_struct *vma;
> +		struct mm_struct *mm = release_new_ctx->mm;
> +
> +		/* the various vma->vm_userfaultfd_ctx still points to it */
> +		down_write(&mm->mmap_sem);
> +		for (vma = mm->mmap; vma; vma = vma->vm_next)
> +			if (vma->vm_userfaultfd_ctx.ctx == release_new_ctx)
> +				vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
> +		up_write(&mm->mmap_sem);
> +
> +		userfaultfd_ctx_put(release_new_ctx);
> +	}
> +
>  	/*
>  	 * ctx may go away after this if the userfault pseudo fd is
>  	 * already released.
> 

-- 
Sincerely yours,
Mike.

--
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>

Re: [PATCH 0/1] Re: kernel BUG at fs/userfaultfd.c:LINE!

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

Hi!

> > Andrea Arcangeli (1):
> >   userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK
> >     fails
> >
> >  fs/userfaultfd.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> The original report footer was stripped, so:
> 
> Please credit me with: Reported-by: syzbot <syzkaller@googlegroups.com>

Please don't. We don't credit our CPUs, and we don't credit Qemu. We
credit humans.

> and we also need to tell syzbot about the fix with:
> 
> #syz fix:
> userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails

Now you claimed you care about bugs being fixed. What about actually
testing Andrea's fix and telling us if it fixes the problem or not,
and maybe saying "thank you"?

Thank you,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/1] Re: kernel BUG at fs/userfaultfd.c:LINE!

[Eric Biggers]

On Wed, Jan 17, 2018 at 09:56:29AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Andrea Arcangeli (1):
> > >   userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK
> > >     fails
> > >
> > >  fs/userfaultfd.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > The original report footer was stripped, so:
> > 
> > Please credit me with: Reported-by: syzbot <syzkaller@googlegroups.com>
> 
> Please don't. We don't credit our CPUs, and we don't credit Qemu. We
> credit humans.
> 

The difference is that unlike your CPU or QEMU, syzbot is a program specifically
written to find and report Linux kernel bugs.  And although Dmitry Vyukov has
done most of the work, syzkaller and syzbot have had many contributors, and you
are welcome to contribute too: https://github.com/google/syzkaller

> > and we also need to tell syzbot about the fix with:
> > 
> > #syz fix:
> > userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails
> 
> Now you claimed you care about bugs being fixed. What about actually
> testing Andrea's fix and telling us if it fixes the problem or not,
> and maybe saying "thank you"?

Of course the syzbot team cares about bugs being fixed, why else would they
report them?

I too would like to see syzbot become smarter about handling bugs with
reproducers.  For example it could bisect to find the commit which introduced
the bug, and could automatically detect where the bug has/hasn't been fixed.  Of
course due to the nature of the kernel it's not possible with every bug, but for
some it is possible.

Nevertheless, at the end of the day, no matter how a bug is reported or who
reports it, it is primarily the responsibility of the person patching the bug to
test their patch.  I've never really understood why people try to patch
reproducible bugs without even testing their fix; it just doesn't make any
sense.  It's pretty easy to run the syzkaller-provided reproducers too.
Personally I've fixed 20+ syzkaller-reported bugs, and I always run the
reproducer if there is one.  In fact the reproducer is usually needed to even
figure out what to fix in the first place...

Yes, Andrea deserves thanks for fixing this bug!  But so does syzbot and its
authors for reporting this bug.  And personally I am not at all impressed by the
fact that userfaultfd has no maintainer listed in MAINTAINERS, nor did any of
the authors feel responsible enough to quickly patch a critical security bug in
code they wrote less than a year ago, even after I Cc'ed them with a simplified
reproducer and explanation of the problem.  Note that userfaultfd is usable by
unprivileged users and is enabled on most major Linux distros.  Does syzbot need
to start automatically requesting CVE's as well? :-)

(And yes, I wanted to fix this myself, as I've done with a lot of other of the
syzbot-reported bugs, but unfortunately I wasn't familiar enough with the
userfaultfd code, and there are 200 other bugs to work on too...)

Eric

--
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>

Re: [PATCH 0/1] Re: kernel BUG at fs/userfaultfd.c:LINE!

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

On Wed 2018-01-17 15:26:31, Eric Biggers wrote:
> On Wed, Jan 17, 2018 at 09:56:29AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > Andrea Arcangeli (1):
> > > >   userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK
> > > >     fails
> > > >
> > > >  fs/userfaultfd.c | 20 ++++++++++++++++++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > The original report footer was stripped, so:
> > > 
> > > Please credit me with: Reported-by: syzbot <syzkaller@googlegroups.com>
> > 
> > Please don't. We don't credit our CPUs, and we don't credit Qemu. We
> > credit humans.
> 
> The difference is that unlike your CPU or QEMU, syzbot is a program specifically
> written to find and report Linux kernel bugs.  And although Dmitry Vyukov has
> done most of the work, syzkaller and syzbot have had many contributors, and you
> are welcome to contribute too: https://github.com/google/syzkaller

No.

Someone is responsible for sending those reports to lkml, and that
someone is not a program, that is a human being.

And that someone should be in the From: address, and he gets the
credit when it goes right, and blame when it gets wrong. Pick that
person. He is responsible for reviewing mails the bot sends (perhaps
adding information that would normally be there but syzbot is not yet
able to add it automatically -- such as what tree it is to the
subject), and he should act on replies.

> > > and we also need to tell syzbot about the fix with:
> > > 
> > > #syz fix:
> > > userfaultfd: clear the vma->vm_userfaultfd_ctx if UFFD_EVENT_FORK fails
> > 
> > Now you claimed you care about bugs being fixed. What about actually
> > testing Andrea's fix and telling us if it fixes the problem or not,
> > and maybe saying "thank you"?
> 
> Of course the syzbot team cares about bugs being fixed, why else would they
> report them?

From the emails it looks like the bot is doing that for fame.

> Nevertheless, at the end of the day, no matter how a bug is reported or who
> reports it, it is primarily the responsibility of the person patching the bug to
> test their patch. 

Umm. Really? That's not how it historically worked. You report a bug,
you are expected to care enough to do the testing. You also say a
"thank you" to person who fixes the bug. Just because.

And syzbot does not do any of that, and that's why human should be in
the loop.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]