Hello Ajay, Kalle,
On 8/29/23 10:31, Kalle Valo wrote:
> Kalle Valo <[email protected]> writes:
>
>> kernel test robot <[email protected]> writes:
[...]
>>> sparse warnings: (new ones prefixed by >>)
>>>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@
>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: expected unsigned int key_mgmt_suite
>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: got restricted __be32 [usertype]
>>
>> Yeah, this is an old issue but we really should try to fix this,
>> especially so as I would like to make wireless code sparse warning free
>> in the near future. IIRC there were some problems with nl80211 interface
>> as well so this might not be simple fix still.
>
> For reference here's the old discussion:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> 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. -
- It could have allowed to simply get rid of the all casts on AKM suites in
wilc driver ([4]), but then new kernel/drivers would break with existing
userspace, so it is not an option
Now, I see multiple options to fix the sparse warning:
- apply the same fix as for wpa_supplicant ([3]) in wilc driver (so basically,
become compatible with both endianness)
- 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)
- take the initial quick but not-so-nice double cast fix and call it a day
What's your opinion on this ?
[1]
https://lore.kernel.org/linux-wireless/[email protected]/
[2]
https://lore.kernel.org/linux-wireless/[email protected]/
[3] https://w1.fi/cgit/hostap/commit/?id=8392ea9e75eacf30cb09671e463d9a37c3eadd6a
[4]
https://lore.kernel.org/linux-wireless/[email protected]/
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
> > For reference here's the old discussion:
> >
> > https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
> >
> > 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*.
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, 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?
(But - the firmware also has a parameter key_mgmt_suites [in struct
wilc_external_auth_param] which is never even set in
wilc_set_external_auth_param??)
Also note that the supplicant will *only* read RSN_AUTH_KEY_MGMT_SAE in
big endian, so you've already lost here pretty much?
> - It could have allowed to simply get rid of the all casts on AKM suites in
> wilc driver ([4]), but then new kernel/drivers would break with existing
> userspace, so it is not an option
I am wondering if it works at all ...
> Now, I see multiple options to fix the sparse warning:
> - apply the same fix as for wpa_supplicant ([3]) in wilc driver (so basically,
> become compatible with both endianness)
But this cannot be done! On input to the driver, the value is in host
byte order always. The question is on output - and there you cannot
detect it.
> - 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)
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)
||
> - take the initial quick but not-so-nice double cast fix and call it a day
but that doesn't actually work for anything other than
WLAN_AKM_SUITE_SAE...
johannes
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/[email protected]/
>>>
>>> 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 :/
>
> 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.
> 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.
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 ?)
> (But - the firmware also has a parameter key_mgmt_suites [in struct
> wilc_external_auth_param] which is never even set in
> wilc_set_external_auth_param??)
>
>
> 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
>> - It could have allowed to simply get rid of the all casts on AKM suites in
>> wilc driver ([4]), but then new kernel/drivers would break with existing
>> userspace, so it is not an option
>
> I am wondering if it works at all ...
>
>> Now, I see multiple options to fix the sparse warning:
>> - apply the same fix as for wpa_supplicant ([3]) in wilc driver (so basically,
>> become compatible with both endianness)
>
> But this cannot be done! On input to the driver, the value is in host
> byte order always. The question is on output - and there you cannot
> detect it.
Yeah, this suggestion is wrong because of the misunderstanding clarified above.
>> - 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.
> 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)
Thanks,
Alexis
>> - take the initial quick but not-so-nice double cast fix and call it a day
>
> but that doesn't actually work for anything other than
> WLAN_AKM_SUITE_SAE...
>
> johannes
>
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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/[email protected]/
> > > >
> > > > 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
Hi
On 2/14/24 02:29, Johannes Berg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi,
>
>>> For reference here's the old discussion:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>>>
>>> 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*.
>
> 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, 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?
>
> (But - the firmware also has a parameter key_mgmt_suites [in struct
> wilc_external_auth_param] which is never even set in
> wilc_set_external_auth_param??)
>
>
yeah, *key_mgmt_suites* is not passed to the firmware. It is added to
*wilc_external_auth_param* but it was not necessary since SAE AUTH is
offloaded to user space so it takes care of complete AUTH exchange and
only confirm once it is done. key_mgmt_suites, which is received in
connect(), is needs to be maintained in driver to pass back using
cfg80211_external_auth_request().
> Also note that the supplicant will *only* read RSN_AUTH_KEY_MGMT_SAE in
> big endian, so you've already lost here pretty much?
>
>> - It could have allowed to simply get rid of the all casts on AKM suites in
>> wilc driver ([4]), but then new kernel/drivers would break with existing
>> userspace, so it is not an option
>
> I am wondering if it works at all ...
> >> Now, I see multiple options to fix the sparse warning:
>> - apply the same fix as for wpa_supplicant ([3]) in wilc driver (so basically,
>> become compatible with both endianness)
>
> But this cannot be done! On input to the driver, the value is in host
> byte order always. The question is on output - and there you cannot
> detect it.
>
>> - 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)
>
> 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)
> ||
IMO this patch is better solution since it covers for both old and new
wpa_s(3) and even the fixes in new wpa_s are not required with this
change. After this change, the driver can be modified to use the host
byte order that will also resolve the sparse warning.
Regards,
Ajay