Re: [PATCH] io_uring: validate user-controlled cq.head in io_cqe_cache_refill()

在 2026/5/13 22:20, Jens Axboe 写道:
> On 5/13/26 8:18 AM, Jens Axboe wrote:
>> On 5/13/26 12:32 AM, Zizhi Wo wrote:
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index 4ed998d60c09..92e255e9e08f 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -710,11 +710,13 @@ static bool io_fill_nop_cqe(struct io_ring_ctx *ctx, unsigned int off)
>>>    * fill the cq entry
>>>    */
>>>   bool io_cqe_cache_refill(struct io_ring_ctx *ctx, bool overflow, bool cqe32)
>>>   {
>>>   	struct io_rings *rings = ctx->rings;
>>> -	unsigned int off = ctx->cached_cq_tail & (ctx->cq_entries - 1);
>>> +	unsigned int head = READ_ONCE(ctx->rings->cq.head);
>>> +	unsigned int tail = ctx->cached_cq_tail;
>>> +	unsigned int off = tail & (ctx->cq_entries - 1);
>>>   	unsigned int free, queued, len;
>>
>> This looks wrong, as you're snapshotting 'tail' while it could get
>> modified by if a nop fill before the refill happens. And fwiw, looks
>> like the refill part potentially suffers from the same unsigned issue.
> 

Yes. I wasn't aware of io_fill_nop_cqe(), the fact that
cached_cq_tail can be modified between the snapshot and the refill
was missed. The same oversight also means the unsigned issue in that
function went unnoticed...

> To be clearer, I think you want to add a helper ala:
> 
> static unsigned int io_cqring_queued(struct io_ring_ctx *ctx)
> {
> 	struct io_rings *rings = io_get_rings(ctx);
> 	int diff;
> 
> 	diff = (int)( ctx->cached_cq_tail - READ_ONCE(rings->cq.head));
> 	if (diff >= 0)
>          	return min((unsigned int) diff, ctx->cq_entries);
> 	return 0;
> }
> 
> or something like that, and then use it in both spots. Would make for a
> cleaner fix, too.
> 

Thanks for the suggestion. I'll send a v2 using this helper in both
spots.

Thanks,
Zizhi Wo