Recently the kernel test robot has reported an ARM-specific BUILD_BUG_ON()
in an old and unmaintained wil6210 wireless driver. The problem comes from
the structure packing rules of old ARM ABI ('-mabi=apcs-gnu'). For example,
the following structure is packed to 18 bytes instead of 16:

struct poorly_packed {
        unsigned int a;
        unsigned int b;
        unsigned short c;
        union {
                struct {
                        unsigned short d;
                        unsigned int e;
                } __attribute__((packed));
                struct {
                        unsigned short d;
                        unsigned int e;
                } __attribute__((packed)) inner;
        };
} __attribute__((packed));

To fit it into 16 bytes, it's required to add packed attribute to the
container union as well:

struct poorly_packed {
        unsigned int a;
        unsigned int b;
        unsigned short c;
        union {
                struct {
                        unsigned short d;
                        unsigned int e;
                } __attribute__((packed));
                struct {
                        unsigned short d;
                        unsigned int e;
                } __attribute__((packed)) inner;
        } __attribute__((packed));
} __attribute__((packed));

Thanks to Andrew Pinski of GCC team for sorting the things out at
https://gcc.gnu.org/pipermail/gcc/2023-November/242888.html.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311150821.cI4yciFE-lkp@intel.com
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 include/uapi/linux/stddef.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
index 5c6c4269f7ef..2ec6f35cda32 100644
--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -27,7 +27,7 @@
 	union { \
 		struct { MEMBERS } ATTRS; \
 		struct TAG { MEMBERS } ATTRS NAME; \
-	}
+	} ATTRS
 
 #ifdef __cplusplus
 /* sizeof(struct{}) is 1 in C++, not 0, can't use C version of the macro. */
-- 
2.42.0

On Mon, 20 Nov 2023 14:05:08 +0300, Dmitry Antipov wrote:
> Recently the kernel test robot has reported an ARM-specific BUILD_BUG_ON()
> in an old and unmaintained wil6210 wireless driver. The problem comes from
> the structure packing rules of old ARM ABI ('-mabi=apcs-gnu'). For example,
> the following structure is packed to 18 bytes instead of 16:
> 
> struct poorly_packed {
>         unsigned int a;
>         unsigned int b;
>         unsigned short c;
>         union {
>                 struct {
>                         unsigned short d;
>                         unsigned int e;
>                 } __attribute__((packed));
>                 struct {
>                         unsigned short d;
>                         unsigned int e;
>                 } __attribute__((packed)) inner;
>         };
> } __attribute__((packed));
> 
> [...]

Applied to for-linus/hardening, thanks!

[1/1] uapi: propagate __struct_group() attributes to the container union
      https://git.kernel.org/kees/c/4e86f32a13af

Take care,

-- 
Kees Cook

On Mon, 2023-11-20 at 14:05 +0300, Dmitry Antipov wrote:
> Recently the kernel test robot has reported an ARM-specific BUILD_BUG_ON()
> in an old and unmaintained wil6210 wireless driver.
> 

Heh. I guess it wasn't unmaintained enough for someone to touch it and
add struct_group() to it...

>  include/uapi/linux/stddef.h | 2 +-
> 

No idea what tree this should go through, but I guess wireless isn't
appropriate.

johannes

On Fri, Nov 24, 2023 at 06:37:14PM +0100, Johannes Berg wrote:
> On Mon, 2023-11-20 at 14:05 +0300, Dmitry Antipov wrote:
> > Recently the kernel test robot has reported an ARM-specific BUILD_BUG_ON()
> > in an old and unmaintained wil6210 wireless driver.
> > 
> 
> Heh. I guess it wasn't unmaintained enough for someone to touch it and
> add struct_group() to it...

It needed it since it was using memcpy across fields. But it's been a
while since that change happened, so finding this bug now is pretty
impressive. :)

> 
> >  include/uapi/linux/stddef.h | 2 +-
> > 
> 
> No idea what tree this should go through, but I guess wireless isn't
> appropriate.

I'll take it; thanks for the find Dmitry!

-- 
Kees Cook