netdev - Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

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: <ceec28d4-48e2-4de1-9f26-bfd3c61fde45@app.fastmail.com>
Date: Sat, 12 Aug 2023 10:23:30 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "edward.cree" <edward.cree@....com>, linux-net-drivers@....com,
 "David S . Miller" <davem@...emloft.net>, "Jakub Kicinski" <kuba@...nel.org>,
 "Eric Dumazet" <edumazet@...gle.com>, "Paolo Abeni" <pabeni@...hat.com>
Cc: "Edward Cree" <ecree.xilinx@...il.com>, Netdev <netdev@...r.kernel.org>,
 "Martin Habets" <habetsm.xilinx@...il.com>,
 "Kees Cook" <keescook@...omium.org>
Subject: Re: [PATCH v2 net-next 1/3] sfc: use padding to fix alignment in loopback test

On Fri, Jun 23, 2023, at 20:38, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.com>
>
> Add two bytes of padding to the start of struct efx_loopback_payload,
>  which are not sent on the wire.  This ensures the 'ip' member is
>  4-byte aligned, preventing the following W=1 warning:
> net/ethernet/sfc/selftest.c:46:15: error: field ip within 'struct 
> efx_loopback_payload' is less aligned than 'struct iphdr' and is 
> usually due to 'struct efx_loopback_payload' being packed, which can 
> lead to unaligned accesses [-Werror,-Wunaligned-access]
>         struct iphdr ip;
>
> Reported-by: Arnd Bergmann <arnd@...db.de>
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
> ---
>  drivers/net/ethernet/sfc/selftest.c | 47 +++++++++++++++++------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/selftest.c 
> b/drivers/net/ethernet/sfc/selftest.c
> index 3c5227afd497..96d856b9043c 100644
> --- a/drivers/net/ethernet/sfc/selftest.c
> +++ b/drivers/net/ethernet/sfc/selftest.c
> @@ -42,12 +42,16 @@
>   * Falcon only performs RSS on TCP/UDP packets.
>   */
>  struct efx_loopback_payload {
> +	char pad[2]; /* Ensures ip is 4-byte aligned */
>  	struct ethhdr header;
>  	struct iphdr ip;
>  	struct udphdr udp;
>  	__be16 iteration;
>  	char msg[64];
> -} __packed;
> +} __packed __aligned(4);

Unfortunately, the same warning now came back after commit
55c1528f9b97f ("sfc: fix field-spanning memcpy in selftest"), which
does:

 struct efx_loopback_payload {
        char pad[2]; /* Ensures ip is 4-byte aligned */
-       struct ethhdr header;
-       struct iphdr ip;
-       struct udphdr udp;
-       __be16 iteration;
-       char msg[64];
+       struct_group_attr(packet, __packed,
+               struct ethhdr header;
+               struct iphdr ip;
+               struct udphdr udp;
+               __be16 iteration;
+               char msg[64];
+       );
 } __packed __aligned(4);

I'm out of ideas for how to fix both warnings at the same time,
with the struct group we get the iphdr at an invalid offset from
the start of the inner structure, but without it the memcpy
find the field overflow.

My original patch would probably fix it, but as you pointed
out that was rather ugly.

     Arnd