* [PATCH] epoll: Fix UAF dentry name access in wakeup source setup
@ 2020-04-29 2:31 Jann Horn
2020-04-29 2:46 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: Jann Horn @ 2020-04-29 2:31 UTC (permalink / raw)
To: Andrew Morton
Cc: Alexander Viro, linux-fsdevel, linux-kernel,
Arve Hjønnevåg, NeilBrown, Rafael J . Wysocki
In ep_create_wakeup_source(), epi->ffd.file is some random file we're
watching with epoll, so it might well be renamed concurrently. And when a
file gets renamed, the buffer containing its name may be freed.
This can be reproduced by racing a task that keeps adding and removing
EPOLLWAKEUP epoll entries for a fifo with another task that keeps renaming
the fifo between two long names if you add an mdelay(200) call directly
before wakeup_source_register(); KASAN then complains:
BUG: KASAN: use-after-free in strlen+0xa/0x40
Read of size 1 at addr ffff888065fda990 by task wakemeup/2375
[...]
Call Trace:
[...]
strlen+0xa/0x40
kstrdup+0x1a/0x60
wakeup_source_create+0x43/0xb0
wakeup_source_register+0x13/0x60
ep_create_wakeup_source+0x7f/0xf0
do_epoll_ctl+0x13d0/0x1880
[...]
__x64_sys_epoll_ctl+0xc3/0x110
[...]
Allocated by task 2376:
[...]
__d_alloc+0x323/0x3c0
d_alloc+0x30/0xf0
__lookup_hash+0x61/0xc0
do_renameat2+0x3fa/0x6d0
__x64_sys_rename+0x3a/0x40
[...]
Freed by task 2379:
[...]
kfree_rcu_work+0x9b/0x5d0
[...]
Backporting note: This patch depends on commit 49d31c2f389a ("dentry name
snapshots"). Maybe that one should also be backported as a dependency for
pre-v4.13? (Sorry, I wasn't sure how to properly express this as a "Fixes:"
tag.)
Cc: stable@vger.kernel.org
Fixes: 4d7e30d98939 ("epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll events are ready")
Signed-off-by: Jann Horn <jannh@google.com>
---
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);
if (!ws)
return -ENOMEM;
base-commit: 96c9a7802af7d500a582d89a8b864584fe878c1b
--
2.26.2.303.gf8c07b1a785-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] epoll: Fix UAF dentry name access in wakeup source setup
2020-04-29 2:31 [PATCH] epoll: Fix UAF dentry name access in wakeup source setup Jann Horn
@ 2020-04-29 2:46 ` Al Viro
2020-04-29 3:30 ` Jann Horn
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2020-04-29 2:46 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-fsdevel, linux-kernel,
Arve Hjønnevåg, NeilBrown, Rafael J . Wysocki
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] epoll: Fix UAF dentry name access in wakeup source setup
2020-04-29 2:46 ` Al Viro
@ 2020-04-29 3:30 ` Jann Horn
0 siblings, 0 replies; 3+ messages in thread
From: Jann Horn @ 2020-04-29 3:30 UTC (permalink / raw)
To: Al Viro
Cc: Andrew Morton, linux-fsdevel, kernel list,
Arve Hjønnevåg, NeilBrown, Rafael J . Wysocki
On Wed, Apr 29, 2020 at 4:46 AM Al Viro <viro@zeniv.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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-29 3:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 2:31 [PATCH] epoll: Fix UAF dentry name access in wakeup source setup Jann Horn
2020-04-29 2:46 ` Al Viro
2020-04-29 3:30 ` Jann Horn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).