Re: [PATCH v3 5/7] net: of_net: Add of_get_nvmem_eui48() helper for EUI-48 lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unaligned memory access regression due to unaligned stack array passed to ether_addr_copy() and is_valid_ether_addr().
--

commit d14ab417c7b180ecf43c9c668c8189080233fb01
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

net: of_net: Add of_get_nvmem_eui48() helper for EUI-48 lookup

This commit factors out the NVMEM EUI-48 retrieval logic into a generic
helper function, allowing other subsystems to reuse it. It updates
of_get_mac_address_nvmem() to use this new helper.

> diff --git a/net/core/of_net.c b/net/core/of_net.c
> index 93ea425b9248a..75341c186123e 100644
> --- a/net/core/of_net.c
> +++ b/net/core/of_net.c
> @@ -61,9 +61,7 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
>  int of_get_mac_address_nvmem(struct device_node *np, u8 *addr)
>  {
>  	struct platform_device *pdev = of_find_device_by_node(np);
> -	struct nvmem_cell *cell;
> -	const void *mac;
> -	size_t len;
> +	u8 mac[ETH_ALEN];
>  	int ret;
>  
>  	/* Try lookup by device first, there might be a nvmem_cell_lookup
[ ... ]
> -	cell = of_nvmem_cell_get(np, "mac-address");
> -	if (IS_ERR(cell))
> -		return PTR_ERR(cell);
> -
> -	mac = nvmem_cell_read(cell, &len);
> -	nvmem_cell_put(cell);
> -
> -	if (IS_ERR(mac))
> -		return PTR_ERR(mac);
> -
> -	if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
> -		kfree(mac);
> -		return -EINVAL;
> -	}
> -
> -	memcpy(addr, mac, ETH_ALEN);
> -	kfree(mac);
> +	ret = of_get_nvmem_eui48(np, "mac-address", mac);
> +	if (ret)
> +		return ret;
> +
> +	if (!is_valid_ether_addr(mac))
> +		return -EINVAL;
> +
> +	ether_addr_copy(addr, mac);
> +	return 0;
>  }

[Severity: High]
Do ether_addr_copy() and is_valid_ether_addr() require their arguments to be
16-bit aligned?

The stack array u8 mac[ETH_ALEN] is naturally 1-byte aligned. On architectures
without CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, ether_addr_copy() and
is_valid_ether_addr() cast the pointers to u16* to perform 16-bit accesses:

include/linux/etherdevice.h:ether_addr_copy() {
...
#else
	u16 *a = (u16 *)dst;
	const u16 *b = (const u16 *)src;
...

Could this unaligned array trigger alignment faults on strict-alignment
architectures?

The previous implementation used kzalloc() for the buffer (which guarantees
alignment) and memcpy(), avoiding this issue. Switching to ether_addr_copy()
also imposes the same 16-bit alignment requirement on the caller-provided
addr buffer, which wasn't previously required.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-block-as-nvmem-v3-0-82681f50aa35@oss.qualcomm.com?part=5