Re: [PATCH v7 8/9] Bluetooth: qca: Set NVMEM BD address quirks when address is invalid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NVMEM BD_ADDR retrieval does not reject the unprogrammed flash state (BDADDR_NONE / FF:FF:FF:FF:FF:FF).
- [Medium] is_configured() and get_missing_options() fail to consider HCI_QUIRK_USE_BDADDR_NVMEM when determining if a device is configured.
--

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

Bluetooth: qca: Set NVMEM BD address quirks when address is invalid
    
When the controller BD address is invalid (zero or default),
set the NVMEM quirks to allow retrieving the address from a
'local-bd-address' NVMEM cell. The BD address is often stored
alongside the WiFi MAC address in big-endian format, so also
set the big-endian quirk.

> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 04ebe290bc784..06cb787b0559f 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -723,8 +723,11 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>  	}
>  
>  	bda = (struct hci_rp_read_bd_addr *)skb->data;
> -	if (!bacmp(&bda->bdaddr, &config->bdaddr))
> +	if (!bacmp(&bda->bdaddr, &config->bdaddr)) {
>  		hci_set_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY);
> +		hci_set_quirk(hdev, HCI_QUIRK_USE_BDADDR_NVMEM);

[Severity: High]
Does this safely handle devices with an unprogrammed NVMEM cell?

If a device has an unprogrammed flash state (BDADDR_NONE or FF:FF:FF:FF:FF:FF),
of_get_nvmem_eui48() will retrieve it without rejecting it, because it only
checks for all-zeroes:

net/core/of_net.c:of_get_nvmem_eui48() {
	if (len != ETH_ALEN || !memchr_inv(eui48, 0, ETH_ALEN)) {
		kfree(eui48);
		return -EINVAL;
	}
...

Then hci_dev_get_bd_addr_from_nvmem() will assign this to hdev->public_addr
without checking against BDADDR_NONE:

net/bluetooth/hci_sync.c:hci_dev_get_bd_addr_from_nvmem() {
	err = of_get_nvmem_eui48(np, "local-bd-address", ba);
	if (err)
		return err;

	if (hci_test_quirk(hdev, HCI_QUIRK_BDADDR_NVMEM_BE))
		baswap(&hdev->public_addr, (bdaddr_t *)ba);
	else
		bacpy(&hdev->public_addr, (bdaddr_t *)ba);
...

Could this result in multiple devices on the network being initialized with a
broadcast Bluetooth address instead of being marked as unconfigured?

[Severity: Medium]
Will this quirk be properly evaluated when determining if the device is
configured?

Currently, the core logic in net/bluetooth/mgmt.c:is_configured() only checks
for HCI_QUIRK_INVALID_BDADDR and HCI_QUIRK_USE_BDADDR_PROPERTY:

	if ((hci_test_quirk(hdev, HCI_QUIRK_INVALID_BDADDR) ||
	     hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY)) &&
	    !bacmp(&hdev->public_addr, BDADDR_ANY))
		return false;

Similarly, net/bluetooth/mgmt.c:get_missing_options() omits
HCI_QUIRK_USE_BDADDR_NVMEM:

	if ((hci_test_quirk(hdev, HCI_QUIRK_INVALID_BDADDR) ||
	     hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY)) &&
	    !bacmp(&hdev->public_addr, BDADDR_ANY))
		options |= MGMT_OPTION_PUBLIC_ADDRESS;

While the driver sets both HCI_QUIRK_USE_BDADDR_PROPERTY and
HCI_QUIRK_USE_BDADDR_NVMEM here, does the core logic need an update to handle
HCI_QUIRK_USE_BDADDR_NVMEM independently?

> +		hci_set_quirk(hdev, HCI_QUIRK_BDADDR_NVMEM_BE);
> +	}
>  
>  	kfree_skb(skb);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-block-as-nvmem-v7-0-3fe8205ef0a8@oss.qualcomm.com?part=8