cleanup for req_free/find_next

PATCH In review Pavel Begunkov 3 participants 12 messages Jun 29, 2020 – Jul 7, 2020

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Pavel Begunkov

author

2020-06-29 19:13 UTC #1 lore raw

PATCH 1/5 io_uring: deduplicate freeing linked timeouts

In reply to Pavel Begunkov

Linked timeout cancellation code is repeated in in io_req_link_next() and io_fail_links(), and they differ in details even though shouldn't. Basing on the fact that there is maximum one armed linked timeout in a link, and it immediately follows the head, extract a function that will check for it and defuse.

Justification:
- DRY and cleaner
- better inlining for io_req_link_next() (just 1 call site now)
- isolates linked_timeouts from common path
- reduces time under spinlock for failed links
- actually less code

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 88 +++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 48 deletions(-)

fs/io_uring.c +40 ~~-48~~

index 6710097564de..4cd6d24276c3 100644
@@ -1552,37 +1552,49 @@ static bool io_link_cancel_timeout(struct io_kiocb *req)
 	return false;
 }
 
-static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
+static void io_kill_linked_timeout(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	struct io_kiocb *link;
 	bool wake_ev = false;
+	unsigned long flags = 0; /* false positive warning */
+
+	if (!(req->flags & REQ_F_COMP_LOCKED))
+		spin_lock_irqsave(&ctx->completion_lock, flags);
+
+	if (list_empty(&req->link_list))
+		goto out;
+	link = list_first_entry(&req->link_list, struct io_kiocb, link_list);
+	if (link->opcode != IORING_OP_LINK_TIMEOUT)
+		goto out;
+
+	list_del_init(&link->link_list);
+	wake_ev = io_link_cancel_timeout(link);
+	req->flags &= ~REQ_F_LINK_TIMEOUT;
+out:
+	if (!(req->flags & REQ_F_COMP_LOCKED))
+		spin_unlock_irqrestore(&ctx->completion_lock, flags);
+	if (wake_ev)
+		io_cqring_ev_posted(ctx);
+}
+
+static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
+{
+	struct io_kiocb *nxt;
 
 	/*
 	 * The list should never be empty when we are called here. But could
 	 * potentially happen if the chain is messed up, check to be on the
 	 * safe side.
 	 */
-	while (!list_empty(&req->link_list)) {
-		struct io_kiocb *nxt = list_first_entry(&req->link_list,
-						struct io_kiocb, link_list);
-
-		if (unlikely((req->flags & REQ_F_LINK_TIMEOUT) &&
-			     (nxt->flags & REQ_F_TIMEOUT))) {
-			list_del_init(&nxt->link_list);
-			wake_ev |= io_link_cancel_timeout(nxt);
-			req->flags &= ~REQ_F_LINK_TIMEOUT;
-			continue;
-		}
-
-		list_del_init(&req->link_list);
-		if (!list_empty(&nxt->link_list))
-			nxt->flags |= REQ_F_LINK_HEAD;
-		*nxtptr = nxt;
-		break;
-	}
+	if (unlikely(list_empty(&req->link_list)))
+		return;
 
-	if (wake_ev)
-		io_cqring_ev_posted(ctx);
+	nxt = list_first_entry(&req->link_list, struct io_kiocb, link_list);
+	list_del_init(&req->link_list);
+	if (!list_empty(&nxt->link_list))
+		nxt->flags |= REQ_F_LINK_HEAD;
+	*nxtptr = nxt;
 }
 
 /*

@@ -1591,9 +1603,6 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
 static void io_fail_links(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ctx->completion_lock, flags);
 
 	while (!list_empty(&req->link_list)) {
 		struct io_kiocb *link = list_first_entry(&req->link_list,

@@ -1602,18 +1611,12 @@ static void io_fail_links(struct io_kiocb *req)
 		list_del_init(&link->link_list);
 		trace_io_uring_fail_link(req, link);
 
-		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
-		    link->opcode == IORING_OP_LINK_TIMEOUT) {
-			io_link_cancel_timeout(link);
-		} else {
-			io_cqring_fill_event(link, -ECANCELED);
-			__io_double_put_req(link);
-		}
+		io_cqring_fill_event(link, -ECANCELED);
+		__io_double_put_req(link);
 		req->flags &= ~REQ_F_LINK_TIMEOUT;
 	}
 
 	io_commit_cqring(ctx);
-	spin_unlock_irqrestore(&ctx->completion_lock, flags);
 	io_cqring_ev_posted(ctx);
 }

@@ -1623,30 +1626,19 @@ static void io_req_find_next(struct io_kiocb *req, struct io_kiocb **nxt)
 		return;
 	req->flags &= ~REQ_F_LINK_HEAD;
 
+	if (req->flags & REQ_F_LINK_TIMEOUT)
+		io_kill_linked_timeout(req);
+
 	/*
 	 * If LINK is set, we have dependent requests in this chain. If we
 	 * didn't fail this request, queue the first one up, moving any other
 	 * dependencies to the next request. In case of failure, fail the rest
 	 * of the chain.
 	 */
