Message ID | 20210909161409.2250920-1-keescook@chromium.org (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | rapidio: Avoid bogus __alloc_size warning | expand |
On Thu, 9 Sep 2021 09:14:09 -0700 Kees Cook <keescook@chromium.org> wrote: > GCC 9.3 (but not later) incorrectly evaluates the arguments to > check_copy_size(), getting seemingly confused by the size being returned > from array_size(). Instead, perform the calculation once, which both > makes the code more readable and avoids the bug in GCC. > > In file included from arch/x86/include/asm/preempt.h:7, > from include/linux/preempt.h:78, > from include/linux/spinlock.h:55, > from include/linux/mm_types.h:9, > from include/linux/buildid.h:5, > from include/linux/module.h:14, > from drivers/rapidio/devices/rio_mport_cdev.c:13: > In function 'check_copy_size', > inlined from 'copy_from_user' at include/linux/uaccess.h:191:6, > inlined from 'rio_mport_transfer_ioctl' at drivers/rapidio/devices/rio_mport_cdev.c:983:6: > include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small > 213 | __bad_copy_to(); > | ^~~~~~~~~~~~~~~ > > But the allocation size and the copy size are identical: > > transfer = vmalloc(array_size(sizeof(*transfer), transaction.count)); > if (!transfer) > return -ENOMEM; > > if (unlikely(copy_from_user(transfer, > (void __user *)(uintptr_t)transaction.block, > array_size(sizeof(*transfer), transaction.count)))) { That's an "error", not a warning. Or is this thanks to the new -Werror? Either way, I'm inclined to cc:stable on this, because use of gcc-9 on older kernels will be a common thing down the ages. If it's really an "error" on non-Werror kernels then definitely cc:stable.
On 9/9/21 13:27, Andrew Morton wrote: ... >> include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small >> 213 | __bad_copy_to(); >> | ^~~~~~~~~~~~~~~ >> >> But the allocation size and the copy size are identical: >> >> transfer = vmalloc(array_size(sizeof(*transfer), transaction.count)); >> if (!transfer) >> return -ENOMEM; >> >> if (unlikely(copy_from_user(transfer, >> (void __user *)(uintptr_t)transaction.block, >> array_size(sizeof(*transfer), transaction.count)))) { > > That's an "error", not a warning. Or is this thanks to the new -Werror? > > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > older kernels will be a common thing down the ages. > > If it's really an "error" on non-Werror kernels then definitely cc:stable. > It looks like a hard error, not a warning upgraded by -Werror: I did a local repro, then ran with V=1, removed all -Werror parts of the gcc invocation, ran again, and still reproduced the error. I also verified that the patch causes the error to go away. Also, I can't find anything wrong with the diffs, so: Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On Thu, Sep 09, 2021 at 01:27:52PM -0700, Andrew Morton wrote: > On Thu, 9 Sep 2021 09:14:09 -0700 Kees Cook <keescook@chromium.org> wrote: > > > GCC 9.3 (but not later) incorrectly evaluates the arguments to > > check_copy_size(), getting seemingly confused by the size being returned > > from array_size(). Instead, perform the calculation once, which both > > makes the code more readable and avoids the bug in GCC. > > > > In file included from arch/x86/include/asm/preempt.h:7, > > from include/linux/preempt.h:78, > > from include/linux/spinlock.h:55, > > from include/linux/mm_types.h:9, > > from include/linux/buildid.h:5, > > from include/linux/module.h:14, > > from drivers/rapidio/devices/rio_mport_cdev.c:13: > > In function 'check_copy_size', > > inlined from 'copy_from_user' at include/linux/uaccess.h:191:6, > > inlined from 'rio_mport_transfer_ioctl' at drivers/rapidio/devices/rio_mport_cdev.c:983:6: > > include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small > > 213 | __bad_copy_to(); > > | ^~~~~~~~~~~~~~~ > > > > But the allocation size and the copy size are identical: > > > > transfer = vmalloc(array_size(sizeof(*transfer), transaction.count)); > > if (!transfer) > > return -ENOMEM; > > > > if (unlikely(copy_from_user(transfer, > > (void __user *)(uintptr_t)transaction.block, > > array_size(sizeof(*transfer), transaction.count)))) { > > That's an "error", not a warning. Or is this thanks to the new -Werror? This is a "regular" error (__bad_copy_to() uses __compiletime_error()). > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > older kernels will be a common thing down the ages. > > If it's really an "error" on non-Werror kernels then definitely cc:stable. I would expect that as only being needed if __alloc_size was backported to -stable, which seems unlikely.
On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <keescook@chromium.org> wrote: > > That's an "error", not a warning. Or is this thanks to the new -Werror? > > This is a "regular" error (__bad_copy_to() uses __compiletime_error()). > > > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > > older kernels will be a common thing down the ages. > > > > If it's really an "error" on non-Werror kernels then definitely cc:stable. > > I would expect that as only being needed if __alloc_size was backported > to -stable, which seems unlikely. Ah. Changelog didn't tell me that it's an __alloc_size thing. What's the status of the __alloc_size() patchset, btw?
On Thu, Sep 09, 2021 at 04:11:09PM -0700, Andrew Morton wrote: > On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <keescook@chromium.org> wrote: > > > > That's an "error", not a warning. Or is this thanks to the new -Werror? > > > > This is a "regular" error (__bad_copy_to() uses __compiletime_error()). > > > > > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > > > older kernels will be a common thing down the ages. > > > > > > If it's really an "error" on non-Werror kernels then definitely cc:stable. > > > > I would expect that as only being needed if __alloc_size was backported > > to -stable, which seems unlikely. > > Ah. Changelog didn't tell me that it's an __alloc_size thing. Er, it's in the Subject, but I guess I could repeat it? > What's the status of the __alloc_size() patchset, btw? It's in -next via -mm, and all is well as far as I know: compiler-attributes-add-__alloc_size-for-better-bounds-checking.patch compiler-attributes-add-__alloc_size-for-better-bounds-checking-fix.patch checkpatch-add-__alloc_size-to-known-attribute.patch slab-clean-up-function-declarations.patch slab-add-__alloc_size-attributes-for-better-bounds-checking.patch mm-page_alloc-add-__alloc_size-attributes-for-better-bounds-checking.patch percpu-add-__alloc_size-attributes-for-better-bounds-checking.patch mm-vmalloc-add-__alloc_size-attributes-for-better-bounds-checking.patch FWIW, I had extensively checked (and fixed) warnings from it before sending it your way. This patch is fixing an error that just appeared from randconfig. -Kees
On Thu, Sep 09, 2021 at 06:52:27PM -0700, Kees Cook wrote: > On Thu, Sep 09, 2021 at 04:11:09PM -0700, Andrew Morton wrote: > > On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <keescook@chromium.org> wrote: > > > > > > That's an "error", not a warning. Or is this thanks to the new -Werror? > > > > > > This is a "regular" error (__bad_copy_to() uses __compiletime_error()). > > > > > > > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > > > > older kernels will be a common thing down the ages. > > > > > > > > If it's really an "error" on non-Werror kernels then definitely cc:stable. > > > > > > I would expect that as only being needed if __alloc_size was backported > > > to -stable, which seems unlikely. > > > > Ah. Changelog didn't tell me that it's an __alloc_size thing. > > Er, it's in the Subject, but I guess I could repeat it? > This is how the email looks like to Andrew. https://sylpheed.sraoss.jp/images/sylpheed2-mainwindow.png Try to find the subject in that nonsense. Same for everyone else on email as well. https://marc.info/?l=linux-kernel&m=163120404328790&w=2 I only either read the subject or the body of the commit message and never both. :P regards, dan carpenter
On Fri, 10 Sep 2021 07:50:10 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Sep 09, 2021 at 06:52:27PM -0700, Kees Cook wrote: > > On Thu, Sep 09, 2021 at 04:11:09PM -0700, Andrew Morton wrote: > > > On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <keescook@chromium.org> wrote: > > > > > > > > That's an "error", not a warning. Or is this thanks to the new -Werror? > > > > > > > > This is a "regular" error (__bad_copy_to() uses __compiletime_error()). > > > > > > > > > Either way, I'm inclined to cc:stable on this, because use of gcc-9 on > > > > > older kernels will be a common thing down the ages. > > > > > > > > > > If it's really an "error" on non-Werror kernels then definitely cc:stable. > > > > > > > > I would expect that as only being needed if __alloc_size was backported > > > > to -stable, which seems unlikely. > > > > > > Ah. Changelog didn't tell me that it's an __alloc_size thing. > > > > Er, it's in the Subject, but I guess I could repeat it? > > > > This is how the email looks like to Andrew. > > https://sylpheed.sraoss.jp/images/sylpheed2-mainwindow.png > > Try to find the subject in that nonsense. Same for everyone else on > email as well. > > https://marc.info/?l=linux-kernel&m=163120404328790&w=2 > > I only either read the subject or the body of the commit message and > never both. :P I read the body if the subject looks relevant ;) But that subject reads to me as "rapidio: Avoid bogus *blah* warning". We have soooooo many alloc_foo functions that one's eyes glaze over something like "alloc_size" Why? Because the identifier "__alloc_size" is of great significance to Kees because he wrote the thing. Everyone else just sees "*blah*".
On Thu, Sep 09, 2021 at 10:48:14PM -0700, Andrew Morton wrote: > On Fri, 10 Sep 2021 07:50:10 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > > This is how the email looks like to Andrew. > > > > https://sylpheed.sraoss.jp/images/sylpheed2-mainwindow.png > > > > Try to find the subject in that nonsense. Same for everyone else on > > email as well. > > > > https://marc.info/?l=linux-kernel&m=163120404328790&w=2 > > > > I only either read the subject or the body of the commit message and > > never both. :P > > I read the body if the subject looks relevant ;) > > But that subject reads to me as "rapidio: Avoid bogus *blah* warning". > We have soooooo many alloc_foo functions that one's eyes glaze over > something like "alloc_size" > > Why? Because the identifier "__alloc_size" is of great significance > to Kees because he wrote the thing. Everyone else just sees "*blah*". Heh. Okay, fair enough. I will make Subject/body independent. It felt redundant to me before, but greater verbosity is a good idea. Sorry!
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c index 94331d999d27..7df466e22282 100644 --- a/drivers/rapidio/devices/rio_mport_cdev.c +++ b/drivers/rapidio/devices/rio_mport_cdev.c @@ -965,6 +965,7 @@ static int rio_mport_transfer_ioctl(struct file *filp, void __user *arg) struct rio_transfer_io *transfer; enum dma_data_direction dir; int i, ret = 0; + size_t size; if (unlikely(copy_from_user(&transaction, arg, sizeof(transaction)))) return -EFAULT; @@ -976,13 +977,14 @@ static int rio_mport_transfer_ioctl(struct file *filp, void __user *arg) priv->md->properties.transfer_mode) == 0) return -ENODEV; - transfer = vmalloc(array_size(sizeof(*transfer), transaction.count)); + size = array_size(sizeof(*transfer), transaction.count); + transfer = vmalloc(size); if (!transfer) return -ENOMEM; if (unlikely(copy_from_user(transfer, (void __user *)(uintptr_t)transaction.block, - array_size(sizeof(*transfer), transaction.count)))) { + size))) { ret = -EFAULT; goto out_free; } @@ -994,8 +996,7 @@ static int rio_mport_transfer_ioctl(struct file *filp, void __user *arg) transaction.sync, dir, &transfer[i]); if (unlikely(copy_to_user((void __user *)(uintptr_t)transaction.block, - transfer, - array_size(sizeof(*transfer), transaction.count)))) + transfer, size))) ret = -EFAULT; out_free:
GCC 9.3 (but not later) incorrectly evaluates the arguments to check_copy_size(), getting seemingly confused by the size being returned from array_size(). Instead, perform the calculation once, which both makes the code more readable and avoids the bug in GCC. In file included from arch/x86/include/asm/preempt.h:7, from include/linux/preempt.h:78, from include/linux/spinlock.h:55, from include/linux/mm_types.h:9, from include/linux/buildid.h:5, from include/linux/module.h:14, from drivers/rapidio/devices/rio_mport_cdev.c:13: In function 'check_copy_size', inlined from 'copy_from_user' at include/linux/uaccess.h:191:6, inlined from 'rio_mport_transfer_ioctl' at drivers/rapidio/devices/rio_mport_cdev.c:983:6: include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small 213 | __bad_copy_to(); | ^~~~~~~~~~~~~~~ But the allocation size and the copy size are identical: transfer = vmalloc(array_size(sizeof(*transfer), transaction.count)); if (!transfer) return -ENOMEM; if (unlikely(copy_from_user(transfer, (void __user *)(uintptr_t)transaction.block, array_size(sizeof(*transfer), transaction.count)))) { Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/linux-mm/202109091134.FHnRmRxu-lkp@intel.com/ Cc: Matt Porter <mporter@kernel.crashing.org> Cc: Alexandre Bounine <alex.bou9@gmail.com> Cc: Jing Xiangfeng <jingxiangfeng@huawei.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Souptick Joarder <jrdr.linux@gmail.com> Cc: Gustavo A. R. Silva <gustavoars@kernel.org> Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/rapidio/devices/rio_mport_cdev.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)