| Message ID | ZK0Q8hNiX5JlUPm3@moroto (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
On 7/11/2023 1:51 PM, 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> > --- > no change > > 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 23680f5f1902..d5ce36cb351f 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -402,6 +402,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 || Why not use xfer_start_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); > > @@ -564,9 +570,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 23680f5f1902..d5ce36cb351f 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -402,6 +402,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); @@ -564,9 +570,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> --- no change drivers/accel/qaic/qaic_control.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)