* [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call @ 2021-06-15 10:45 Colin King 2021-06-15 10:47 ` Colin Ian King 0 siblings, 1 reply; 5+ messages in thread From: Colin King @ 2021-06-15 10:45 UTC (permalink / raw) To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-janitors, linux-kernel From: Colin Ian King <[email protected]> Static analysis is warning that the sizeof being used is should be of *data->tags[i] and not data->tags[i]. Although these are the same size on 64 bit systems it is not a portable assumption to assume this is true for all cases. Addresses-Coverity: ("Sizeof not portable") Fixes: d878c81610e1 ("io_uring: hide rsrc tag copy into generic helpers") Signed-off-by: Colin Ian King <[email protected]> --- fs/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index d665c9419ad3..6b1a70449749 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7231,7 +7231,7 @@ static int io_rsrc_data_alloc(struct io_ring_ctx *ctx, rsrc_put_fn *do_put, ret = -EFAULT; for (i = 0; i < nr; i++) { if (copy_from_user(io_get_tag_slot(data, i), &utags[i], - sizeof(data->tags[i]))) + sizeof(*data->tags[i]))) goto fail; } } -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call 2021-06-15 10:45 [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call Colin King @ 2021-06-15 10:47 ` Colin Ian King 2021-06-15 11:30 ` Pavel Begunkov 0 siblings, 1 reply; 5+ messages in thread From: Colin Ian King @ 2021-06-15 10:47 UTC (permalink / raw) To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-janitors, linux-kernel On 15/06/2021 11:45, Colin King wrote: > From: Colin Ian King <[email protected]> > > Static analysis is warning that the sizeof being used is should be > of *data->tags[i] and not data->tags[i]. Although these are the same > size on 64 bit systems it is not a portable assumption to assume > this is true for all cases. > > Addresses-Coverity: ("Sizeof not portable") > Fixes: d878c81610e1 ("io_uring: hide rsrc tag copy into generic helpers") > Signed-off-by: Colin Ian King <[email protected]> > --- > fs/io_uring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index d665c9419ad3..6b1a70449749 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -7231,7 +7231,7 @@ static int io_rsrc_data_alloc(struct io_ring_ctx *ctx, rsrc_put_fn *do_put, > ret = -EFAULT; > for (i = 0; i < nr; i++) { > if (copy_from_user(io_get_tag_slot(data, i), &utags[i], > - sizeof(data->tags[i]))) > + sizeof(*data->tags[i]))) > goto fail; > } > } > Actually, I think there is also an issue on line 7220 too, I'll fix that and re-send. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call 2021-06-15 10:47 ` Colin Ian King @ 2021-06-15 11:30 ` Pavel Begunkov 2021-06-15 11:35 ` Colin Ian King 0 siblings, 1 reply; 5+ messages in thread From: Pavel Begunkov @ 2021-06-15 11:30 UTC (permalink / raw) To: Colin Ian King, Jens Axboe, io-uring; +Cc: kernel-janitors, linux-kernel On 6/15/21 11:47 AM, Colin Ian King wrote: > On 15/06/2021 11:45, Colin King wrote: >> From: Colin Ian King <[email protected]> >> >> Static analysis is warning that the sizeof being used is should be >> of *data->tags[i] and not data->tags[i]. Although these are the same >> size on 64 bit systems it is not a portable assumption to assume >> this is true for all cases. >> >> Addresses-Coverity: ("Sizeof not portable") >> Fixes: d878c81610e1 ("io_uring: hide rsrc tag copy into generic helpers") >> Signed-off-by: Colin Ian King <[email protected]> >> --- >> fs/io_uring.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index d665c9419ad3..6b1a70449749 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -7231,7 +7231,7 @@ static int io_rsrc_data_alloc(struct io_ring_ctx *ctx, rsrc_put_fn *do_put, >> ret = -EFAULT; >> for (i = 0; i < nr; i++) { >> if (copy_from_user(io_get_tag_slot(data, i), &utags[i], >> - sizeof(data->tags[i]))) >> + sizeof(*data->tags[i]))) >> goto fail; >> } >> } >> Yep, thanks Colin. I think `sizeof(io_get_tag_slot(data, i))` would be less confusing. Or u64 *tag_slot = io_get_tag_slot(data, i); copy_from_user(tag_slot, ..., sizeof(*tag_slot)); -- Pavel Begunkov ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call 2021-06-15 11:30 ` Pavel Begunkov @ 2021-06-15 11:35 ` Colin Ian King 2021-06-15 12:10 ` Pavel Begunkov 0 siblings, 1 reply; 5+ messages in thread From: Colin Ian King @ 2021-06-15 11:35 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe, io-uring; +Cc: kernel-janitors, linux-kernel On 15/06/2021 12:30, Pavel Begunkov wrote: > On 6/15/21 11:47 AM, Colin Ian King wrote: >> On 15/06/2021 11:45, Colin King wrote: >>> From: Colin Ian King <[email protected]> >>> >>> Static analysis is warning that the sizeof being used is should be >>> of *data->tags[i] and not data->tags[i]. Although these are the same >>> size on 64 bit systems it is not a portable assumption to assume >>> this is true for all cases. >>> >>> Addresses-Coverity: ("Sizeof not portable") >>> Fixes: d878c81610e1 ("io_uring: hide rsrc tag copy into generic helpers") >>> Signed-off-by: Colin Ian King <[email protected]> >>> --- >>> fs/io_uring.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index d665c9419ad3..6b1a70449749 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -7231,7 +7231,7 @@ static int io_rsrc_data_alloc(struct io_ring_ctx *ctx, rsrc_put_fn *do_put, >>> ret = -EFAULT; >>> for (i = 0; i < nr; i++) { >>> if (copy_from_user(io_get_tag_slot(data, i), &utags[i], >>> - sizeof(data->tags[i]))) >>> + sizeof(*data->tags[i]))) >>> goto fail; >>> } >>> } >>> > > Yep, thanks Colin. I think `sizeof(io_get_tag_slot(data, i))` > would be less confusing. Or > > u64 *tag_slot = io_get_tag_slot(data, i); > copy_from_user(tag_slot, ..., sizeof(*tag_slot)); > BTW, Coverity is complaining about: 7220 return -ENOMEM; Wrong sizeof argument (SIZEOF_MISMATCH) suspicious_sizeof: Passing argument nr * 8UL /* sizeof (data->tags[0][0]) */ to function io_alloc_page_table and then casting the return value to u64 ** is suspicious. 7221 data->tags = (u64 **)io_alloc_page_table(nr * sizeof(data->tags[0][0])); Not sure if that's a false positive or not. This kind of indirection makes my brain melt. Colin ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call 2021-06-15 11:35 ` Colin Ian King @ 2021-06-15 12:10 ` Pavel Begunkov 0 siblings, 0 replies; 5+ messages in thread From: Pavel Begunkov @ 2021-06-15 12:10 UTC (permalink / raw) To: Colin Ian King, Jens Axboe, io-uring; +Cc: kernel-janitors, linux-kernel On 6/15/21 12:35 PM, Colin Ian King wrote: > On 15/06/2021 12:30, Pavel Begunkov wrote: >> On 6/15/21 11:47 AM, Colin Ian King wrote: >>> On 15/06/2021 11:45, Colin King wrote: >>>> From: Colin Ian King <[email protected]> >>>> >>>> Static analysis is warning that the sizeof being used is should be >>>> of *data->tags[i] and not data->tags[i]. Although these are the same >>>> size on 64 bit systems it is not a portable assumption to assume >>>> this is true for all cases. >>>> >>>> Addresses-Coverity: ("Sizeof not portable") >>>> Fixes: d878c81610e1 ("io_uring: hide rsrc tag copy into generic helpers") >>>> Signed-off-by: Colin Ian King <[email protected]> >>>> --- >>>> fs/io_uring.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index d665c9419ad3..6b1a70449749 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -7231,7 +7231,7 @@ static int io_rsrc_data_alloc(struct io_ring_ctx *ctx, rsrc_put_fn *do_put, >>>> ret = -EFAULT; >>>> for (i = 0; i < nr; i++) { >>>> if (copy_from_user(io_get_tag_slot(data, i), &utags[i], >>>> - sizeof(data->tags[i]))) >>>> + sizeof(*data->tags[i]))) >>>> goto fail; >>>> } >>>> } >>>> >> > > >> Yep, thanks Colin. I think `sizeof(io_get_tag_slot(data, i))` >> would be less confusing. Or >> >> u64 *tag_slot = io_get_tag_slot(data, i); >> copy_from_user(tag_slot, ..., sizeof(*tag_slot)); >> > BTW, Coverity is complaining about: > > 7220 return -ENOMEM; > > Wrong sizeof argument (SIZEOF_MISMATCH) > > suspicious_sizeof: Passing argument nr * 8UL /* sizeof > (data->tags[0][0]) */ to function io_alloc_page_table and then casting > the return value to u64 ** is suspicious. > > 7221 data->tags = (u64 **)io_alloc_page_table(nr * > sizeof(data->tags[0][0])); Ah, this one. We want it to be indexed linearly, but can't allocate as much, so together with io_get_tag_slot() it hides two level tables from us, providing linear indexing. > > Not sure if that's a false positive or not. This kind of indirection > makes my brain melt. So, this one should be a false positive. But agree about the indirection, it's not the first sizeof bug you found. Any better ideas how to push it to the type system? I think something like below would make more sense #define copy_from_user_typed(from, to) \ assert(typeof(from) == typeof(to)), copy_from_user(from, to, sizeof(*from)); -- Pavel Begunkov ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-15 12:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-15 10:45 [PATCH][next] io_uring: Fix incorrect sizeof operator for copy_from_user call Colin King 2021-06-15 10:47 ` Colin Ian King 2021-06-15 11:30 ` Pavel Begunkov 2021-06-15 11:35 ` Colin Ian King 2021-06-15 12:10 ` Pavel Begunkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox