| Message ID | 24d3348b-25ac-4c1b-b171-9dae7c43e4e0@moroto.mountain |
|---|---|
| State | Accepted |
| Commit | 96d3c1cadedb6ae2e8965e19cd12caa244afbd9c |
| Headers | show |
| Series | accel/qaic: Clean up integer overflow checking in map_user_pages() | expand |
On 8/10/2023 6:23 AM, Dan Carpenter wrote: > The encode_dma() function has some validation on in_trans->size but it > would be more clear to move those checks to find_and_map_user_pages(). > > The encode_dma() had two checks: > > if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) > return -EINVAL; > > The in_trans->addr variable is the starting address. The in_trans->size > variable is the total size of the transfer. The transfer can occur in > parts and the resources->xferred_dma_size tracks how many bytes we have > already transferred. > > This patch introduces a new variable "remaining" which represents the > amount we want to transfer (in_trans->size) minus the amount we have > already transferred (resources->xferred_dma_size). > > I have modified the check for if in_trans->size is zero to instead check > if in_trans->size is less than resources->xferred_dma_size. If we have > already transferred more bytes than in_trans->size then there are negative > bytes remaining which doesn't make sense. If there are zero bytes > remaining to be copied, just return success. > > The check in encode_dma() checked that "addr + size" could not overflow > and barring a driver bug that should work, but it's easier to check if > we do this in parts. First check that "in_trans->addr + > resources->xferred_dma_size" is safe. Then check that "xfer_start_addr + > remaining" is safe. > > My final concern was that we are dealing with u64 values but on 32bit > systems the kmalloc() function will truncate the sizes to 32 bits. So > I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);" > and returned -EINVAL if it were >= SIZE_MAX. This will not affect 64bit > systems. > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Looks good to me. Thanks a ton for all the work. Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com> Carl/Pranjal can you have a look just to make sure I haven't missed anything?
On 8/10/2023 5:23 AM, Dan Carpenter wrote: > The encode_dma() function has some validation on in_trans->size but it > would be more clear to move those checks to find_and_map_user_pages(). > > The encode_dma() had two checks: > > if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) > return -EINVAL; > > The in_trans->addr variable is the starting address. The in_trans->size > variable is the total size of the transfer. The transfer can occur in > parts and the resources->xferred_dma_size tracks how many bytes we have > already transferred. > > This patch introduces a new variable "remaining" which represents the > amount we want to transfer (in_trans->size) minus the amount we have > already transferred (resources->xferred_dma_size). > > I have modified the check for if in_trans->size is zero to instead check > if in_trans->size is less than resources->xferred_dma_size. If we have > already transferred more bytes than in_trans->size then there are negative > bytes remaining which doesn't make sense. If there are zero bytes > remaining to be copied, just return success. > > The check in encode_dma() checked that "addr + size" could not overflow > and barring a driver bug that should work, but it's easier to check if > we do this in parts. First check that "in_trans->addr + > resources->xferred_dma_size" is safe. Then check that "xfer_start_addr + > remaining" is safe. > > My final concern was that we are dealing with u64 values but on 32bit > systems the kmalloc() function will truncate the sizes to 32 bits. So > I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);" > and returned -EINVAL if it were >= SIZE_MAX. This will not affect 64bit > systems. > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Looks good to me. Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
On 8/10/2023 6:23 AM, Dan Carpenter wrote: > The encode_dma() function has some validation on in_trans->size but it > would be more clear to move those checks to find_and_map_user_pages(). > > The encode_dma() had two checks: > > if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) > return -EINVAL; > > The in_trans->addr variable is the starting address. The in_trans->size > variable is the total size of the transfer. The transfer can occur in > parts and the resources->xferred_dma_size tracks how many bytes we have > already transferred. > > This patch introduces a new variable "remaining" which represents the > amount we want to transfer (in_trans->size) minus the amount we have > already transferred (resources->xferred_dma_size). > > I have modified the check for if in_trans->size is zero to instead check > if in_trans->size is less than resources->xferred_dma_size. If we have > already transferred more bytes than in_trans->size then there are negative > bytes remaining which doesn't make sense. If there are zero bytes > remaining to be copied, just return success. > > The check in encode_dma() checked that "addr + size" could not overflow > and barring a driver bug that should work, but it's easier to check if > we do this in parts. First check that "in_trans->addr + > resources->xferred_dma_size" is safe. Then check that "xfer_start_addr + > remaining" is safe. > > My final concern was that we are dealing with u64 values but on 32bit > systems the kmalloc() function will truncate the sizes to 32 bits. So > I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);" > and returned -EINVAL if it were >= SIZE_MAX. This will not affect 64bit > systems. > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Pushed to drm-misc-fixes -Jeff
diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c index cfbc92da426f..388abd40024b 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -392,18 +392,31 @@ static int find_and_map_user_pages(struct qaic_device *qdev, struct qaic_manage_trans_dma_xfer *in_trans, struct ioctl_resources *resources, struct dma_xfer *xfer) { + u64 xfer_start_addr, remaining, end, total; unsigned long need_pages; struct page **page_list; unsigned long nr_pages; struct sg_table *sgt; - u64 xfer_start_addr; int ret; int i; - xfer_start_addr = in_trans->addr + resources->xferred_dma_size; + if (check_add_overflow(in_trans->addr, resources->xferred_dma_size, &xfer_start_addr)) + return -EINVAL; - need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) - - resources->xferred_dma_size, PAGE_SIZE); + if (in_trans->size < resources->xferred_dma_size) + return -EINVAL; + remaining = in_trans->size - resources->xferred_dma_size; + if (remaining == 0) + return 0; + + if (check_add_overflow(xfer_start_addr, remaining, &end)) + return -EINVAL; + + total = remaining + offset_in_page(xfer_start_addr); + if (total >= SIZE_MAX) + return -EINVAL; + + need_pages = DIV_ROUND_UP(total, PAGE_SIZE); nr_pages = need_pages; @@ -435,7 +448,7 @@ static int find_and_map_user_pages(struct qaic_device *qdev, ret = sg_alloc_table_from_pages(sgt, page_list, nr_pages, offset_in_page(xfer_start_addr), - in_trans->size - resources->xferred_dma_size, GFP_KERNEL); + remaining, GFP_KERNEL); if (ret) { ret = -ENOMEM; goto free_sgt; @@ -566,9 +579,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 encode_dma() function has some validation on in_trans->size but it would be more clear to move those checks to find_and_map_user_pages(). The encode_dma() had two checks: if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) return -EINVAL; The in_trans->addr variable is the starting address. The in_trans->size variable is the total size of the transfer. The transfer can occur in parts and the resources->xferred_dma_size tracks how many bytes we have already transferred. This patch introduces a new variable "remaining" which represents the amount we want to transfer (in_trans->size) minus the amount we have already transferred (resources->xferred_dma_size). I have modified the check for if in_trans->size is zero to instead check if in_trans->size is less than resources->xferred_dma_size. If we have already transferred more bytes than in_trans->size then there are negative bytes remaining which doesn't make sense. If there are zero bytes remaining to be copied, just return success. The check in encode_dma() checked that "addr + size" could not overflow and barring a driver bug that should work, but it's easier to check if we do this in parts. First check that "in_trans->addr + resources->xferred_dma_size" is safe. Then check that "xfer_start_addr + remaining" is safe. My final concern was that we are dealing with u64 values but on 32bit systems the kmalloc() function will truncate the sizes to 32 bits. So I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);" and returned -EINVAL if it were >= SIZE_MAX. This will not affect 64bit systems. Fixes: 129776ac2e38 ("accel/qaic: Add control path") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v6: it turns out this patch is mostly a clean up instead of a bug fix. The only real issue is the 32bit truncation bug, and no one is going to be using this driver on 32bit. This patch does change the behavior to return 0 for in_trans->size == 0 as discussed earlier. v5: re-write --- drivers/accel/qaic/qaic_control.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)