| Message ID | 435abccc-9251-4c27-9b35-8fdf4bbd2433@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
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 --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;
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(-)