Re: [PATCH] [RFC] mwifiex: Fix NULL pointer deref

[Brian Norris <briannorris@chromium.org>]

On Fri, Jun 21, 2024 at 11:07:27AM +0200, Sascha Hauer wrote:
> I am running plain wpa_supplicant -i mlan0 with this config:
> 
> network={
>         ssid="somessid"
>         mode=2
>         frequency=2412
>         key_mgmt=WPA-PSK WPA-PSK-SHA256
>         proto=RSN
>         group=CCMP
>         pairwise=CCMP
>         psk="12345678"
> }
> 
> wait for the AP to be established, <ctrl-c> wpa_supplicant and start it
> again.

Thanks. I still can't reproduce, but that's OK. From your fuller
description now, it seems it depends on the particulars of the bss
indices in use, and maybe my device/firmware is behaving differently.
Anyway, your current description and patch below make a lot more sense.

> It doesn't seem to be a locking problem, see the patch below which fixes
> my problem.

Sure. It's probably harder to trigger real issues out of some of these
kinds of race conditions, and the logical problem below sounds a lot
more likely.

> At some point during incoming events the correct adapter->priv[]
> is selected based on bss_num and bss_type. when adapter->priv[0] is used
> for AP mode then an incoming event with type MWIFIEX_BSS_TYPE_STA leads
> to adapter->priv[1] being picked which is unused and doesn't have a
> wiphy attached to it.

Ack, that makes a lot of sense -- although I think it also implies
either you're getting some kind of corrupt event buffer from your device
too, or there's something else logically weird with your firmware when
you're receiving MWIFIEX_BSS_TYPE_STA events for a *_AP interface. (Or
maybe that's also racy, as you're changing the virtual interface from
STA to AP? Not sure.)

It also highlights that I find this fallback code from
mwifiex_process_event() weird:

        /* Get BSS number and corresponding priv */
        priv = mwifiex_get_priv_by_id(adapter, EVENT_GET_BSS_NUM(eventcause),
                                      EVENT_GET_BSS_TYPE(eventcause));
        if (!priv)
                priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
                       ^^ // if we didn't match the right BSS/event
			  // metadata, we'll deliver the event to an
			  // arbitrary interface?

But I don't think that's your problem. And at least so far, I don't
think the AP and STA event IDs collide in any way, so I don't think
we're likely to end up misbehaving even if something is misdelievered.

> 
> -------------------------8<----------------------------
> 
> From 3357963821294ff7de26259a154a1cb5bab760cb Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 18 Jun 2024 12:20:20 +0200
> Subject: [PATCH] mwifiex: Do not return unused priv in
>  mwifiex_get_priv_by_id()
> 
> mwifiex_get_priv_by_id() returns the priv pointer corresponding to the
> bss_num and bss_type, but without checking if the priv is actually
> currently in use.
> Unused priv pointers do not have a wiphy attached to them which can lead
> to NULL pointer dereferences further down the callstack.
> Fix this by returning only used priv pointers which have priv->bss_mode
> set to something else than NL80211_IFTYPE_UNSPECIFIED.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/wireless/marvell/mwifiex/main.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 175882485a195..c5164ae41b547 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1287,6 +1287,9 @@ mwifiex_get_priv_by_id(struct mwifiex_adapter *adapter,
>  
>  	for (i = 0; i < adapter->priv_num; i++) {
>  		if (adapter->priv[i]) {
> +			if (adapter->priv[i]->bss_mode == NL80211_IFTYPE_UNSPECIFIED)
> +				continue;
> +
>  			if ((adapter->priv[i]->bss_num == bss_num) &&
>  			    (adapter->priv[i]->bss_type == bss_type))
>  				break;

Acked-by: Brian Norris <briannorris@chromium.org>

Something about this formatting didn't get properly picked up by
patchwork though, so you'll need to resubmit. Feel free to carry the
above tag with it.

Brian