Re: [PATCH net 1/2] net: dsa: mxl862xx: avoid unaligned 16-bit access in api_wrap

[David Laight <david.laight.linux@gmail.com>]

On Fri, 19 Jun 2026 04:39:25 +0100
Daniel Golle <daniel@makrotopia.org> wrote:

> The MXL862XX_API_* macros pass the address of a stack-allocated, __packed
> firmware-ABI struct to mxl862xx_api_wrap() as a void *. The struct has an
> alignment of 1, so the compiler is free to place it at an odd address.
> 
> mxl862xx_api_wrap() reinterprets that buffer as a __le16 * and accesses it
> with data[i], for which the compiler assumes the natural 2-byte alignment
> of __le16 and emits aligned 16-bit loads/stores (e.g. lhu/sh on MIPS).
> When the buffer lands on an odd address these fault on architectures that
> do not support unaligned access, such as MIPS32.

Isn't the correct fix to not pack the structure?
(or probably any of the associated structures??)

	David

> 
> -Waddress-of-packed-member does not catch this: the packed origin is
> laundered through the void * parameter, so the cast inside api_wrap looks
> alignment-safe to the compiler and no warning is emitted.
> 
> Use get_unaligned_le16()/put_unaligned_le16() for the three 16-bit word
> accesses. The byte accesses (*(u8 *)&data[i], crc16()) are already safe
> and are left unchanged.
> 
> Link: https://sashiko.dev/#/patchset/cover.1781319534.git.daniel%40makrotopia.org?part=4
> Fixes: 23794bec1cb6 ("net: dsa: add basic initial driver for MxL862xx switches")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/dsa/mxl862xx/mxl862xx-host.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> index d55f9dff6433..882c5d960941 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> @@ -12,6 +12,7 @@
>  #include <linux/crc16.h>
>  #include <linux/iopoll.h>
>  #include <linux/limits.h>
> +#include <linux/unaligned.h>
>  #include <net/dsa.h>
>  #include "mxl862xx.h"
>  #include "mxl862xx-host.h"
> @@ -349,7 +350,7 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
>  	 * zero words individually.
>  	 */
>  	for (i = 0, zeros = 0; i < size / 2 && zeros < RST_DATA_THRESHOLD; i++)
> -		if (!data[i])
> +		if (!get_unaligned_le16(&data[i]))
>  			zeros++;
>  
>  	if (zeros < RST_DATA_THRESHOLD && (size & 1) && !*(u8 *)&data[i])
> @@ -395,7 +396,7 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
>  			 */
>  			val = *(u8 *)&data[i] | ((crc & 0xff) << 8);
>  		} else {
> -			val = le16_to_cpu(data[i]);
> +			val = get_unaligned_le16(&data[i]);
>  		}
>  
>  		/* After RST_DATA, skip zero data words as the registers
> @@ -453,7 +454,7 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
>  			*(uint8_t *)&data[i] = ret & 0xff;
>  			crc = (ret >> 8) & 0xff;
>  		} else {
> -			data[i] = cpu_to_le16((u16)ret);
> +			put_unaligned_le16((u16)ret, &data[i]);
>  		}
>  	}
>