[PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Alexis Green]

This patch fixes a NULL dereference in ath9k (and likely other drivers) when
fixed mesh paths are used. The problem is that when a station comes up
sta_info_alloc allocates ath_node implicitly via hw->sta_data_size. When it
does that the ath_node is zeroed out. The ath_node isn’t actually initialized
until the station becomes associated and ath9k_sta_state is called.

Normally this is OK because paths won't be formed to a station that isn't
authorized. But when the path is fixed the MAC thinks it knows how to
 route the packet, sends it down to ath9k, and winds up inducing reboots
in places like ath_tx_start when tid->ac is dereferenced.

Signed-off-by: Alexis Green <agreen@cococorp.com>
CC: Jesse Jones <jjones@cococorp.com>

---

diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
index 214e63b..14e146e 100644
--- a/net/mac80211/mesh_hwmp.c
+++ b/net/mac80211/mesh_hwmp.c
@@ -1081,6 +1081,11 @@ int mesh_nexthop_resolve(struct
ieee80211_sub_if_data *sdata,
        if (!err)
                goto endlookup;

+       if (err == -EPERM) {
+               mesh_path_discard_frame(sdata, skb);
+               goto endlookup;
+       }
+
        /* no nexthop found, start resolving */
        mpath = mesh_path_lookup(sdata, target_addr);
        if (!mpath) {
@@ -1145,6 +1150,12 @@ int mesh_nexthop_lookup(struct
ieee80211_sub_if_data *sdata,

        next_hop = rcu_dereference(mpath->next_hop);
        if (next_hop) {
+               if (mpath->flags & MESH_PATH_FIXED) {
+                       if (next_hop->sta_state != IEEE80211_STA_AUTHORIZED) {
+                               err = -EPERM;
+                               goto endlookup;
+                       }
+               }
                memcpy(hdr->addr1, next_hop->sta.addr, ETH_ALEN);
                memcpy(hdr->addr2, sdata->vif.addr, ETH_ALEN);
                ieee80211_mps_set_frame_flags(sdata, next_hop, hdr);

Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Bob Copeland]

On Thu, May 21, 2015 at 06:05:13PM -0700, Alexis Green wrote:
> This patch fixes a NULL dereference in ath9k (and likely other drivers) when
> fixed mesh paths are used. The problem is that when a station comes up
> sta_info_alloc allocates ath_node implicitly via hw->sta_data_size. When it
> does that the ath_node is zeroed out. The ath_node isn’t actually initialized
> until the station becomes associated and ath9k_sta_state is called.

Good catch.

I wonder if we should instead remove the mesh special case in
ieee80211_tx_h_check_assoc() -- given that we require an assoc station in
peering before we send data frames to that RA, and userspace should also be
setting assoc flag after MPM completes, I can't think of a reason offhand why
we'd need to bail out there.

Does this also fix the problem for you?  It passed the wpa_supplicant test
cases at least (but we aren't fixing mpaths in any of those...)

>From 246febaa51d555fda437cc8064798db06c5f4d6e Mon Sep 17 00:00:00 2001
From: Bob Copeland <me@bobcopeland.com>
Date: Mon, 25 May 2015 12:01:52 -0400
Subject: [PATCH] mac80211: enable assoc check for mesh interfaces

We already set a station to be associated when peering completes,
both in user space and in the kernel.  Thus we should always have
an associated sta before sending data frames to that station.

Failure to do this can cause crashes in the lower-level driver due
to transmitting unicast data frames before driver sta structures
(e.g. ampdu state in ath9k) are initialized.  This could have
happened if fixing mpaths, which could then allow TX to stations
with whom we haven't yet completed peering.

Reported-by: Alexis Green <agreen@cococorp.com>
Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/tx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 667111ee6a20..5787f15a3a12 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -301,9 +301,6 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 	if (tx->sdata->vif.type == NL80211_IFTYPE_WDS)
 		return TX_CONTINUE;
 
-	if (tx->sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
-		return TX_CONTINUE;
-
 	if (tx->flags & IEEE80211_TX_PS_BUFFERED)
 		return TX_CONTINUE;
 
-- 
2.1.4



-- 
Bob Copeland %% http://bobcopeland.com/

Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Johannes Berg]

On Mon, 2015-05-25 at 12:15 -0400, Bob Copeland wrote:
> On Thu, May 21, 2015 at 06:05:13PM -0700, Alexis Green wrote:
> > This patch fixes a NULL dereference in ath9k (and likely other drivers) when
> > fixed mesh paths are used. The problem is that when a station comes up
> > sta_info_alloc allocates ath_node implicitly via hw->sta_data_size. When it
> > does that the ath_node is zeroed out. The ath_node isn’t actually initialized
> > until the station becomes associated and ath9k_sta_state is called.
> 
> Good catch.
> 
> I wonder if we should instead remove the mesh special case in
> ieee80211_tx_h_check_assoc() -- given that we require an assoc station in
> peering before we send data frames to that RA, and userspace should also be
> setting assoc flag after MPM completes, I can't think of a reason offhand why
> we'd need to bail out there.
> 
> Does this also fix the problem for you?  It passed the wpa_supplicant test
> cases at least (but we aren't fixing mpaths in any of those...)
> 
> From 246febaa51d555fda437cc8064798db06c5f4d6e Mon Sep 17 00:00:00 2001
> From: Bob Copeland <me@bobcopeland.com>
> Date: Mon, 25 May 2015 12:01:52 -0400
> Subject: [PATCH] mac80211: enable assoc check for mesh interfaces
> 
> We already set a station to be associated when peering completes,
> both in user space and in the kernel.  Thus we should always have
> an associated sta before sending data frames to that station.
> 
> Failure to do this can cause crashes in the lower-level driver due
> to transmitting unicast data frames before driver sta structures
> (e.g. ampdu state in ath9k) are initialized.  This could have
> happened if fixing mpaths, which could then allow TX to stations
> with whom we haven't yet completed peering.
> 
> Reported-by: Alexis Green <agreen@cococorp.com>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>

This looks reasonable to me - Alexis?

johannes

RE: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Jesse Jones]

Sounds good to me and I will try this though unfortunately not until next
week when I get back from vacation.

Thanks,
Jesse

-----Original Message-----
From: Bob Copeland [mailto:me@bobcopeland.com]
Sent: Monday, May 25, 2015 9:16 AM
To: Alexis Green
Cc: Johannes Berg; linux-wireless; Jesse Jones
Subject: Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely
other drivers) when fixed mesh paths are used

On Thu, May 21, 2015 at 06:05:13PM -0700, Alexis Green wrote:
> This patch fixes a NULL dereference in ath9k (and likely other
> drivers) when fixed mesh paths are used. The problem is that when a
> station comes up sta_info_alloc allocates ath_node implicitly via
> hw->sta_data_size. When it does that the ath_node is zeroed out. The
> ath_node isn’t actually initialized until the station becomes associated
> and ath9k_sta_state is called.

Good catch.

I wonder if we should instead remove the mesh special case in
ieee80211_tx_h_check_assoc() -- given that we require an assoc station in
peering before we send data frames to that RA, and userspace should also be
setting assoc flag after MPM completes, I can't think of a reason offhand
why we'd need to bail out there.

Does this also fix the problem for you?  It passed the wpa_supplicant test
cases at least (but we aren't fixing mpaths in any of those...)

>From 246febaa51d555fda437cc8064798db06c5f4d6e Mon Sep 17 00:00:00 2001
From: Bob Copeland <me@bobcopeland.com>
Date: Mon, 25 May 2015 12:01:52 -0400
Subject: [PATCH] mac80211: enable assoc check for mesh interfaces

We already set a station to be associated when peering completes, both in
user space and in the kernel.  Thus we should always have an associated sta
before sending data frames to that station.

Failure to do this can cause crashes in the lower-level driver due to
transmitting unicast data frames before driver sta structures (e.g. ampdu
state in ath9k) are initialized.  This could have happened if fixing mpaths,
which could then allow TX to stations with whom we haven't yet completed
peering.

Reported-by: Alexis Green <agreen@cococorp.com>
Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/tx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index
667111ee6a20..5787f15a3a12 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -301,9 +301,6 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 	if (tx->sdata->vif.type == NL80211_IFTYPE_WDS)
 		return TX_CONTINUE;

-	if (tx->sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
-		return TX_CONTINUE;
-
 	if (tx->flags & IEEE80211_TX_PS_BUFFERED)
 		return TX_CONTINUE;

--
2.1.4



-- 
Bob Copeland %% http://bobcopeland.com/

RE: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Jesse Jones]

> -----Original Message-----
> From: Bob Copeland [mailto:me@bobcopeland.com]
> Sent: Monday, May 25, 2015 9:16 AM
> To: Alexis Green
> Cc: Johannes Berg; linux-wireless; Jesse Jones
> Subject: Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely
> other drivers) when fixed mesh paths are used
>
> On Thu, May 21, 2015 at 06:05:13PM -0700, Alexis Green wrote:
> > This patch fixes a NULL dereference in ath9k (and likely other
> > drivers) when fixed mesh paths are used. The problem is that when a
> > station comes up sta_info_alloc allocates ath_node implicitly via
> > hw->sta_data_size. When it does that the ath_node is zeroed out. The
> > ath_node isn’t actually initialized until the station becomes associated
> > and
> ath9k_sta_state is called.
>
> Good catch.
>
> I wonder if we should instead remove the mesh special case in
> ieee80211_tx_h_check_assoc() -- given that we require an assoc station in
> peering before we send data frames to that RA, and userspace should also
> be setting assoc flag after MPM completes, I can't think of a reason
> offhand
> why we'd need to bail out there.
>
> Does this also fix the problem for you?  It passed the wpa_supplicant test
> cases at least (but we aren't fixing mpaths in any of those...)
>
> From 246febaa51d555fda437cc8064798db06c5f4d6e Mon Sep 17 00:00:00
> 2001
> From: Bob Copeland <me@bobcopeland.com>
> Date: Mon, 25 May 2015 12:01:52 -0400
> Subject: [PATCH] mac80211: enable assoc check for mesh interfaces
>
> We already set a station to be associated when peering completes, both in
> user space and in the kernel.  Thus we should always have an associated
> sta
> before sending data frames to that station.
>
> Failure to do this can cause crashes in the lower-level driver due to
> transmitting unicast data frames before driver sta structures (e.g. ampdu
> state in ath9k) are initialized.  This could have happened if fixing
> mpaths,
> which could then allow TX to stations with whom we haven't yet completed
> peering.
>
> Reported-by: Alexis Green <agreen@cococorp.com>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  net/mac80211/tx.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index
> 667111ee6a20..5787f15a3a12 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -301,9 +301,6 @@ ieee80211_tx_h_check_assoc(struct
> ieee80211_tx_data *tx)
>  	if (tx->sdata->vif.type == NL80211_IFTYPE_WDS)
>  		return TX_CONTINUE;
>
> -	if (tx->sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
> -		return TX_CONTINUE;
> -
>  	if (tx->flags & IEEE80211_TX_PS_BUFFERED)
>  		return TX_CONTINUE;
>
> --
> 2.1.4
>
>
>
> --
> Bob Copeland %% http://bobcopeland.com/

Sorry for the delay in responding to this...

I had trouble reproing the reboot caused by ath9k de-referencing NULL
pointers but after a fair amount of work I was able to consistently get
reboots. Your patch *does* fix the problem so I am going to switch our repo
over to your patch instead of what we were using. Thanks for the help.

  -- Jesse

Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Johannes Berg]

On Fri, 2015-06-12 at 11:01 -0700, Jesse Jones wrote:

> I had trouble reproing the reboot caused by ath9k de-referencing NULL
> pointers but after a fair amount of work I was able to consistently get
> reboots. Your patch *does* fix the problem so I am going to switch our repo
> over to your patch instead of what we were using. Thanks for the help.

Great. Bob, if it's no bother I'd appreciate if you'd resend the patch
correctly so it gets picked up by patchwork etc.

Thanks,
johannes

Re: [PATCH] mac80211: fix a NULL dereference in ath9k (and likely other drivers) when fixed mesh paths are used

[Bob Copeland]

On Fri, Jun 12, 2015 at 10:10:32PM +0200, Johannes Berg wrote:
> On Fri, 2015-06-12 at 11:01 -0700, Jesse Jones wrote:
> 
> > I had trouble reproing the reboot caused by ath9k de-referencing NULL
> > pointers but after a fair amount of work I was able to consistently get
> > reboots. Your patch *does* fix the problem so I am going to switch our repo
> > over to your patch instead of what we were using. Thanks for the help.
> 
> Great. Bob, if it's no bother I'd appreciate if you'd resend the patch
> correctly so it gets picked up by patchwork etc.

Sure.  Jesse, want me to add your Tested-by?

-- 
Bob Copeland %% http://bobcopeland.com/