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

[Dan Carpenter]

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
> 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
> we want to transfer (in_trans->size) minus the ammount we have already
> 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);
> 
> The other question I had is should we add a check:
> 
> 	if (remaining == 0)
> 		return 0;
> 
>  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.

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