[PATCH 5.12] io_uring: reg buffer overflow checks hardening

[PATCH 5.12] io_uring: reg buffer overflow checks hardening

[Pavel Begunkov]

We are safe with overflows in io_sqe_buffer_register() because it will
only yield allocation failure, but it's nicer to check explicitly.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index f3ae83a2d7bc..c33c2eb2a131 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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);
+
 	/*
 	 * 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;
+
 	return 0;
 }
 
-- 
2.24.0

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

[Jens Axboe]

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.

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

-- 
Jens Axboe

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

[Pavel Begunkov]

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