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@xxxxxxxxxx>
> ---
> 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
