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

Message-ID: <a9bd6c47-f54d-466f-8d33-fceb62b0e985@kernel.dk>
Date: Tue, 10 Jun 2025 08:50:44 -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 8:33 AM, Jens Axboe wrote:
> 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...

I think it needs to look more like:

rcu_read_lock();
tsk = rcu_dereference(sq->thread);

if (tsk) {
	get_task_struct(tsk);
	rcu_read_unlock();
	[...] /* getrusage stuff */
	put_task_struct(tsk);
} else {
	rcu_read_unlock();
}

and then it would be sane and avoid calling getrusage() with the rcu
read lock held.

If you agree, can you send a v2 with those changes?

>> 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?

For some reason I read this as the io_uring.c hunk - the puts here look
fine, ignore the noise on that part...

> 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.

This should still be the Fixes target though, imho.

-- 
Jens Axboe