[PATCH] rapidio: Avoid bogus __alloc_size warning

[PATCH] rapidio: Avoid bogus __alloc_size warning

[Kees Cook]

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(-)

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:
-- 
2.30.2

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Andrew Morton]

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.

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Andrew Morton]

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?

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Dan Carpenter]

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

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Andrew Morton]

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*".

Re: [PATCH] rapidio: Avoid bogus __alloc_size warning

[Kees Cook]

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!

-- 
Kees Cook