accel/qaic: Clean up integer overflow checking in map_user_pages()
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: 129776ac2e
("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/24d3348b-25ac-4c1b-b171-9dae7c43e4e0@moroto.mountain
This commit is contained in:
parent
2d956177b7
commit
96d3c1cade
|
@ -392,18 +392,31 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
|
||||||
struct qaic_manage_trans_dma_xfer *in_trans,
|
struct qaic_manage_trans_dma_xfer *in_trans,
|
||||||
struct ioctl_resources *resources, struct dma_xfer *xfer)
|
struct ioctl_resources *resources, struct dma_xfer *xfer)
|
||||||
{
|
{
|
||||||
|
u64 xfer_start_addr, remaining, end, total;
|
||||||
unsigned long need_pages;
|
unsigned long need_pages;
|
||||||
struct page **page_list;
|
struct page **page_list;
|
||||||
unsigned long nr_pages;
|
unsigned long nr_pages;
|
||||||
struct sg_table *sgt;
|
struct sg_table *sgt;
|
||||||
u64 xfer_start_addr;
|
|
||||||
int ret;
|
int ret;
|
||||||
int i;
|
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) -
|
if (in_trans->size < resources->xferred_dma_size)
|
||||||
resources->xferred_dma_size, PAGE_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;
|
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,
|
ret = sg_alloc_table_from_pages(sgt, page_list, nr_pages,
|
||||||
offset_in_page(xfer_start_addr),
|
offset_in_page(xfer_start_addr),
|
||||||
in_trans->size - resources->xferred_dma_size, GFP_KERNEL);
|
remaining, GFP_KERNEL);
|
||||||
if (ret) {
|
if (ret) {
|
||||||
ret = -ENOMEM;
|
ret = -ENOMEM;
|
||||||
goto free_sgt;
|
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)
|
QAIC_MANAGE_EXT_MSG_LENGTH)
|
||||||
return -ENOMEM;
|
return -ENOMEM;
|
||||||
|
|
||||||
if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
|
|
||||||
return -EINVAL;
|
|
||||||
|
|
||||||
xfer = kmalloc(sizeof(*xfer), GFP_KERNEL);
|
xfer = kmalloc(sizeof(*xfer), GFP_KERNEL);
|
||||||
if (!xfer)
|
if (!xfer)
|
||||||
return -ENOMEM;
|
return -ENOMEM;
|
||||||
|
|
Loading…
Reference in New Issue