On 8/7/2023 8:43 AM, Dan Carpenter wrote:
On Mon, Aug 07, 2023 at 05:09:34PM +0300, Dan Carpenter wrote:
The encode_dma() function has some validation on in_trans->size but it's
not complete and 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;
It's not sufficeint to just check if in_trans->size is zero. The
sufficient
resources->xferred_dma_size variable represent the number of bytes
already transferred. If we have already transferred more bytes than
in_trans->size then there are negative bytes remaining which doesn't
make sense. Check for that as well.
I introduced a new variable "remaining" which represents the amount
introduce (commit text should be present tense per my understanding)
we want to transfer (in_trans->size) minus the ammount we have already
amount
transferred (resources->xferred_dma_size).
The check in encode_dma() checked that "addr + size" could not overflow
however we may already have transferred some bytes so the real starting
address is "xfer_start_addr" so check that "xfer_start_addr + size"
cannot overflow instead. Also check that "addr +
resources->xferred_dma_size cannot overflow.
My other 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@xxxxxxxxxx>
---
This is re-write re-write of the previous version.
I am not necessarily sure it is correct. Please review carefully. In
particular, please check how "total" is calculated. Maybe it would make
more sense to write that as:
total = remaining + offset_in_page(xfer_start_addr);
I think this makes more sense.
The other question I had is should we add a check:
if (remaining == 0)
return 0;
I don't see why adding this would hurt anything. I don't believe it is
necessary. Remaining is a function of of the driver code and not an
external input. The driver transfers as much as it can, and stops when
everything is sent (remaining == 0). If we hit this check, it is a
driver bug.
drivers/accel/qaic/qaic_control.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index cfbc92da426f..d64505bcf4ae 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -392,18 +392,28 @@ 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;
+
+ if (in_trans->size == 0 ||
+ in_trans->size < resources->xferred_dma_size ||
+ check_add_overflow(xfer_start_addr, in_trans->size, &end))
^^^^^^^^^^^^^^
This should be remaining. So maybe it should be something like this
with a return 0 for no bytes remaining and total calculated differently.
Yep, should be remaining. Your below proposed version looks good to me.
if (check_add_overflow(in_trans->addr, resources->xferred_dma_size, &xfer_start_addr))
return -EINVAL;
if (in_trans->size < resources->xferred_dma_size)
return -EINVAL;
This check should never hit unless we have a driver bug. I don't object
to having it though.
remaining = in_trans->size - resources->xferred_dma_size;
if (remaining == 0)
return 0;
if (check_add_overflow(xfer_start_addr, remaining, &end))
return -EINVAL;
/* Still not really sure why total is calculated this way */
Physical memory layout.
Lets say remaining is 4k, and PAGE_SIZE is 4k.
4k / 4k = 1 so we need 1 page.
Except, if that 4k remaining is some remainder of the transfer and not
the complete transfer, then where we start the transfer matters.
If the remaining 4k starts right at a page boundary, then we just need a
single page. However, if the remaining 4k starts X bytes into a page
(where X is non-zero), we would actually be transferring data from two
physical pages, even though 4k can be fully represented by one page.
This representation might make a bit more sense (although I argue it is
wrong) -
total = remaining;
need_pages = DIV_ROUND_UP(total, PAGE_SIZE);
if (offset_in_page(xfer_state_addr))
need_pages++;
Where this breaks down is when the start addr and the remaining combine
to fit into a page.
Assume remaining == 5k and PAGE_SIZE == 4k. offset_in_page() is going
to return 1k.
The right answer is 2 pages. The first page will contain 3k (4k - 1k
from the offset) and the second page will contain 2k.
DIV_ROUND_UP(remaining, PAGE_SIZE) == 5k / 4k == 2
DIV_ROUND_UP(remaining + offset_in_page(), PAGE_SIZE) == (5k + 1k) / 4k == 2
Seems like we need a comment explaining this.
total = remaining + offset_in_page(xfer_start_addr);
if (total >= SIZE_MAX)
return -EINVAL;
need_pages = DIV_ROUND_UP(total, PAGE_SIZE);
regards,
dan carpenter
+ return -EINVAL;
- need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) -
- resources->xferred_dma_size, PAGE_SIZE);
+ remaining = in_trans->size - resources->xferred_dma_size;
+ total = in_trans->size + offset_in_page(xfer_start_addr);
+ if (total >= SIZE_MAX)
+ return -EINVAL;
+
+ need_pages = DIV_ROUND_UP(total - resources->xferred_dma_size, PAGE_SIZE);
nr_pages = need_pages;
@@ -435,7 +445,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 +576,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;
--
2.39.2
