| Message ID | 20220104072658.69756-11-marcan@marcan.st |
|---|---|
| State | Accepted |
| Commit | 687f767d6fab448e4e80011cf5230cd021135bfb |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
On 04/01/2022 23.23, Andy Shevchenko wrote: > 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). I usually check for trivial flags before calling more expensive functions because it's more efficient in the common case. Obviously here performance doesn't matter though. > > .... > >> +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. What do you mean by implicit format string? The format string is at the top of the file and its length is right next to it, which makes it harder for them to accidentally fall out of sync. +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3) > >> + nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1; > > Also, with temporary variable the code can be better to read > > size_t mac_len = ...; > Sure.
On Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@marcan.st> wrote: > On 04/01/2022 23.23, Andy Shevchenko wrote: > > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: ... > >> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > >> + 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. > > What do you mean by implicit format string? When I read the above code I feel uncomfortable because no-one can see (without additional action and more reading and checking) if it's correct or not. This is potential to be error prone. > The format string is at the > top of the file and its length is right next to it, which makes it > harder for them to accidentally fall out of sync. It is not an argument. Just you may do the same in the code directly and more explicitly: Also you don't check the return code of snprintf which means that you don't care about the result, which seems to me wrong approach. If you don't care about the result, so it means it's not very important, right? > +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3)
On 2022/01/06 23:20, Andy Shevchenko wrote: > On Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@marcan.st> wrote: >> On 04/01/2022 23.23, Andy Shevchenko wrote: >>> On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: > > ... > >>>> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > >>>> + 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. >> >> What do you mean by implicit format string? > > When I read the above code I feel uncomfortable because no-one can see > (without additional action and more reading and checking) if it's > correct or not. This is potential to be error prone. > >> The format string is at the >> top of the file and its length is right next to it, which makes it >> harder for them to accidentally fall out of sync. > > It is not an argument. Just you may do the same in the code directly > and more explicitly: The point is that BRCMF_FW_MACADDR_LEN and BRCMF_FW_MACADDR_FMT need to be in sync, and BRCMF_FW_MACADDR_LEN is used in two different places. If I inline the format string into the code, someone could change it without changing the length, or changing the length inline only next to the format string. Then we overflow the NVRAM buffer because the allocation is not sized properly. By having them as defines, it is obvious that they go together, and if one changes the other one has to change too, and the nvram allocation can't end up improperly sized as long as they are kept in sync. > Also you don't check the return code of snprintf which means that you > don't care about the result, which seems to me wrong approach. If you > don't care about the result, so it means it's not very important, > right? > That snprintf can never fail as long as the format string/length are in sync. I'll make it BUG_ON(... != size), so it complains loudly if someone screws up the format string in the future.
On 09/01/2022 05.14, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin 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). > > What overrides what. Can you elaborate a bit? The behavior seems to be: - Use the NVRAM MAC address, if any - Use the SROM MAC address, if any - Fail to boot So a platform with a module containing a MAC address may choose to override it using the DT mechanism with this patch. This is consistent with the behavior of other drivers implementing platform MAC support.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c index 054ea3ed133e..e3f7ac0f5671 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c @@ -21,6 +21,8 @@ #define BRCMF_FW_NVRAM_DEVPATH_LEN 19 /* devpath0=pcie/1/4/ */ #define BRCMF_FW_NVRAM_PCIEDEV_LEN 10 /* pcie/1/4/ + \0 */ #define BRCMF_FW_DEFAULT_BOARDREV "boardrev=0xff" +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3) enum nvram_parser_state { IDLE, @@ -57,6 +59,7 @@ struct nvram_parser { bool multi_dev_v1; bool multi_dev_v2; bool boardrev_found; + bool strip_mac; }; /* @@ -121,6 +124,10 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp) nvp->multi_dev_v2 = true; 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) + st = COMMENT; } else if (!is_nvram_char(c) || c == ' ') { brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n", nvp->line, nvp->column); @@ -207,6 +214,8 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp, size = BRCMF_FW_MAX_NVRAM_SIZE; else size = data_len; + /* Add space for properties we may add */ + size += BRCMF_FW_MACADDR_LEN + 1; /* Alloc for extra 0 byte + roundup by 4 + length field */ size += 1 + 3 + sizeof(u32); nvp->nvram = kzalloc(size, GFP_KERNEL); @@ -366,22 +375,34 @@ static void brcmf_fw_add_defaults(struct nvram_parser *nvp) nvp->nvram_len++; } +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); + nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1; +} + /* brcmf_nvram_strip :Takes a buffer of "<var>=<value>\n" lines read from a fil * and ending in a NUL. Removes carriage returns, empty lines, comment lines, * and converts newlines to NULs. Shortens buffer as needed and pads with NULs. * End of buffer is completed with token identifying length of buffer. */ static void *brcmf_fw_nvram_strip(const u8 *data, size_t data_len, - u32 *new_length, u16 domain_nr, u16 bus_nr) + u32 *new_length, u16 domain_nr, u16 bus_nr, + struct device *dev) { struct nvram_parser nvp; u32 pad; u32 token; __le32 token_le; + u8 mac[ETH_ALEN]; if (brcmf_init_nvram_parser(&nvp, data, data_len) < 0) return NULL; + if (eth_platform_get_mac_address(dev, mac) == 0) + nvp.strip_mac = true; + while (nvp.pos < data_len) { nvp.state = nv_parser_states[nvp.state](&nvp); if (nvp.state == END) @@ -402,6 +423,9 @@ static void *brcmf_fw_nvram_strip(const u8 *data, size_t data_len, brcmf_fw_add_defaults(&nvp); + if (nvp.strip_mac) + brcmf_fw_add_macaddr(&nvp, mac); + pad = nvp.nvram_len; *new_length = roundup(nvp.nvram_len + 1, 4); while (pad != *new_length) { @@ -546,7 +570,8 @@ static int brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx) if (data) nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length, fwctx->req->domain_nr, - fwctx->req->bus_nr); + fwctx->req->bus_nr, + fwctx->dev); if (free_bcm47xx_nvram) bcm47xx_nvram_release_contents(data);