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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux