kernel BUG at fs/userfaultfd.c:LINE!

[syzbot]

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


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.
Please credit me with: Reported-by: syzbot <syzk...@googlegroups.com>

syzbot will keep track of this bug report.
Once a fix for this bug is merged into any tree, reply to this email with:
#syz fix: exact-commit-title
If you want to test a patch for this bug, please reply with:
#syz test: git://repo/address.git branch
and provide the patch inline or as an attachment.
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.

[Eric Biggers]

[+Cc aarc...@redhat.com, linu...@kvack.org]

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

[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: sta...@vger.kernel.org
Signed-off-by: Andrea Arcangeli <aarc...@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.

[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

[Dmitry Vyukov]

On Sat, Dec 23, 2017 at 1:25 AM, Andrea Arcangeli <aarc...@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 <syzk...@googlegroups.com>

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

#syz fix:

[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: sta...@vger.kernel.org
> Signed-off-by: Andrea Arcangeli <aarc...@redhat.com>

Reviewed-by: Mike Rapoport <rp...@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...

--
Sincerely yours,
Mike.

[Pavel Machek]

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 <syzk...@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

[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 <syzk...@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

[Pavel Machek]

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 <syzk...@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.

[Eric Biggers]

On Fri, Dec 22, 2017 at 01:37:01PM -0800, syzbot wrote:

This crash is no longer occurring, presumably was fixed by the following commit
so telling syzbot: