lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAG48ez3UsadMEHac-muTMvCgLPNV=BnFjn2sNWm59iQ-hxF+rw@mail.gmail.com>
Date:   Wed, 29 Apr 2020 05:30:12 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        NeilBrown <neilb@...e.de>, "Rafael J . Wysocki" <rjw@...k.pl>
Subject: Re: [PATCH] epoll: Fix UAF dentry name access in wakeup source setup

On Wed, Apr 29, 2020 at 4:46 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> On Wed, Apr 29, 2020 at 04:31:04AM +0200, Jann Horn wrote:
> > I'm guessing this will go through akpm's tree?
> >
> >  fs/eventpoll.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 8c596641a72b0..5052a41670479 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -1450,7 +1450,7 @@ static int reverse_path_check(void)
> >
> >  static int ep_create_wakeup_source(struct epitem *epi)
> >  {
> > -     const char *name;
> > +     struct name_snapshot name;
> >       struct wakeup_source *ws;
> >
> >       if (!epi->ep->ws) {
> > @@ -1459,8 +1459,9 @@ static int ep_create_wakeup_source(struct epitem *epi)
> >                       return -ENOMEM;
> >       }
> >
> > -     name = epi->ffd.file->f_path.dentry->d_name.name;
> > -     ws = wakeup_source_register(NULL, name);
> > +     take_dentry_name_snapshot(&name, epi->ffd.file->f_path.dentry);
> > +     ws = wakeup_source_register(NULL, name.name.name);
> > +     release_dentry_name_snapshot(&name);
>
> I'm not sure I like it.  Sure, it won't get freed under you that way; it still
> can go absolutely stale by the time you return from wakeup_source_register().
> What is it being used for?

The one user I'm aware of is Android; they use EPOLLWAKEUP in the
following places:

https://cs.android.com/search?q=EPOLLWAKEUP%20-file:strace%20-file:uapi%2F%20-file:syscall%2Fzerrors%20-file:sys%2Funix%2Fzerrors%20-file:prebuilts%2F%20-file:mod.rs%20-file:libcap-ng%2F%20-file:tests%2F&sq=

I see timerfds, /dev/input/event*, some other stuff with input devices
and video devices, binder, netlink socket, and some other stuff like
that - nothing that's actually likely to be renamed.


Searching on cs.android.com for places that parse this stuff, there
seems to be some code that uploads it as part of bug reports or
something (?), and some parser whose precise purpose I can't figure
out right now:
<https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/com/android/internal/os/KernelWakelockReader.java;l=210?q=%2Fwakeup_sources%20-file:sepolicy%20&ss=android>

(Arve might know what this is actually good for.)


Anyway, I don't think that name is actually particularly critical to
the correct functioning of the system; and the bug is a memory
corruption issue, so we should fix it somehow. And adding
infrastructure to power management so that it can invoke a callback to
figure out the (potentially, under rare circumstances, changing) name
of a wakeup source seems like a bit of overkill to me.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