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 <[email protected]>
Link: https://lore.kernel.org/linux-mm/[email protected]/
Cc: Matt Porter <[email protected]>
Cc: Alexandre Bounine <[email protected]>
Cc: Jing Xiangfeng <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Souptick Joarder <[email protected]>
Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
On Thu, 9 Sep 2021 09:14:09 -0700 Kees Cook <[email protected]> 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 <[email protected]>
thanks,
--
John Hubbard
NVIDIA
On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <[email protected]> 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 01:27:52PM -0700, Andrew Morton wrote:
> On Thu, 9 Sep 2021 09:14:09 -0700 Kees Cook <[email protected]> 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
On Thu, Sep 09, 2021 at 04:11:09PM -0700, Andrew Morton wrote:
> On Thu, 9 Sep 2021 15:51:23 -0700 Kees Cook <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
