* [PATCH v2] aio: Add support for the POLLFREE
@ 2021-10-06 19:50 Ramji Jiyani
2021-10-06 21:16 ` Eric Biggers
0 siblings, 1 reply; 3+ messages in thread
From: Ramji Jiyani @ 2021-10-06 19:50 UTC (permalink / raw)
To: arnd, viro, bcrl
Cc: hch, kernel-team, linux-aio, linux-arch, linux-fsdevel,
linux-kernel, stable, oleg, Ramji Jiyani, Jeff Moyer
Commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread
exits.") fixed the use-after-free in eventpoll but aio still has the
same issue because it doesn't honor the POLLFREE flag.
Add support for the POLLFREE flag to force complete iocb inline in
aio_poll_wake(). A thread may use it to signal it's exit and/or request
to cleanup while pending poll request. In this case, aio_poll_wake()
needs to make sure it doesn't keep any reference to the queue entry
before returning from wake to avoid possible use after free via
poll_cancel() path.
The POLLFREE flag is no more exclusive to the epoll and is being
shared with the aio. Remove comment from poll.h to avoid confusion.
This fixes a use after free issue between binder thread and aio
interactions in certain sequence of events [1].
[1] https://lore.kernel.org/all/CAKUd0B_TCXRY4h1hTztfwWbNSFQqsudDLn2S_28csgWZmZAG3Q@mail.gmail.com/
Signed-off-by: Ramji Jiyani <ramjiyani@google.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
---
Changes since v1:
- Removed parenthesis around POLLFREE macro definition as per review.
- Updated description to refer UAF issue discussion this patch fixes.
- Updated description to remove reference to parenthesis change.
- Added Reviewed-by
---
fs/aio.c | 45 ++++++++++++++++++---------------
include/uapi/asm-generic/poll.h | 2 +-
2 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 51b08ab01dff..5d539c05df42 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1674,6 +1674,7 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync,
{
struct poll_iocb *req = container_of(wait, struct poll_iocb, wait);
struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);
+ struct kioctx *ctx = iocb->ki_ctx;
__poll_t mask = key_to_poll(key);
unsigned long flags;
@@ -1683,29 +1684,33 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync,
list_del_init(&req->wait.entry);
- if (mask && spin_trylock_irqsave(&iocb->ki_ctx->ctx_lock, flags)) {
- struct kioctx *ctx = iocb->ki_ctx;
+ /*
+ * Use irqsave/irqrestore because not all filesystems (e.g. fuse)
+ * call this function with IRQs disabled and because IRQs have to
+ * be disabled before ctx_lock is obtained.
+ */
+ if (mask & POLLFREE) {
+ /* Force complete iocb inline to remove refs to deleted entry */
+ spin_lock_irqsave(&ctx->ctx_lock, flags);
+ } else if (!(mask && spin_trylock_irqsave(&ctx->ctx_lock, flags))) {
+ /* Can't complete iocb inline; schedule for later */
+ schedule_work(&req->work);
+ return 1;
+ }
- /*
- * Try to complete the iocb inline if we can. Use
- * irqsave/irqrestore because not all filesystems (e.g. fuse)
- * call this function with IRQs disabled and because IRQs
- * have to be disabled before ctx_lock is obtained.
- */
- list_del(&iocb->ki_list);
- iocb->ki_res.res = mangle_poll(mask);
- req->done = true;
- if (iocb->ki_eventfd && eventfd_signal_allowed()) {
- iocb = NULL;
- INIT_WORK(&req->work, aio_poll_put_work);
- schedule_work(&req->work);
- }
- spin_unlock_irqrestore(&ctx->ctx_lock, flags);
- if (iocb)
- iocb_put(iocb);
- } else {
+ /* complete iocb inline */
+ list_del(&iocb->ki_list);
+ iocb->ki_res.res = mangle_poll(mask);
+ req->done = true;
+ if (iocb->ki_eventfd && eventfd_signal_allowed()) {
+ iocb = NULL;
+ INIT_WORK(&req->work, aio_poll_put_work);
schedule_work(&req->work);
}
+ spin_unlock_irqrestore(&ctx->ctx_lock, flags);
+ if (iocb)
+ iocb_put(iocb);
+
return 1;
}
diff --git a/include/uapi/asm-generic/poll.h b/include/uapi/asm-generic/poll.h
index 41b509f410bf..f9c520ce4bf4 100644
--- a/include/uapi/asm-generic/poll.h
+++ b/include/uapi/asm-generic/poll.h
@@ -29,7 +29,7 @@
#define POLLRDHUP 0x2000
#endif
-#define POLLFREE (__force __poll_t)0x4000 /* currently only for epoll */
+#define POLLFREE (__force __poll_t)0x4000
#define POLL_BUSY_LOOP (__force __poll_t)0x8000
--
2.33.0.800.g4c38ced690-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] aio: Add support for the POLLFREE
2021-10-06 19:50 [PATCH v2] aio: Add support for the POLLFREE Ramji Jiyani
@ 2021-10-06 21:16 ` Eric Biggers
2021-10-06 22:36 ` Ramji Jiyani
0 siblings, 1 reply; 3+ messages in thread
From: Eric Biggers @ 2021-10-06 21:16 UTC (permalink / raw)
To: Ramji Jiyani
Cc: arnd, viro, bcrl, hch, kernel-team, linux-aio, linux-arch,
linux-fsdevel, linux-kernel, stable, oleg, Jeff Moyer
On Wed, Oct 06, 2021 at 07:50:29PM +0000, Ramji Jiyani wrote:
> Commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread
> exits.") fixed the use-after-free in eventpoll but aio still has the
> same issue because it doesn't honor the POLLFREE flag.
>
> Add support for the POLLFREE flag to force complete iocb inline in
> aio_poll_wake(). A thread may use it to signal it's exit and/or request
> to cleanup while pending poll request. In this case, aio_poll_wake()
> needs to make sure it doesn't keep any reference to the queue entry
> before returning from wake to avoid possible use after free via
> poll_cancel() path.
>
> The POLLFREE flag is no more exclusive to the epoll and is being
> shared with the aio. Remove comment from poll.h to avoid confusion.
>
> This fixes a use after free issue between binder thread and aio
> interactions in certain sequence of events [1].
>
> [1] https://lore.kernel.org/all/CAKUd0B_TCXRY4h1hTztfwWbNSFQqsudDLn2S_28csgWZmZAG3Q@mail.gmail.com/
>
> Signed-off-by: Ramji Jiyani <ramjiyani@google.com>
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Can you add Fixes and Cc stable tags to ensure that this fix gets backported?
Please refer to Documentation/process/submitting-patches.rst.
- Eric
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] aio: Add support for the POLLFREE
2021-10-06 21:16 ` Eric Biggers
@ 2021-10-06 22:36 ` Ramji Jiyani
0 siblings, 0 replies; 3+ messages in thread
From: Ramji Jiyani @ 2021-10-06 22:36 UTC (permalink / raw)
To: Eric Biggers
Cc: arnd, viro, bcrl, hch, kernel-team, linux-aio, linux-arch,
linux-fsdevel, linux-kernel, stable, oleg, Jeff Moyer
On Wed, Oct 6, 2021 at 2:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Oct 06, 2021 at 07:50:29PM +0000, Ramji Jiyani wrote:
> > Commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread
> > exits.") fixed the use-after-free in eventpoll but aio still has the
> > same issue because it doesn't honor the POLLFREE flag.
> >
> > Add support for the POLLFREE flag to force complete iocb inline in
> > aio_poll_wake(). A thread may use it to signal it's exit and/or request
> > to cleanup while pending poll request. In this case, aio_poll_wake()
> > needs to make sure it doesn't keep any reference to the queue entry
> > before returning from wake to avoid possible use after free via
> > poll_cancel() path.
> >
> > The POLLFREE flag is no more exclusive to the epoll and is being
> > shared with the aio. Remove comment from poll.h to avoid confusion.
> >
> > This fixes a use after free issue between binder thread and aio
> > interactions in certain sequence of events [1].
> >
> > [1] https://lore.kernel.org/all/CAKUd0B_TCXRY4h1hTztfwWbNSFQqsudDLn2S_28csgWZmZAG3Q@mail.gmail.com/
> >
> > Signed-off-by: Ramji Jiyani <ramjiyani@google.com>
> > Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>
> Can you add Fixes and Cc stable tags to ensure that this fix gets backported?
> Please refer to Documentation/process/submitting-patches.rst.
>
> - Eric
Thanks Eric. I'll send v3 with these changes soon.
~ Ramji
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-06 22:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06 19:50 [PATCH v2] aio: Add support for the POLLFREE Ramji Jiyani
2021-10-06 21:16 ` Eric Biggers
2021-10-06 22:36 ` Ramji Jiyani
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).