[PATCH v5] accel/qaic: tighten integer overflow checking in map_user_pages()

Wed Aug 9 18:15:29 UTC 2023

On 8/7/2023 8:43 AM, Dan Carpenter wrote:
> On Mon, Aug 07, 2023 at 05:09:34PM +0300, Dan Carpenter wrote:
>> The encode_dma() function has some validation on in_trans->size but it's
>> not complete and it would be more clear to move those checks to
>> find_and_map_user_pages().
>>
>> The encode_dma() had two checks:
>>
>> 	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
>> 		return -EINVAL;
>>
>> It's not sufficeint to just check if in_trans->size is zero.  The

sufficient

>> resources->xferred_dma_size variable represent the number of bytes
>> already transferred.  If we have already transferred more bytes than
>> in_trans->size then there are negative bytes remaining which doesn't
>> make sense.  Check for that as well.
>>
>> I introduced a new variable "remaining" which represents the amount

introduce (commit text should be present tense per my understanding)

>> we want to transfer (in_trans->size) minus the ammount we have already

amount

>> transferred (resources->xferred_dma_size).
>>
>> The check in encode_dma() checked that "addr + size" could not overflow
>> however we may already have transferred some bytes so the real starting
>> address is "xfer_start_addr" so check that "xfer_start_addr + size"
>> cannot overflow instead.  Also check that "addr +
>> resources->xferred_dma_size cannot overflow.
>>
>> My other concern was that we are dealing with u64 values but on 32bit
>> systems the kmalloc() function will truncate the sizes to 32 bits.  So
>> I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);"
>> and returned -EINVAL if it were >= SIZE_MAX.  This will not affect 64bit
>> systems.
>>
>> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
>> Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
>> ---
>> This is re-write re-write of the previous version.
>>
>> I am not necessarily sure it is correct.  Please review carefully.  In
>> particular, please check how "total" is calculated.  Maybe it would make
>> more sense to write that as:
>>
>> 	total = remaining + offset_in_page(xfer_start_addr);

I think this makes more sense.

>>
>> The other question I had is should we add a check:
>>
>> 	if (remaining == 0)
>> 		return 0;

I don't see why adding this would hurt anything.  I don't believe it is 
necessary.  Remaining is a function of of the driver code and not an 
external input.  The driver transfers as much as it can, and stops when 
everything is sent (remaining == 0).  If we hit this check, it is a 
driver bug.

>>
>>   drivers/accel/qaic/qaic_control.c | 23 +++++++++++++++--------
>>   1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
>> index cfbc92da426f..d64505bcf4ae 100644
>> --- a/drivers/accel/qaic/qaic_control.c
>> +++ b/drivers/accel/qaic/qaic_control.c
>> @@ -392,18 +392,28 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
>>   				   struct qaic_manage_trans_dma_xfer *in_trans,
>>   				   struct ioctl_resources *resources, struct dma_xfer *xfer)
>>   {
>> +	u64 xfer_start_addr, remaining, end, total;
>>   	unsigned long need_pages;
>>   	struct page **page_list;
>>   	unsigned long nr_pages;
>>   	struct sg_table *sgt;
>> -	u64 xfer_start_addr;
>>   	int ret;
>>   	int i;
>>   
>> -	xfer_start_addr = in_trans->addr + resources->xferred_dma_size;
>> +	if (check_add_overflow(in_trans->addr, resources->xferred_dma_size, &xfer_start_addr))
>> +		return -EINVAL;
>> +
>> +	if (in_trans->size == 0 ||
>> +	    in_trans->size < resources->xferred_dma_size ||
>> +	    check_add_overflow(xfer_start_addr, in_trans->size, &end))
>                                                  ^^^^^^^^^^^^^^
> This should be remaining.  So maybe it should be something like this
> with a return 0 for no bytes remaining and total calculated differently.

Yep, should be remaining.  Your below proposed version looks good to me.

> 
> 	if (check_add_overflow(in_trans->addr, resources->xferred_dma_size, &xfer_start_addr))
> 		return -EINVAL;
> 
> 	if (in_trans->size < resources->xferred_dma_size)
> 		return -EINVAL;

This check should never hit unless we have a driver bug.  I don't object 
to having it though.

> 	remaining = in_trans->size - resources->xferred_dma_size;
> 	if (remaining == 0)
> 		return 0;
> 
> 	if (check_add_overflow(xfer_start_addr, remaining, &end))
> 		return -EINVAL;
> 
> 	/* Still not really sure why total is calculated this way */

Physical memory layout.

Lets say remaining is 4k, and PAGE_SIZE is 4k.

4k / 4k = 1 so we need 1 page.

Except, if that 4k remaining is some remainder of the transfer and not 
the complete transfer, then where we start the transfer matters.

If the remaining 4k starts right at a page boundary, then we just need a 
single page.  However, if the remaining 4k starts X bytes into a page 
(where X is non-zero), we would actually be transferring data from two 
physical pages, even though 4k can be fully represented by one page.

This representation might make a bit more sense (although I argue it is 
wrong) -

total = remaining;
need_pages = DIV_ROUND_UP(total, PAGE_SIZE);
if (offset_in_page(xfer_state_addr))
	need_pages++;

Where this breaks down is when the start addr and the remaining combine 
to fit into a page.

Assume remaining == 5k and PAGE_SIZE == 4k.  offset_in_page() is going 
to return 1k.

The right answer is 2 pages.  The first page will contain 3k (4k - 1k 
from the offset) and the second page will contain 2k.

DIV_ROUND_UP(remaining, PAGE_SIZE) == 5k / 4k == 2

DIV_ROUND_UP(remaining + offset_in_page(), PAGE_SIZE) == (5k + 1k) / 4k == 2

Seems like we need a comment explaining this.

> 	total = remaining + offset_in_page(xfer_start_addr);
> 	if (total >= SIZE_MAX)
> 		return -EINVAL;
> 
> 	need_pages = DIV_ROUND_UP(total, PAGE_SIZE);
> 
> regards,
> dan carpenter
> 
>> +		return -EINVAL;
>>   
>> -	need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) -
>> -				  resources->xferred_dma_size, PAGE_SIZE);
>> +	remaining = in_trans->size - resources->xferred_dma_size;
>> +	total = in_trans->size + offset_in_page(xfer_start_addr);
>> +	if (total >= SIZE_MAX)
>> +		return -EINVAL;
>> +
>> +	need_pages = DIV_ROUND_UP(total - resources->xferred_dma_size, PAGE_SIZE);
>>   
>>   	nr_pages = need_pages;
>>   
>> @@ -435,7 +445,7 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
>>   
>>   	ret = sg_alloc_table_from_pages(sgt, page_list, nr_pages,
>>   					offset_in_page(xfer_start_addr),
>> -					in_trans->size - resources->xferred_dma_size, GFP_KERNEL);
>> +					remaining, GFP_KERNEL);
>>   	if (ret) {
>>   		ret = -ENOMEM;
>>   		goto free_sgt;
>> @@ -566,9 +576,6 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list
>>   	    QAIC_MANAGE_EXT_MSG_LENGTH)
>>   		return -ENOMEM;
>>   
>> -	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
>> -		return -EINVAL;
>> -
>>   	xfer = kmalloc(sizeof(*xfer), GFP_KERNEL);
>>   	if (!xfer)
>>   		return -ENOMEM;
>> -- 
>> 2.39.2