| From | Andy Shevchenko <> | Date | Tue, 4 Jan 2022 16:23:41 +0200 | Subject | Re: [PATCH v2 10/35] brcmfmac: firmware: Allow platform to override macaddr |
| |
On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: > > On Device Tree platforms, it is customary to be able to set the MAC > address via the Device Tree, as it is often stored in system firmware. > This is particularly relevant for Apple ARM64 platforms, where this > information comes from system configuration and passed through by the > bootloader into the DT. > > Implement support for this by fetching the platform MAC address and > adding or replacing the macaddr= property in nvram. This becomes the > dongle's default MAC address. > > On platforms with an SROM MAC address, this overrides it. On platforms > without one, such as Apple ARM64 devices, this is required for the > firmware to boot (it will fail if it does not have a valid MAC at all).
...
> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3)
...
> if (strncmp(&nvp->data[nvp->entry], "boardrev", 8) == 0) > nvp->boardrev_found = true; > + /* strip macaddr if platform MAC overrides */ > + if (nvp->strip_mac && > + strncmp(&nvp->data[nvp->entry], "macaddr", 7) == 0)
If it has no side effects, I would rather swap the operands of && so you match string first (it will be in align with above code at least, although I haven't checked bigger context).
....
> +static void brcmf_fw_add_macaddr(struct nvram_parser *nvp, u8 *mac) > +{ > + snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1, > + BRCMF_FW_MACADDR_FMT, mac);
Please, avoid using implict format string, it's dangerous from security p.o.v.
> + nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1;
Also, with temporary variable the code can be better to read
size_t mac_len = ...;
> +}
-- With Best Regards, Andy Shevchenko
|