-	if (req->flags & REQ_F_FAIL_LINK) {
+	if (req->flags & REQ_F_FAIL_LINK)
 		io_fail_links(req);
-	} else if ((req->flags & (REQ_F_LINK_TIMEOUT | REQ_F_COMP_LOCKED)) ==
-			REQ_F_LINK_TIMEOUT) {
-		struct io_ring_ctx *ctx = req->ctx;
-		unsigned long flags;
-
-		/*
-		 * If this is a timeout link, we could be racing with the
-		 * timeout timer. Grab the completion lock for this case to
-		 * protect against that.
-		 */
-		spin_lock_irqsave(&ctx->completion_lock, flags);
-		io_req_link_next(req, nxt);
-		spin_unlock_irqrestore(&ctx->completion_lock, flags);
-	} else {
+	else
 		io_req_link_next(req, nxt);
-	}
 }
 
 static void __io_req_task_cancel(struct io_kiocb *req, int error)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-06-29 20:43 UTC #5 lore raw

PATCH 5/5 io_uring: fix use after free

In reply to Pavel Begunkov

After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/io_uring.c +5 -2

index 40996f25822e..013f31e35e78 100644
@@ -1526,12 +1526,15 @@ static void io_dismantle_req(struct io_kiocb *req)
 
 static void __io_free_req(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx;
+
 	io_dismantle_req(req);
-	percpu_ref_put(&req->ctx->refs);
+	ctx = req->ctx;
 	if (likely(!io_is_fallback_req(req)))
 		kmem_cache_free(req_cachep, req);
 	else
-		clear_bit_unlock(0, (unsigned long *) &req->ctx->fallback_req);
+		clear_bit_unlock(0, (unsigned long *) &ctx->fallback_req);
+	percpu_ref_put(&ctx->refs);
 }
 
 static bool io_link_cancel_timeout(struct io_kiocb *req)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-06-29 20:45 UTC #0 lore raw

cover cleanup for req_free/find_next

Cleans a bit up completion path.

Regarding [5/5], it's so unlikely to happen, that I don't even try to port it for 5.8, there would be merge conflicts. However, if anyone think otherwise or this ever showed itself -- let me know.

Pavel Begunkov (5):
io_uring: deduplicate freeing linked timeouts
io_uring: replace find_next() out param with ret
io_uring: kill REQ_F_TIMEOUT
io_uring: kill REQ_F_TIMEOUT_NOSEQ
io_uring: fix use after free

 fs/io_uring.c | 140 ++++++++++++++++++++++++--------------------------
 1 file changed, 66 insertions(+), 74 deletions(-)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-06-29 20:46 UTC #3 lore raw

PATCH 3/5 io_uring: kill REQ_F_TIMEOUT

Now REQ_F_TIMEOUT is set but never used, kill it

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 4 ----
 1 file changed, 4 deletions(-)

fs/io_uring.c +0 -4

