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;
