[PATCH 4/5] accel/qaic: move and expand integer overflow checks for map_user_pages()

[Jeffrey Hugo]

On 6/21/2023 1:22 AM, Dan Carpenter wrote:
> The integer overflow checking for find_and_map_user_pages() was done in
> encode_dma().  Presumably this was to do it before the allocation.  But
> it's not super important that the failure path is a fast path and it
> hurts readability to put the check so far from the where the variable is
> used.
> 
> Move the check to find_and_map_user_pages() instead and add some more
> additional potential integer overflow checks.
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
> ---
> I kind of went to town adding integer overflow checks here.  Please,
> review this extra carefully.
> 
>   drivers/accel/qaic/qaic_control.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index 96a26539df18..03932197f1ac 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -401,6 +401,12 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
>   
>   	xfer_start_addr = in_trans->addr + resources->xferred_dma_size;
>   
> +	if (in_trans->size == 0 ||
> +	    in_trans->addr + in_trans->size < in_trans->addr ||
> +	    in_trans->addr + resources->xferred_dma_size < in_trans->addr ||
> +	    in_trans->size + offset_in_page(xfer_start_addr) < resources->xferred_dma_size)

These checks seem correct to me.
However, I wonder if it would be better to use check_add_overflow() 
instead of open coding the check?  Feels like that would make the 
purpose of the code clearer and reduce the possibility that the logic is 
wrong.

For the final check, I'm thinking that it does not need to check against 
resources->xferred_dma_size and can check against in_trans->size 
instead, which would then make the use of check_add_overflow() possible. 
  xferred_dma_size should be trusted, and should be <= size.  So then, 
the only way it would appear that check fails is addition overflow, and 
so checking against size should then be valid.

> +		return -EINVAL;
> +
>   	need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) -
>   				  resources->xferred_dma_size, PAGE_SIZE);
>   
> @@ -563,9 +569,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;