diff mbox series

[v2,10/35] brcmfmac: firmware: Allow platform to override macaddr

Message ID 20220104072658.69756-11-marcan@marcan.st
State Accepted
Commit 687f767d6fab448e4e80011cf5230cd021135bfb
Headers show
Series brcmfmac: Support Apple T2 and M1 platforms | expand

Commit Message

Hector Martin Jan. 4, 2022, 7:26 a.m. UTC
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).

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 29 +++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Hector Martin Jan. 5, 2022, 1:26 p.m. UTC | #1
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.
Andy Shevchenko Jan. 6, 2022, 2:20 p.m. UTC | #2
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)
Hector Martin Jan. 7, 2022, 2:39 a.m. UTC | #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.
Hector Martin Jan. 17, 2022, 6:38 a.m. UTC | #4
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 mbox series

Patch

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);