diff mbox series

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

Message ID 435abccc-9251-4c27-9b35-8fdf4bbd2433@moroto.mountain (mailing list archive)
State New, archived
Headers show
Series accel/qaic: Improve bounds checking in encode/decode | expand

Commit Message

Dan Carpenter June 21, 2023, 7:22 a.m. UTC
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@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(-)

Comments

Jeffrey Hugo July 7, 2023, 7:16 p.m. UTC | #1
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@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;
diff mbox series

Patch

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