linux-kernel - Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a1x0dU=x=mnBC8JeDG=dsQNfyO7X=16jm0WUwQ8wwLp=w@mail.gmail.com>
Date:   Tue, 2 Nov 2021 09:47:28 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Taras Chornyi <tchornyi@...vell.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Volodymyr Mytnyk <vmytnyk@...vell.com>,
        Yevhen Orlov <yevhen.orlov@...ision.eu>,
        Vadym Kochan <vkochan@...vell.com>,
        Arnd Bergmann <arnd@...db.de>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [-next] net: marvell: prestera: Add explicit padding

On Tue, Nov 2, 2021 at 9:24 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> On m68k:
>
>     In function ‘prestera_hw_build_tests’,
>         inlined from ‘prestera_hw_switch_init’ at drivers/net/ethernet/marvell/prestera/prestera_hw.c:788:2:
>     ././include/linux/compiler_types.h:335:38: error: call to ‘__compiletime_assert_345’ declared with attribute error: BUILD_BUG_ON failed: sizeof(struct prestera_msg_switch_attr_req) != 16
>     ...
>
> The driver assumes structure members are naturally aligned, but does not
> add explicit padding, thus breaking architectures where integral values
> are not always naturally aligned (e.g. on m68k, __alignof(int) is 2, not
> 4).
>
> Fixes: bb5dbf2cc64d5cfa ("net: marvell: prestera: add firmware v4.0 support")
> Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>

Looks good to me,

Reviewed-by: Arnd Bergmann <arnd@...db.de>

> Compile-tested only.
>
> BTW, I sincerely doubt the use of __packed on structs like:
>
>     union prestera_msg_switch_param {
>             u8 mac[ETH_ALEN];
>             __le32 ageing_timeout_ms;
>     } __packed;
>
> This struct is only used as a member in another struct, where it is
> be naturally aligned anyway.

Agreed, this __packed attribute is clearly bogus and should be removed.

Same for

+struct prestera_msg_event_port_param {
+       union {
+               struct {
+                       u8 oper;
+                       __le32 mode;
+                       __le32 speed;
+                       u8 duplex;
+                       u8 fc;
+                       u8 fec;
+               } __packed mac;
+               struct {
+                       u8 mdix;
+                       __le64 lmode_bmap;
+                       u8 fc;
+               } __packed phy;
+       } __packed;
+} __packed __aligned(4);

This makes no sense at all. I would suggest marking only
the individual fields that are misaligned as __packed, but
not the structure itself.

and then there is this

+       union {
+               struct {
+                       u8 admin:1;
+                       u8 fc;
+                       u8 ap_enable;
+                       union {
+                               struct {
+                                       __le32 mode;
+                                       u8  inband:1;
+                                       __le32 speed;
+                                       u8  duplex;
+                                       u8  fec;
+                                       u8  fec_supp;
+                               } __packed reg_mode;
+                               struct {
+                                       __le32 mode;
+                                       __le32 speed;
+                                       u8  fec;
+                                       u8  fec_supp;
+                               } __packed ap_modes[PRESTERA_AP_PORT_MAX];
+                       } __packed;
+               } __packed mac;
+               struct {
+                       u8 admin:1;
+                       u8 adv_enable;
+                       __le64 modes;
+                       __le32 mode;
+                       u8 mdix;
+               } __packed phy;
+       } __packed link;

which puts misaligned bit fields in the middle of a packed structure!

       Arnd