index 52e5c8730dd5..159882906a24 100644
@@ -531,7 +531,6 @@ enum {
 	REQ_F_CUR_POS_BIT,
 	REQ_F_NOWAIT_BIT,
 	REQ_F_LINK_TIMEOUT_BIT,
-	REQ_F_TIMEOUT_BIT,
 	REQ_F_ISREG_BIT,
 	REQ_F_TIMEOUT_NOSEQ_BIT,
 	REQ_F_COMP_LOCKED_BIT,

@@ -574,8 +573,6 @@ enum {
 	REQ_F_NOWAIT		= BIT(REQ_F_NOWAIT_BIT),
 	/* has linked timeout */
 	REQ_F_LINK_TIMEOUT	= BIT(REQ_F_LINK_TIMEOUT_BIT),
-	/* timeout request */
-	REQ_F_TIMEOUT		= BIT(REQ_F_TIMEOUT_BIT),
 	/* regular file */
 	REQ_F_ISREG		= BIT(REQ_F_ISREG_BIT),
 	/* no timeout sequence */

@@ -5040,7 +5037,6 @@ static int io_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 
 	data = &req->io->timeout;
 	data->req = req;
-	req->flags |= REQ_F_TIMEOUT;
 
 	if (get_timespec64(&data->ts, u64_to_user_ptr(sqe->addr)))
 		return -EFAULT;

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-06-29 21:03 UTC #2 lore raw

PATCH 2/5 io_uring: replace find_next() out param with ret

In reply to Pavel Begunkov

Generally, it's better to return a value directly than having out parameter. It's cleaner and saves from some kinds of ugly bugs. May also be faster.

Return next request from io_req_find_next() and friends directly instead of passing out parameter.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

fs/io_uring.c +18 ~~-18~~

index 4cd6d24276c3..52e5c8730dd5 100644
@@ -1578,7 +1578,7 @@ static void io_kill_linked_timeout(struct io_kiocb *req)
 		io_cqring_ev_posted(ctx);
 }
 
-static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
+static struct io_kiocb *io_req_link_next(struct io_kiocb *req)
 {
 	struct io_kiocb *nxt;

@@ -1588,13 +1588,13 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
 	 * safe side.
 	 */
 	if (unlikely(list_empty(&req->link_list)))
-		return;
+		return NULL;
 
 	nxt = list_first_entry(&req->link_list, struct io_kiocb, link_list);
 	list_del_init(&req->link_list);
 	if (!list_empty(&nxt->link_list))
 		nxt->flags |= REQ_F_LINK_HEAD;
-	*nxtptr = nxt;
+	return nxt;
 }
 
 /*

@@ -1620,10 +1620,10 @@ static void io_fail_links(struct io_kiocb *req)
 	io_cqring_ev_posted(ctx);
 }
 
-static void io_req_find_next(struct io_kiocb *req, struct io_kiocb **nxt)
+static struct io_kiocb *io_req_find_next(struct io_kiocb *req)
 {
 	if (likely(!(req->flags & REQ_F_LINK_HEAD)))
-		return;
+		return NULL;
 	req->flags &= ~REQ_F_LINK_HEAD;
 
 	if (req->flags & REQ_F_LINK_TIMEOUT)

@@ -1635,10 +1635,10 @@ static void io_req_find_next(struct io_kiocb *req, struct io_kiocb **nxt)
 	 * dependencies to the next request. In case of failure, fail the rest
 	 * of the chain.
 	 */
-	if (req->flags & REQ_F_FAIL_LINK)
-		io_fail_links(req);
-	else
-		io_req_link_next(req, nxt);
+	if (likely(!(req->flags & REQ_F_FAIL_LINK)))
+		return io_req_link_next(req);
+	io_fail_links(req);
+	return NULL;
 }
 
 static void __io_req_task_cancel(struct io_kiocb *req, int error)

@@ -1701,9 +1701,8 @@ static void io_req_task_queue(struct io_kiocb *req)
 
 static void io_queue_next(struct io_kiocb *req)
 {
-	struct io_kiocb *nxt = NULL;
+	struct io_kiocb *nxt = io_req_find_next(req);
 
-	io_req_find_next(req, &nxt);
 	if (nxt)
 		io_req_task_queue(nxt);
 }

