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

[Dan Carpenter]

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))
+		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