linux-kernel - Re: [PATCH v2 02/12] wifi: mwifiex: fix MAC address handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZzS7TKwPC1uxqzbg@pengutronix.de>
Date: Wed, 13 Nov 2024 15:44:28 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Brian Norris <briannorris@...omium.org>
Cc: Francesco Dolcini <francesco@...cini.it>, Kalle Valo <kvalo@...nel.org>,
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	David Lin <yu-hao.lin@....com>, kernel@...gutronix.de,
	stable@...r.kernel.org
Subject: Re: [PATCH v2 02/12] wifi: mwifiex: fix MAC address handling

Hi Brian,

It's been a while, but I'd like to get this forward now.

On Fri, Oct 04, 2024 at 04:15:32PM -0700, Brian Norris wrote:
> I think I'm generally supportive of the direction this changes things,
> but I'm a bit hesitant about two things:
> 1. the potential user-visible changes and
> 2. the linux-stable backport (Cc stable below)
> 
> For 1: MAC addresses are important in some contexts, and this might
> significantly change the addresses that devices get in practice. Such
> users might not really care about the weird details of when the address
> incremented; but they *probably* care that a certain sequence of "boot
> device; run hostapd with <foo> config file" produces the same address.
> 
> Also, I'm not sure I know enough of the implications of potential
> over-use of the locally administered bit. Are there significant
> downsides to it (aside from the simple fact that it's a different
> address)?

Not that I know of, but that doesn't mean much.

> 
> And I see that you rightly don't know how many addresses are actually
> reserved, but I have an educated guess that it's not just 1.

Even if there are more addresses reserved, we don't know which these
are, see below.

> For one,
> this driver used to default-create 3 interfaces:
> 1211c961170c mwifiex: do not create AP and P2P interfaces upon driver loading
> 
> and when we stopped doing that, we still kept support for a module
> parameter for the old way:
> 0013c7cebed6 mwifiex: module load parameter for interface creation
> 
> Perhaps these "initial" interfaces should at least be allowed permanent
> addresses?

I started up a board with the downstream driver. It comes up with these
MAC addresses:

wlp1s0    Link encap:Ethernet  HWaddr 34:6F:24:4E:E0:3D
uap0      Link encap:Ethernet  HWaddr 36:6F:24:4E:E1:3D
wfd0      Link encap:Ethernet  HWaddr 36:6F:24:4E:E0:3D

The permanent address from EEPROM is 34:6F:24:4E:E0:3D which is
used for wlp1s0. For the other addresses the locally admistered bit is
set (34 -> 36). Here's the corresponding code:

	if (priv->bss_type == MLAN_BSS_TYPE_WIFIDIRECT) {
		if (priv->bss_virtual) {
			...
		} else {
			priv->current_addr[0] |= 0x02;
		}
	}

	if (priv->bss_type != MLAN_BSS_TYPE_WIFIDIRECT) {
		if (priv->bss_index) {
			priv->current_addr[0] |= 0x02;
			priv->current_addr[4] += priv->bss_index;
		}
	}

See https://github.com/nxp-imx/mwifiex/blob/lf-6.6.3_1.0.0/mxm_wifiex/wlan_src/mlinux/moal_main.c#L8383

Note this behaviour was changed in the driver in a0835444f1
("mxm_wifiex: update to mxm5x17344.p1 release"). Before that the driver
has just done a priv->current_addr[4] += priv->bss_index without
setting the locally admistered bit. Of course the commit message
says nothing about the reasons for this change.

The downstream driver puts the bss_num (or bss_index) into different
bits than the upstream driver does. It uses current_addr[4] whereas the
upstream driver uses current_addr[5]. So even when there's more than
one MAC address reserved for one chip, both drivers disagree on which
addresses these are.

Given that, I think our safest bet is to always set the locally
admistered bit for derived MAC addresses.

> 
> So anyway, I don't really know for sure the right answer, but I want to
> log my concerns, in case you had more thoughts on backward
> compatibility.
> 
> And given all the uncertainty above, I'm extra hesitant about
> backporting likely-user-visible changes to stable (#2).

I can remove the stable tag if makes you feel more comfortable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |