Re: [PATCH 1/2] fuse: io-uring: clear ent->fuse_req in commit_fetch error path

[Bernd Schubert]

On 5/17/26 14:59, Berkant Koc wrote:
> [You don't often get email from me@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Berkant Koc <me@xxxxxxxxxx>
> 
> fuse_uring_commit_fetch() locates a request, removes it from the
> processing queue, clears req->ring_entry, then calls
> fuse_ring_ent_set_commit() under queue->lock. On the error branch
> (set_commit returning non-zero because the entry is not in
> FRRS_USERSPACE) the function unlocks the queue and ends the request
> directly with fuse_request_end(), but it never clears ent->fuse_req.
> 
> ent->fuse_req then keeps pointing at the freed fuse_req while the entry
> remains on a queue list. Subsequent teardown via
> fuse_uring_entry_teardown() reads ent->fuse_req under queue->lock and
> hands the dangling pointer to fuse_uring_stop_fuse_req_end(), which
> dereferences it and calls fuse_request_end() a second time on freed
> memory.
> 
> Route the error branch through fuse_uring_req_end() instead. That
> helper acquires queue->lock, clears ent->fuse_req under the lock,
> removes the request from any list it is still on, drops the lock, sets
> req->out.h.error, clears FR_SENT and ends the request. The
> ent->fuse_req = NULL store under the lock is what closes the window
> for the later teardown reader.
> 
> Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support")
> Cc: stable@xxxxxxxxxxxxxxx # 6.14+
> Signed-off-by: Berkant Koc <me@xxxxxxxxxx>
> ---
>  fs/fuse/dev_uring.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 7b9822e8837b..7523569ffdce 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -924,9 +924,7 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
>                 pr_info_ratelimited("qid=%d commit_id %llu state %d",
>                                     queue->qid, commit_id, ent->state);
>                 spin_unlock(&queue->lock);
> -               req->out.h.error = err;
> -               clear_bit(FR_SENT, &req->flags);
> -               fuse_request_end(req);
> +               fuse_uring_req_end(ent, req, err);
>                 return err;
>         }
> 
> --
> 2.47.3

We already had a security report for that on Friday


> 
> [You don't often get email from kipreyyy@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> fuse_uring_commit_fetch() error path calls fuse_request_end(req) without
> clearing ent->fuse_req when fuse_ring_ent_set_commit() fails. The
> still-pending fuse_uring_send_in_task() task-work later dereferences the
> dangling pointer through fuse_uring_prepare_send(), causing a
> use-after-free.
> 
> Clear ent->fuse_req under queue->lock in the error path, matching the
> pattern in fuse_uring_req_end(). Add a NULL check in
> fuse_uring_send_in_task() so the entry is gracefully recycled if the
> request was detached.
> 
> Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support")
> Signed-off-by: Zhenghang Xiao
> ---
>  fs/fuse/dev_uring.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 7b9822e8837b..5e51c36ae2a0 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -921,6 +921,13 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
> 
>         err = fuse_ring_ent_set_commit(ent);
>         if (err != 0) {
> +               /*
> +                * Entry is not in FRRS_USERSPACE state. Clear the
> +                * back-pointer to prevent the still-pending
> +                * fuse_uring_send_in_task() from dereferencing a request
> +                * that is about to be freed.
> +                */
> +               ent->fuse_req = NULL;
>                 pr_info_ratelimited("qid=%d commit_id %llu state %d",
>                                     queue->qid, commit_id, ent->state);
>                 spin_unlock(&queue->lock);
> @@ -1220,6 +1227,15 @@ static void fuse_uring_send_in_task(struct io_tw_req tw_req, io_tw_token_t tw)
>         int err;
> 
>         if (!tw.cancel) {
> +               /*
> +                * If the request was detached (e.g. by fuse_uring_commit_fetch
> +                * error path), fuse_req will be NULL. Bail out and recycle the
> +                * entry for the next request.
> +                */
> +               if (!ent->fuse_req) {
> +                       fuse_uring_next_fuse_req(ent, queue, issue_flags);
> +                       return;
> +               }
>                 err = fuse_uring_prepare_send(ent, ent->fuse_req);
>                 if (err) {
>                         fuse_uring_next_fuse_req(ent, queue, issue_flags);
> --




I had already replied to Zhenghang on Friday, I don't think it is
enough. This condition

>         err = fuse_ring_ent_set_commit(ent);
>         if (err != 0) {

can also be triggered by a bad fuse-server / user-space and a teardown
race. The tear down race is easy, I think.

Fuse-server that wants to trick us is harder, as checking for
ent->fuse_req == NULL in fuse_uring_send_in_task() is not enough, the
race is valid all over the copy operation (fuse_uring_prepare_send()). I
didn't come to it yesterday (was working for main work), going to look
into it a bit later today.


Thanks,
Bernd