Re: [PATCH 5.12] io_uring: reg buffer overflow checks hardening

[Pavel Begunkov <asml.silence@gmail.com>]

On 24/03/2021 14:55, Jens Axboe wrote:
> On 3/24/21 8:40 AM, Pavel Begunkov wrote:
>> We are safe with overflows in io_sqe_buffer_register() because it will
>> only yield allocation failure, but it's nicer to check explicitly.
> 
> Right, either that or fault when mapping. So nothing serious here, but
> would be nice to clean up though and just explicitly make it return
> -EOVERFLOW when that is the case.
> 
>> @@ -8306,6 +8306,8 @@ static int io_buffers_map_alloc(struct io_ring_ctx *ctx, unsigned int nr_args)
>>  
>>  static int io_buffer_validate(struct iovec *iov)
>>  {
>> +	u64 tmp, acct_len = iov->iov_len + (PAGE_SIZE - 1);
>> +
> 
> No need for those parens.

Right, but purely formally underflows are UB.

>>  	/*
>>  	 * Don't impose further limits on the size and buffer
>>  	 * constraints here, we'll -EINVAL later when IO is
>> @@ -8318,6 +8320,9 @@ static int io_buffer_validate(struct iovec *iov)
>>  	if (iov->iov_len > SZ_1G)
>>  		return -EFAULT;
>>  
>> +	if (check_add_overflow((u64)iov->iov_base, acct_len, &tmp))
>> +		return -EOVERFLOW;
>> +
> 
> Is this right for 32-bit?

Nope, better be unsigned long.

btw, imu and io_import_fixed does it through u64, that's confusing.

-- 
Pavel Begunkov