linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rapidio: Avoid bogus __alloc_size warning
@ 2021-09-09 16:14 Kees Cook
  2021-09-09 20:27 ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2021-09-09 16:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-09 16:14 [PATCH] rapidio: Avoid bogus __alloc_size warning Kees Cook
@ 2021-09-09 20:27 ` Andrew Morton
  2021-09-09 22:26   ` John Hubbard
  2021-09-09 22:51   ` Kees Cook
  0 siblings, 2 replies; 9+ messages in thread
From: Andrew Morton @ 2021-09-09 20:27 UTC (permalink / raw)
  To: Kees Cook
  Cc: kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-09 20:27 ` Andrew Morton
@ 2021-09-09 22:26   ` John Hubbard
  2021-09-09 22:51   ` Kees Cook
  1 sibling, 0 replies; 9+ messages in thread
From: John Hubbard @ 2021-09-09 22:26 UTC (permalink / raw)
  To: Andrew Morton, Kees Cook
  Cc: kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-09 20:27 ` Andrew Morton
  2021-09-09 22:26   ` John Hubbard
@ 2021-09-09 22:51   ` Kees Cook
  2021-09-09 23:11     ` Andrew Morton
  1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2021-09-09 22:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-09 22:51   ` Kees Cook
@ 2021-09-09 23:11     ` Andrew Morton
  2021-09-10  1:52       ` Kees Cook
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-09-09 23:11 UTC (permalink / raw)
  To: Kees Cook
  Cc: kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-09 23:11     ` Andrew Morton
@ 2021-09-10  1:52       ` Kees Cook
  2021-09-10  4:50         ` Dan Carpenter
  0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2021-09-10  1:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, Dan Carpenter, linux-kernel,
	linux-hardening

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-10  1:52       ` Kees Cook
@ 2021-09-10  4:50         ` Dan Carpenter
  2021-09-10  5:48           ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-09-10  4:50 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, linux-kernel, linux-hardening

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-10  4:50         ` Dan Carpenter
@ 2021-09-10  5:48           ` Andrew Morton
  2021-09-10  6:29             ` Kees Cook
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-09-10  5:48 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Kees Cook, kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, linux-kernel, linux-hardening

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rapidio: Avoid bogus __alloc_size warning
  2021-09-10  5:48           ` Andrew Morton
@ 2021-09-10  6:29             ` Kees Cook
  0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2021-09-10  6:29 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Dan Carpenter, kernel test robot, Matt Porter, Alexandre Bounine,
	Jing Xiangfeng, Ira Weiny, John Hubbard, Souptick Joarder,
	Gustavo A . R . Silva, linux-kernel, linux-hardening

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-10  6:29 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-09 16:14 [PATCH] rapidio: Avoid bogus __alloc_size warning Kees Cook
2021-09-09 20:27 ` Andrew Morton
2021-09-09 22:26   ` John Hubbard
2021-09-09 22:51   ` Kees Cook
2021-09-09 23:11     ` Andrew Morton
2021-09-10  1:52       ` Kees Cook
2021-09-10  4:50         ` Dan Carpenter
2021-09-10  5:48           ` Andrew Morton
2021-09-10  6:29             ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).