Re: [PATCH RFC 4/5] wifi: ath12k: Support MAC address from the firmware node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ath12k: invalid MAC address bypass via ignored device_get_mac_address() return
- [Medium] wifi: ath12k: DT MAC override ignored on multi-radio wiphy
- [Low] ath12k: unaligned memory access risk with mac_from_dt array
--

commit 8b8a36f00c150187c70e8a8b42d0d7b0eecc142a
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

wifi: ath12k: Support MAC address from the firmware node

Read a MAC address from the device's firmware node (a DT "mac-address"/
"local-mac-address" property, or an nvmem cell named "mac-address" via
device_get_mac_address()), and use it in preference to the address from
the firmware board data. This mirrors what ath10k and ath11k already do
and lets the WLAN MAC be sourced from firmware-provisioned storage such
as a UEFI variable exposed through nvmem.

> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 01f8c6572f657..f4d2d0377c8ef 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -14878,15 +14878,27 @@ static int ath12k_mac_hw_register(struct ath12k_hw *ah)
>  	u32 ht_cap = U32_MAX, antennas_rx = 0, antennas_tx = 0;
>  	bool is_6ghz = false, is_raw_mode = false, is_monitor_disable = false;
>  	u8 *mac_addr = NULL;
> +	u8 mac_from_dt[ETH_ALEN] = {};

[Severity: Low]
Does this code risk an unaligned memory access trap?

The mac_from_dt array is declared as u8 without __aligned(2), meaning it
could be placed at an odd address on the stack.

When mac_from_dt is later passed to is_zero_ether_addr() and
ether_addr_copy(), both cast the pointer to u16* (and u32* depending on arch
config), which can trigger alignment traps or performance penalties on
architectures with strict alignment rules.

>  	u8 mbssid_max_interfaces = 0;
>  
>  	wiphy->max_ap_assoc_sta = 0;
>  
> +	/*
> +	 * Allow a MAC address provided through the device tree (directly or
> +	 * via an nvmem cell named "mac-address") to override the one from the
> +	 * firmware board data.
> +	 */
> +	device_get_mac_address(ab->dev, mac_from_dt);

[Severity: Medium]
Could this bypass input validation if device_get_mac_address() fails?

If fwnode_get_mac_address() internal to device_get_mac_address() reads an
invalid MAC address (e.g. multicast), is_valid_ether_addr() inside
fwnode_get_mac_addr() will fail and return -EINVAL.

However, the invalid non-zero bytes are left in mac_from_dt. Because the
error return from device_get_mac_address() is ignored here, the subsequent
check against is_zero_ether_addr(mac_from_dt) will succeed, causing the
invalid address to be applied.

> +
>  	for_each_ar(ah, ar, i) {
>  		u32 ht_cap_info = 0;
>  
>  		pdev = ar->pdev;
> -		if (ar->ab->pdevs_macaddr_valid) {
> +		if (!is_zero_ether_addr(mac_from_dt)) {
> +			/* A device tree / nvmem address takes precedence. */
> +			ether_addr_copy(ar->mac_addr, mac_from_dt);

[Severity: Medium]
Is the device tree override ignored for multi-radio devices?

Later in this same for_each_ar() loop, the permanent MAC address is selected:

	if (i == 0)
		mac_addr = ar->mac_addr;
	else
		mac_addr = ab->mac_addr;

If the device has multiple radios (i > 0), mac_addr defaults to ab->mac_addr.
Because ab->mac_addr is never updated with mac_from_dt,
SET_IEEE80211_PERM_ADDR and ath12k_mac_setup_mac_address_list() will silently
fall back to the un-overridden firmware MAC address for the entire wiphy.

> +			ar->mac_addr[4] += ar->pdev_idx;
> +		} else if (ar->ab->pdevs_macaddr_valid) {
>  			ether_addr_copy(ar->mac_addr, pdev->mac_addr);
>  		} else {
>  			ether_addr_copy(ar->mac_addr, ar->ab->mac_addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-b4-nvmem-efivar-v1-0-187c85f65018@oss.qualcomm.com?part=4