@@ -1753,13 +1752,15 @@ static void io_req_free_batch(struct req_batch *rb, struct io_kiocb *req)
  * Drop reference to request, return next in chain (if there is one) if this
  * was the last reference to this request.
  */
-__attribute__((nonnull))
-static void io_put_req_find_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
+static struct io_kiocb *io_put_req_find_next(struct io_kiocb *req)
 {
+	struct io_kiocb *nxt = NULL;
+
 	if (refcount_dec_and_test(&req->refs)) {
-		io_req_find_next(req, nxtptr);
+		nxt = io_req_find_next(req);
 		__io_free_req(req);
 	}
+	return nxt;
 }
 
 static void io_put_req(struct io_kiocb *req)

@@ -1780,7 +1781,7 @@ static struct io_wq_work *io_steal_work(struct io_kiocb *req)
 	if (refcount_read(&req->refs) != 1)
 		return NULL;
 
-	io_req_find_next(req, &nxt);
+	nxt = io_req_find_next(req);
 	if (!nxt)
 		return NULL;

@@ -4465,7 +4466,7 @@ static void io_poll_task_handler(struct io_kiocb *req, struct io_kiocb **nxt)
 	hash_del(&req->hash_node);
 	io_poll_complete(req, req->result, 0);
 	req->flags |= REQ_F_COMP_LOCKED;
-	io_put_req_find_next(req, nxt);
+	*nxt = io_put_req_find_next(req);
 	spin_unlock_irq(&ctx->completion_lock);
 
 	io_cqring_ev_posted(ctx);

@@ -5915,9 +5916,8 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	}
 
 err:
-	nxt = NULL;
 	/* drop submission reference */
