Message-ID: <29a2374b9a92e09e247ec9eb3f98e93aef7614c2.camel@sipsolutions.net>
Date: Wed, 14 Feb 2024 12:28:51 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Alexis Lothoré <alexis.lothore@...tlin.com>, Kalle
	Valo <kvalo@...nel.org>, kernel test robot <lkp@...el.com>
Cc: Ajay Singh <ajay.kathat@...rochip.com>, oe-kbuild-all@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org
Subject: Re: drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42:
 sparse: sparse: incorrect type in assignment (different base types)

On Wed, 2024-02-14 at 12:03 +0100, Alexis Lothoré wrote:
> On 2/14/24 10:29, Johannes Berg wrote:
> > Hi,
> > 
> > > > For reference here's the old discussion:
> > > > 
> > > > https://patchwork.kernel.org/project/linux-wireless/patch/20220720160302.231516-1-ajay.kathat@microchip.com/
> > > > 
> > > > Any volunteers to help fix this? I would prefers fixes for issues like
> > > > this compared to questionable random cleanups we always get.
> > > 
> > > I'm bumping this old thread because it looks like the sparse warning is still
> > > present in WILC driver, and I would gladly help getting rid of it, but since
> > > there's already been a fair amount of discussions around it, I am not sure what
> > > is expected/what should be done. Here is my understanding so far:
> > > - Ajay has proposed a patch ([1]) which indeed fixes the warning but involves
> > > many casts
> > > - Johannes and Jouni then gave details about the original issue leading to those
> > > casts ([2]): wpa_supplicant somehow forces the AKM suites values to be be32 at
> > > some point, while it should be treated in host endianness
> > > - as pointed by Ajay, the corresponding fix has been made since then by Jouni in
> > > wpa_supplicant ([3]). The fix make sure to handle key_mgmt_suite in host
> > > endianness AND to keep compatibility with current drivers having the be32 fix. -
> > 
> > Am I confused, or is the change [3] in the other direction?
> > 
> > From what I see, the code there (now changed again, btw) is about
> > reading the value *from the driver*.
> 
> Ah, you are right, so [3] is rather about supporting drivers sending values with
> host endianness, while interface historically expects big endian :/

Well it was kind of documented  originally as host byte order, but
(some) drivers historically did big endian ...

Anyway, same thing - but note it's only doing the big endian check for a
single value.

> > The driver change is about getting the value *from the supplicant*.
> > 
> > And the _outgoing_ code (sending it to the driver) from the supplicant
> > has - as far as I can tell - been putting it into the attribute in host
> > byte order forever? See commit cfaab58007b4 ("nl80211: Connect API
> > support").
> > 
> > 
> > Aha! So, I'm not _completely_ confused, 
> 
> So I am the one confused :) Thanks for the clarification. I did not dig enough,
> and since the cast is done right at connect message reception, I assumed wrongly
> that the issue was on the supplicant->driver path.

Yeah it's confusing ...

> > however, the only use of this value in this driver is sending it back to the supplicant!
> > Which seems entirely wrong, since the supplicant assumes basically anything
> > will be handled?
> 
> Not sure to fully understand why its wrong (supplicant seems to expect AKM
> suites to be provided) , but I may be lacking more general understanding about
> the external auth process.

Well the firmware cannot support arbitrary AKM here, I'd think? And
anyway it's not even sent to the firmware at all, so it has to be
assuming it's SAE or so?

> Or do you mean that AKM suites should be enforced to RSN_AUTH_KEY_MGMT_SAE only
> in this driver->supplicant call (and so, not forward any possible value ?)

Well it's more a question of what the firmware will actually be able to
do here, I'd think. IIUC the whole point of this is to let firmware know
what it's expected to do.

> > Also note that the supplicant will *only* read RSN_AUTH_KEY_MGMT_SAE in
> > big endian, so you've already lost here pretty much?
> 
> So we have to keep some big endian conversion on the driver/nl80211 ->
> supplicant path, IIUC

If we were to drop support for old wpa_supplicant, then no, we could
just use host byte order everywhere.

As we want to continue supporting old wpa_supplicant, which really just
checked for RSN_AUTH_KEY_MGMT_SAE in big endian (before [3], when it
started checking for both big endian and host byte order), then we have
to send *that particular value* in big endian. For every other value we
have to send it in host byte order, because either it doesn't matter
(old wpa_s) or is checked in host byte order (new wpa_s, after later
patches to this code checking for other values.)

But! That actually also means the driver is wrong now, because it sends
_any_ value back in big endian, when wpa_s - new or old - only checks
for this one value in big endian. It's OK with old since it only checks
for this value in the first place, but broken for new!

> > > - apply the same fix as for wpa_supplicant ([3]), not in wilc but in nl80211
> > > (may need to update not only wilc but any driver having trailing be32 cast on
> > > AKM suites)
> 
> So to fix my initial suggestion, it is not "doing the same fix" but rather "move
> the be conversion from the driver to nl80211 layer". Which matches in fact what
> you are suggesting below.

Well you could do the exact same thing in the driver.

But it's probably cleaner to handle the backward compat issue in nl80211
and let the driver(s) just use host byte order generally, as intended?

> > That might even work? Well, not the same fix, since again input vs.
> > output, but something like this:
> > 
> > --- a/net/wireless/nl80211.c
> > +++ b/net/wireless/nl80211.c
> > @@ -20136,9 +20136,27 @@ int cfg80211_external_auth_request(struct net_device *dev,
> >  	if (!hdr)
> >  		goto nla_put_failure;
> >  
> > +	/*
> > +	 * Some drivers and due to that userspace (wpa_supplicant) were
> > +	 * in the past interpreting this value as a big-endian value,
> > +	 * at a time where only WLAN_AKM_SUITE_SAE was used. This is now
> > +	 * fixed, but for the benefit of older wpa_supplicant versions,
> > +	 * send this particular value in big-endian. Note that newer
> > +	 * wpa_supplicant will also detect this particular value in big
> > +	 * endian still, so it all continues to work.
> > +	 */
> > +	if (params->key_mgmt_suite == WLAN_AKM_SUITE_SAE) {
> > +		if (nla_put_be32(msg, NL80211_ATTR_AKM_SUITES,
> > +				 cpu_to_be32(WLAN_AKM_SUITE_SAE))
> > +			goto nla_put_failure;
> > +	} else {
> > +		if (nla_put_u32(msg, NL80211_ATTR_AKM_SUITES,
> > +				params->key_mgmt_suite)))
> > +			goto nla_put_failure;
> > +	}
> > +
> >  	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
> >  	    nla_put_u32(msg, NL80211_ATTR_IFINDEX, dev->ifindex) ||
> > -	    nla_put_u32(msg, NL80211_ATTR_AKM_SUITES, params->key_mgmt_suite) ||
> >  	    nla_put_u32(msg, NL80211_ATTR_EXTERNAL_AUTH_ACTION,
> >  			params->action) ||
> >  	    nla_put(msg, NL80211_ATTR_BSSID, ETH_ALEN, params->bssid)
> > > > 
> 
> If this kind of fix is ok in nl80211, I can do some tests with it, see how it
> would affect other drivers ( I think there's only Quantenna driver which would
> be affected)

I'm happy with that, sure. Please do check.

Thanks!

johannes

Powered by blists - more mailing lists