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

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

[Daniel Golle]

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.

-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]);
 		}
 	}
 
-- 
2.54.0

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

[David Laight]

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]);
>  		}
>  	}
>  

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

[Jakub Kicinski]

On Fri, 19 Jun 2026 10:01:54 +0100 David Laight 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??)

Agreed, this is very silly:

struct mxl862xx_register_mod {
	__le16 addr;
	__le16 data;
	__le16 mask;
} __packed;

But some structs won't get aligned:

struct mxl862xx_mac_table_clear {
	u8 type;
	u8 port_id;
} __packed;

So I guess the "just don't pack" will have some corner cases, too.

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

[David Laight]

On Wed, 24 Jun 2026 17:52:39 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 19 Jun 2026 10:01:54 +0100 David Laight 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??)  
> 
> Agreed, this is very silly:
> 
> struct mxl862xx_register_mod {
> 	__le16 addr;
> 	__le16 data;
> 	__le16 mask;
> } __packed;
> 
> But some structs won't get aligned:
> 
> struct mxl862xx_mac_table_clear {
> 	u8 type;
> 	u8 port_id;
> } __packed;

Does that one need an aligned(2) ?

> So I guess the "just don't pack" will have some corner cases, too.

The main problem is the original 32bit arm abi which 32bit aligns
all structures.
But that is pretty much dead and would want a packed_if_arm_oabi
define.
Unlikely to be relevant for this code.

	David

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 19 Jun 2026 04:39:25 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: dsa: mxl862xx: avoid unaligned 16-bit access in api_wrap
    https://git.kernel.org/netdev/net/c/6b3f7af57881
  - [net,2/2] net: dsa: mxl862xx: fix use-after-free of DSA ports in crc_err_work
    https://git.kernel.org/netdev/net/c/bcb3b8314611

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html