-	io_put_req_find_next(req, &nxt);
+	nxt = io_put_req_find_next(req);
 
 	if (linked_timeout) {
 		if (!ret)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-06-29 21:03 UTC #4 lore raw

PATCH 4/5 io_uring: kill REQ_F_TIMEOUT_NOSEQ

In reply to Pavel Begunkov

There are too many useless flags, kill REQ_F_TIMEOUT_NOSEQ, which can be easily infered from req.timeout itself.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

fs/io_uring.c +8 -7

index 159882906a24..40996f25822e 100644
@@ -532,7 +532,6 @@ enum {
 	REQ_F_NOWAIT_BIT,
 	REQ_F_LINK_TIMEOUT_BIT,
 	REQ_F_ISREG_BIT,
-	REQ_F_TIMEOUT_NOSEQ_BIT,
 	REQ_F_COMP_LOCKED_BIT,
 	REQ_F_NEED_CLEANUP_BIT,
 	REQ_F_OVERFLOW_BIT,

@@ -575,8 +574,6 @@ enum {
 	REQ_F_LINK_TIMEOUT	= BIT(REQ_F_LINK_TIMEOUT_BIT),
 	/* regular file */
 	REQ_F_ISREG		= BIT(REQ_F_ISREG_BIT),
-	/* no timeout sequence */
-	REQ_F_TIMEOUT_NOSEQ	= BIT(REQ_F_TIMEOUT_NOSEQ_BIT),
 	/* completion under lock */
 	REQ_F_COMP_LOCKED	= BIT(REQ_F_COMP_LOCKED_BIT),
 	/* needs cleanup */

@@ -1010,6 +1007,11 @@ static void io_ring_ctx_ref_free(struct percpu_ref *ref)
 	complete(&ctx->ref_comp);
 }
 
+static inline bool io_is_timeout_noseq(struct io_kiocb *req)
+{
+	return !req->timeout.off;
+}
+
 static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 {
 	struct io_ring_ctx *ctx;

@@ -1222,7 +1224,7 @@ static void io_flush_timeouts(struct io_ring_ctx *ctx)
 		struct io_kiocb *req = list_first_entry(&ctx->timeout_list,
 							struct io_kiocb, list);
 
-		if (req->flags & REQ_F_TIMEOUT_NOSEQ)
+		if (io_is_timeout_noseq(req))
 			break;
 		if (req->timeout.target_seq != ctx->cached_cq_tail
 					- atomic_read(&ctx->cq_timeouts))

@@ -5064,8 +5066,7 @@ static int io_timeout(struct io_kiocb *req)
 	 * timeout event to be satisfied. If it isn't set, then this is
 	 * a pure timeout request, sequence isn't used.
 	 */
-	if (!off) {
-		req->flags |= REQ_F_TIMEOUT_NOSEQ;
+	if (io_is_timeout_noseq(req)) {
 		entry = ctx->timeout_list.prev;
 		goto add;
 	}

@@ -5080,7 +5081,7 @@ static int io_timeout(struct io_kiocb *req)
 	list_for_each_prev(entry, &ctx->timeout_list) {
 		struct io_kiocb *nxt = list_entry(entry, struct io_kiocb, list);
 
-		if (nxt->flags & REQ_F_TIMEOUT_NOSEQ)
+		if (io_is_timeout_noseq(nxt))
 			continue;
 		/* nxt.seq is behind @tail, otherwise would've been completed */
 		if (off >= nxt->timeout.target_seq - tail)

Signature · 1 line

2.24.0

Jann Horn

2020-07-03 02:40 UTC #6 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

In reply to Pavel Begunkov

Quoted 3 lines · Pavel Begunkov

After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.

Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.

And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.

Pavel Begunkov

author

2020-07-03 19:50 UTC #7 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

Quoted 11 lines · Jann Horn

Quoted 3 lines · Pavel Begunkov
After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.
Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.

It was there from the beginning,
0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")

Quoted 4 lines

And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.

As mentioned in the cover letter, it's pretty unlikely to ever happen. No one seems to have seen it since its introduction in November 2019. And as the patch can't be backported automatically, not sure it's worth the effort. Am I misjudging here?

Signature · 1 line

Pavel Begunkov

Jann Horn

2020-07-03 21:33 UTC #8 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

Quoted 17 lines · Pavel Begunkov

Quoted 11 lines · Jann Horn
Quoted 3 lines · Pavel Begunkov
After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.
Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.
It was there from the beginning,
0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.
As mentioned in the cover letter, it's pretty unlikely to ever happen. No one seems to have seen it since its introduction in November 2019. And as the patch can't be backported automatically, not sure it's worth the effort. Am I misjudging here?

Use-after-free bugs are often security bugs; in particular when, as in this case, data is written through the freed pointer. That means that even if this is extremely unlikely to occur in practice under normal circumstances, you should assume that someone may invest a significant amount of time into engineering some way to make this bug happen. If you can show that the bug is _impossible_ to hit, that's fine, I guess. But if it's merely "it's a really tight race and unlikely to happen", I think we should be fixing it on the stable branches. For example, on kernels with PREEMPT=y (typically you get that config either with "lowlatency" kernels or on Android, I think), attackers can play games like giving their own tasks "idle" scheduling priority and intentionally preempting them right in the middle of a race window, which makes it possible to delay execution for intervals on the order of seconds if the attacker can manage to make the scheduler IPI hit in the right place.

I guess one way to hit this bug on mainline would be to go through the io_async_task_func() canceled==true case, right? That will drop references on a request at the very end, without holding any locks or so that might keep the context alive.

Pavel Begunkov

author

2020-07-04 06:51 UTC #9 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

Quoted 27 lines · Jann Horn

Quoted 17 lines · Pavel Begunkov
Quoted 11 lines · Jann Horn
Quoted 3 lines · Pavel Begunkov
After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.
Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.
It was there from the beginning,
0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.
As mentioned in the cover letter, it's pretty unlikely to ever happen. No one seems to have seen it since its introduction in November 2019. And as the patch can't be backported automatically, not sure it's worth the effort. Am I misjudging here?

Quoted 5 lines

Use-after-free bugs are often security bugs; in particular when, as in this case, data is written through the freed pointer. That means that even if this is extremely unlikely to occur in practice under normal circumstances, you should assume that someone may invest a significant amount of time into engineering some way to make this bug happen. If

Good point, agree.

Quoted 11 lines

you can show that the bug is _impossible_ to hit, that's fine, I guess. But if it's merely "it's a really tight race and unlikely to happen", I think we should be fixing it on the stable branches. For example, on kernels with PREEMPT=y (typically you get that config either with "lowlatency" kernels or on Android, I think), attackers can play games like giving their own tasks "idle" scheduling priority and intentionally preempting them right in the middle of a race window, which makes it possible to delay execution for intervals on the order of seconds if the attacker can manage to make the scheduler IPI hit in the right place.

Quoted 4 lines

I guess one way to hit this bug on mainline would be to go through the io_async_task_func() canceled==true case, right? That will drop references on a request at the very end, without holding any locks or so that might keep the context alive.

Yes, for an example, but even simpler to submit async reads/writes (i.e. with kiocb->ki_complete set) that would trigger io_complete_rw() from irq. Or do something with timeout or poll requests.

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2020-07-07 12:48 UTC #10 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

Quoted 28 lines · Pavel Begunkov

Quoted 27 lines · Jann Horn
Quoted 25 lines · Pavel Begunkov
Quoted 11 lines · Jann Horn
Quoted 3 lines · Pavel Begunkov
After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.
Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.
It was there from the beginning,
0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")
Quoted 4 lines
And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.
As mentioned in the cover letter, it's pretty unlikely to ever happen. No one seems to have seen it since its introduction in November 2019. And as the patch can't be backported automatically, not sure it's worth the effort. Am I misjudging here?

Quoted 5 lines

Quoted 5 lines
Use-after-free bugs are often security bugs; in particular when, as in this case, data is written through the freed pointer. That means that even if this is extremely unlikely to occur in practice under normal circumstances, you should assume that someone may invest a significant amount of time into engineering some way to make this bug happen. If

Jens, how would you prefer to handle this for 5.8? I can send a patch, but
1. it's fixed in for-5.9
2. it would be a merge conflict regardless of 1.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-07-07 13:56 UTC #11 lore raw

Re: [PATCH 5/5] io_uring: fix use after free

Quoted 35 lines · Pavel Begunkov

Quoted 28 lines · Pavel Begunkov
Quoted 27 lines · Jann Horn
Quoted 25 lines · Pavel Begunkov
Quoted 11 lines · Jann Horn
Quoted 3 lines · Pavel Begunkov
After __io_free_req() put a ctx ref, it should assumed that the ctx may already be gone. However, it can be accessed to put back fallback req. Free req first and then put a req.
Please stick "Fixes" tags on bug fixes to make it easy to see when the fixed bug was introduced (especially for ones that fix severe issues like UAFs). From a cursory glance, it kinda seems like this one _might_ have been introduced in 2b85edfc0c90ef, which would mean that it landed in 5.6? But I can't really tell for sure without investing more time; you probably know that better.
It was there from the beginning,
0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")
Quoted 4 lines
And if this actually does affect existing releases, please also stick a "Cc: [email protected]" tag on it so that the fix can be shipped to users of those releases.
As mentioned in the cover letter, it's pretty unlikely to ever happen. No one seems to have seen it since its introduction in November 2019. And as the patch can't be backported automatically, not sure it's worth the effort. Am I misjudging here?
Quoted 5 lines
Quoted 5 lines
Use-after-free bugs are often security bugs; in particular when, as in this case, data is written through the freed pointer. That means that even if this is extremely unlikely to occur in practice under normal circumstances, you should assume that someone may invest a significant amount of time into engineering some way to make this bug happen. If

Quoted 3 lines

Jens, how would you prefer to handle this for 5.8? I can send a patch, but
1. it's fixed in for-5.9
2. it would be a merge conflict regardless of 1.

Given the type of bug and conditions required to trigger it, I think we should just send it in for stable once it lands in 5.9. We need GFP_KERNEL allocations to fail, the fallback req the last to be released, the ctx freed (and reused) in an extremely short window.

If it was more severe or easier to trigger, then we could deal with the pain of the conflict. But I don't think it's worth it for this one.

Signature · 1 line

Jens Axboe