Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b02c8912-216a-cef1-e87c-1eb8ac14dc64@redhat.com>
Date:   Fri, 8 Jul 2022 20:56:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a
 random MAC
Content-Language: en-US
To:     Arend Van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Kalle Valo <kvalo@kernel.org>
Cc:     linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        SHA-cyfmac-dev-list@infineon.com
References: <20220708133712.102179-1-hdegoede@redhat.com>
 <20220708133712.102179-2-hdegoede@redhat.com>
 <9bf3dc09-73df-8914-aa13-68ed9d48f350@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <9bf3dc09-73df-8914-aa13-68ed9d48f350@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 7/8/22 20:27, Arend Van Spriel wrote:
> On 7/8/2022 3:37 PM, Hans de Goede wrote:
>> On some boards there is no eeprom to hold the nvram, in this case instead
>> a board specific nvram is loaded from /lib/firmware. On most boards the
>> macaddr=... setting in the /lib/firmware nvram file is ignored because
>> the wifi/bt chip has a unique MAC programmed into the chip itself.
>>
>> But in some cases the actual MAC from the /lib/firmware nvram file gets
>> used, leading to MAC conflicts.
>>
>> The MAC addresses in the troublesome nvram files seem to all come from
>> the same nvram file template, so we can detect this by checking for
>> the template nvram file MAC.
>>
>> Detect that the default MAC address is being used and replace it
>> with a random MAC address to avoid MAC address conflicts.
>>
>> Note that udev will detect this is a random MAC based on
>> /sys/class/net/wlan0/addr_assign_type and then replace this with
>> a MAC based on hashing the netdev-name + the machine-id. So that
>> the MAC address is both guaranteed to be unique per machine while
>> it is still the same/persistent at each boot (assuming the
>> default Link.MACAddressPolicy=persistent udev setting).
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   .../broadcom/brcm80211/brcmfmac/common.c      | 23 +++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> index dccd8f4ca1d0..7485e784be2a 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> 
> [...]
> 
>> @@ -226,6 +240,15 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
>>               bphy_err(drvr, "Retrieving cur_etheraddr failed, %d\n", err);
>>               goto done;
>>           }
>> +
>> +        if (ether_addr_equal_unaligned(ifp->mac_addr, brcmf_default_mac_address)) {
> 
> Is it necessary to use the unaligned variant?

The type is: u8 mac_addr[ETH_ALEN] so there is no guarantee it will
be aligned. Even if it is aligned today, if someone inserts
a struct member in front of it it will end up unaligned.

Regards,

Hans