| Message ID | e6cbc8a3-c2ae-46be-a731-494470c0a21c@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] accel/qaic: tighten integer overflow checking in map_user_pages() | expand |
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 > 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 > we want to transfer (in_trans->size) minus the ammount we have already > 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@linaro.org> > --- > 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); > > The other question I had is should we add a check: > > if (remaining == 0) > return 0; > > 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. 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; 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 */ 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
On Mon, Aug 07, 2023 at 05:09:34PM +0300, Dan Carpenter wrote: > + remaining = in_trans->size - resources->xferred_dma_size; > + total = in_trans->size + offset_in_page(xfer_start_addr); > + if (total >= SIZE_MAX) Btw, I wrote it >= instead of > to silence some idiotic static analysis. On a 64bit system U64_MAX can't be greater than SIZE_MAX so Gcc will complain. However this test only affect 32bit systems and > and >= SIZE_MAX on a 32bit system are effecitively the same. Neither one is ever going to happen. regards, dan carpenter
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@linaro.org> >> --- >> 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
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)) + 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;
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 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 we want to transfer (in_trans->size) minus the ammount we have already 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@linaro.org> --- 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); The other question I had is should we add a check: if (remaining == 0) return 0; drivers/accel/qaic/qaic_control.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)