linux-kernel - Re: [PATCH] io_uring: fix use-after-free of sq->thread in __io_uring_show

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab764b69-6b94-4163-b114-f4889a043040@kernel.dk>
Date: Tue, 10 Jun 2025 08:33:56 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Penglei Jiang <superman.xpt@...il.com>
Cc: io-uring@...r.kernel.org, linux-kernel@...r.kernel.org,
 syzbot+531502bbbe51d2f769f4@...kaller.appspotmail.com
Subject: Re: [PATCH] io_uring: fix use-after-free of sq->thread in
 __io_uring_show_fdinfo()

On 6/10/25 5:17 AM, Penglei Jiang wrote:
> diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c
> index e9355276ab5d..2911352bbae1 100644
> --- a/io_uring/fdinfo.c
> +++ b/io_uring/fdinfo.c
> @@ -141,19 +141,23 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m)
>  
>  	if (ctx->flags & IORING_SETUP_SQPOLL) {
>  		struct io_sq_data *sq = ctx->sq_data;
> +		struct task_struct *tsk;
>  
> +		rcu_read_lock();
> +		tsk = rcu_dereference(sq->thread);
>  		/*
>  		 * sq->thread might be NULL if we raced with the sqpoll
>  		 * thread termination.
>  		 */
> -		if (sq->thread) {
> +		if (tsk) {
>  			sq_pid = sq->task_pid;
>  			sq_cpu = sq->sq_cpu;
> -			getrusage(sq->thread, RUSAGE_SELF, &sq_usage);
> +			getrusage(tsk, RUSAGE_SELF, &sq_usage);
>  			sq_total_time = (sq_usage.ru_stime.tv_sec * 1000000
>  					 + sq_usage.ru_stime.tv_usec);
>  			sq_work_time = sq->work_time;
>  		}
> +		rcu_read_unlock();
>  	}

Don't think this will work, if we're racing with the mmput and then end
up doing that inside an RCU read locked region...

> diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
> index 03c699493b5a..0625a421626f 100644
> --- a/io_uring/sqpoll.c
> +++ b/io_uring/sqpoll.c
> @@ -270,7 +270,8 @@ static int io_sq_thread(void *data)
>  	/* offload context creation failed, just exit */
>  	if (!current->io_uring) {
>  		mutex_lock(&sqd->lock);
> -		sqd->thread = NULL;
> +		rcu_assign_pointer(sqd->thread, NULL);
> +		put_task_struct(current);
>  		mutex_unlock(&sqd->lock);
>  		goto err_out;
>  	}

You do this in both spots, why the put_task_struct(current)? That seems
like that would be very wrong and instantly break. Did you run this
patch?

I do agree that there's an issue here though, let me take a closer look
at it. This would have been introduced with the getrusage change though,
no? This one:

ommit 3fcb9d17206e31630f802a3ab52081d1342b8ed9
Author: Xiaobing Li <xiaobing.li@...sung.com>
Date:   Wed Feb 28 17:12:51 2024 +0800

    io_uring/sqpoll: statistics of the true utilization of sq threads

as that is the one that added prodding at sq->thread from fdinfo.

-- 
Jens Axboe