Build error in crypto/marvell/cesa/cipher.c

Build error in crypto/marvell/cesa/cipher.c

[Linus Torvalds]

So I don't see anything that has changed, and I suspect the only
change is that my compiler version changed, but my arm64 build fails
right now with FORTIFY_STRING enabled.

On arm64 I now get this warning:

  In function 'fortify_memcpy_chk',
    inlined from 'mv_cesa_des3_ede_setkey' and
drivers/crypto/marvell/cesa/cipher.c:307:2:
  ./include/linux/fortify-string.h:583:25: error: call to
'__write_overflow_field' declared with attribute warning: detected
write beyond size of field (1st parameter); maybe use struct_group()?
[-Werror=attribute-warning[

but I haven't been compiling regularly enough to know when this
warning suddenly started showing up.

I enabled the cesa driver on x86-64 (by also letting it build with
COMPILE_TEST), and I do *not* see this warning on x86-64, which makes
me think it's the compiler version that matters here.

On my arm64 setup, I have gcc-13.1.1, while my x86-64 build is still 12.3.1.

But I think the warning is correct.  The problem is that the 'ctx'
pointer is wrongly typed, and it's using "struct mv_cesa_des_ctx"
(which has a "key[]" size of DES_KEY_SIZE).

And it *should* use "struct mv_cesa_des3_ctx" which has otherwise the
same layout, but the right key size (DES3_EDE_KEY_SIZE).

Fixing that type fixes the warning.

I'm actually surprised that gcc-12 doesn't seem to warn about this.
Kees? This looks like a rather obvious overflow, which makes me think
I'm missing something.

I get a similar error in 'irdma_clr_wqes()' at
drivers/infiniband/hw/irdma/uk.c:103 (and same thing on line 105). I
don't see what the right solution there is, but it looks like we have

        IRDMA_CQP_WQE_SIZE = 8
        __le64 elem[IRDMA_CQP_WQE_SIZE];

and it's doing a 4kB memset to that element. The mistake is not as
obvious as in the cesa driver.

Kees, any idea why I'm seeing it now? Is it the new
-fstrict-flex-arrays=3? And if so, why? None of this is about flex
arrays...

Anyway, please forward me fixes so that I can have a working arm64
test build again....

               Linus

Re: Build error in crypto/marvell/cesa/cipher.c

[Kees Cook]

On Wed, Jun 28, 2023 at 08:13:25PM -0700, Linus Torvalds wrote:
> So I don't see anything that has changed, and I suspect the only
> change is that my compiler version changed, but my arm64 build fails
> right now with FORTIFY_STRING enabled.
> 
> On arm64 I now get this warning:
> 
>   In function 'fortify_memcpy_chk',
>     inlined from 'mv_cesa_des3_ede_setkey' and
> drivers/crypto/marvell/cesa/cipher.c:307:2:
>   ./include/linux/fortify-string.h:583:25: error: call to
> '__write_overflow_field' declared with attribute warning: detected
> write beyond size of field (1st parameter); maybe use struct_group()?
> [-Werror=attribute-warning[

This was fixed very recently here:
https://lore.kernel.org/all/20230523083313.899332-1-arnd@kernel.org/
and Herbert took it.

I assume the crypto tree hasn't been merged yet?

> Kees, any idea why I'm seeing it now? Is it the new
> -fstrict-flex-arrays=3? And if so, why? None of this is about flex
> arrays...

The unexpected bit is that without -fstrict-flex-arrays=3 (i.e. the
default since the dawn of time), the compiler treats any array that
happens to be the last struct member as a flexible array. So with it
enabled, FORTIFY_SOURCE gains coverage over things it should have been
examining before.

-- 
Kees Cook

Re: Build error in crypto/marvell/cesa/cipher.c

[Kees Cook]

On Wed, Jun 28, 2023 at 08:13:25PM -0700, Linus Torvalds wrote:
> I get a similar error in 'irdma_clr_wqes()' at
> drivers/infiniband/hw/irdma/uk.c:103 (and same thing on line 105). I
> don't see what the right solution there is, but it looks like we have
> 
>         IRDMA_CQP_WQE_SIZE = 8
>         __le64 elem[IRDMA_CQP_WQE_SIZE];
> 
> and it's doing a 4kB memset to that element. The mistake is not as
> obvious as in the cesa driver.

I pressed "send" too fast. :)

This should also already be fixed:
https://lore.kernel.org/all/20230523111859.2197825-1-arnd@kernel.org/

-- 
Kees Cook

Re: Build error in crypto/marvell/cesa/cipher.c

[Herbert Xu]

On Wed, Jun 28, 2023 at 08:48:19PM -0700, Kees Cook wrote:
>
> This was fixed very recently here:
> https://lore.kernel.org/all/20230523083313.899332-1-arnd@kernel.org/
> and Herbert took it.
> 
> I assume the crypto tree hasn't been merged yet?

Yes it's in cryptodev:

https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=efbc7764c4446566edb76ca05e903b5905673d2e

Now that this seems to be causing build failures, I'll add it to the
crypto tree.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Build error in crypto/marvell/cesa/cipher.c

[Linus Torvalds]

On Wed, 28 Jun 2023 at 20:48, Kees Cook <keescook@chromium.org> wrote:
>
> The unexpected bit is that without -fstrict-flex-arrays=3 (i.e. the
> default since the dawn of time), the compiler treats any array that
> happens to be the last struct member as a flexible array.

Oh. Ok, that explains why it's showing up for me now, at least. It's
an odd rule, but I can see why people would have done that.

I've only seen the zero- and one-sized arrays commonly used for the
traditional "fake flex array", but I guess other sizes can easily
happen.

                 Linus

Re: Build error in crypto/marvell/cesa/cipher.c

[Leon Romanovsky]

On Wed, Jun 28, 2023 at 08:53:26PM -0700, Kees Cook wrote:
> On Wed, Jun 28, 2023 at 08:13:25PM -0700, Linus Torvalds wrote:
> > I get a similar error in 'irdma_clr_wqes()' at
> > drivers/infiniband/hw/irdma/uk.c:103 (and same thing on line 105). I
> > don't see what the right solution there is, but it looks like we have
> > 
> >         IRDMA_CQP_WQE_SIZE = 8
> >         __le64 elem[IRDMA_CQP_WQE_SIZE];
> > 
> > and it's doing a 4kB memset to that element. The mistake is not as
> > obvious as in the cesa driver.
> 
> I pressed "send" too fast. :)
> 
> This should also already be fixed:
> https://lore.kernel.org/all/20230523111859.2197825-1-arnd@kernel.org/

The fix is in latest RDMA PR:
https://lore.kernel.org/linux-rdma/ZJzUeFT7lLqEjMJn@nvidia.com/T/#u

Arnd Bergmann (1):
      RDMA/irdma: avoid fortify-string warning in irdma_clr_wqes


Thanks

> 
> -- 
> Kees Cook

Re: Build error in crypto/marvell/cesa/cipher.c

[Jason Gunthorpe]

On Wed, Jun 28, 2023 at 08:13:25PM -0700, Linus Torvalds wrote:

> I get a similar error in 'irdma_clr_wqes()' at
> drivers/infiniband/hw/irdma/uk.c:103 (and same thing on line 105). I
> don't see what the right solution there is, but it looks like we have
> 
>         IRDMA_CQP_WQE_SIZE = 8
>         __le64 elem[IRDMA_CQP_WQE_SIZE];
> 
> and it's doing a 4kB memset to that element. The mistake is not as
> obvious as in the cesa driver.

I think this fix is in the RDMA PR i just sent you:

commit b002760f877c0d91ecd3c78565b52f4bbac379dd
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue May 23 13:18:45 2023 +0200

    RDMA/irdma: avoid fortify-string warning in irdma_clr_wqes
    
    Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") triggers a
    warning for fortified memset():
    
    In function 'fortify_memset_chk',
        inlined from 'irdma_clr_wqes' at drivers/infiniband/hw/irdma/uk.c:103:4:
    include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write b>
      493 |                         __write_overflow_field(p_size_field, size);
          |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    
    The problem here isthat the inner array only has four 8-byte elements, so
    clearing 4096 bytes overflows that. As this structure is part of an outer
    array, change the code to pass a pointer to the irdma_qp_quanta instead,
    and change the size argument for readability, matching the comment above
    it.
    
    Fixes: 551c46edc769 ("RDMA/irdma: Add user/kernel shared libraries")
    Link: https://lore.kernel.org/r/20230523111859.2197825-1-arnd@kernel.org
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Acked-by: Shiraz Saleem <shiraz.saleem@intel.com>
    Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason