[PATCH] mac80211: prevent out of bounds access in ieee80211_rx_h_action()

[PATCH] mac80211: prevent out of bounds access in ieee80211_rx_h_action()

[Dan Carpenter]

Smatch complains that status->band comes from the skb->data so it
cannot be trusted.  It's a u8 so it can be in the 0-255 range but the
rx->local->hw.wiphy->bands[] array only has 6 elements.

Fixes: 1d8d3dec5fbb ("mac80211: handle SMPS action frames")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 net/mac80211/rx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 93680af62c47..9b0cf9e3b7bd 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3287,6 +3287,9 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
 	if (len < IEEE80211_MIN_ACTION_SIZE)
 		return RX_DROP_UNUSABLE;
 
+	if (status->band >= NUM_NL80211_BANDS)
+		goto invalid;
+
 	if (!rx->sta && mgmt->u.action.category != WLAN_CATEGORY_PUBLIC &&
 	    mgmt->u.action.category != WLAN_CATEGORY_SELF_PROTECTED &&
 	    mgmt->u.action.category != WLAN_CATEGORY_SPECTRUM_MGMT)
-- 
2.20.1

Re: [PATCH] mac80211: prevent out of bounds access in ieee80211_rx_h_action()

[Johannes Berg]

On Wed, 2022-01-19 at 15:59 +0300, Dan Carpenter wrote:
> Smatch complains that status->band comes from the skb->data
> 

Hmm. How does it come to that conclusion? It's not really true? It comes
from skb->cb, and the driver should fill it. Also, we have:

void ieee80211_rx_list(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
                       struct sk_buff *skb, struct list_head *list)
{
...
        if (WARN_ON(status->band >= NUM_NL80211_BANDS))
                goto drop;


so I really don't think this patch is needed?

johannes

Re: [PATCH] mac80211: prevent out of bounds access in ieee80211_rx_h_action()

[Dan Carpenter]

On Wed, Jan 19, 2022 at 02:56:52PM +0100, Johannes Berg wrote:
> On Wed, 2022-01-19 at 15:59 +0300, Dan Carpenter wrote:
> > Smatch complains that status->band comes from the skb->data
> > 
> 
> Hmm. How does it come to that conclusion? It's not really true? It comes
> from skb->cb, and the driver should fill it.

Ugh...  Sorry.  I misread the code.  I spent some time trying to figure
this out as well, but I still didn't figure it out.

So, yeah.  It's skb->cb and Smatch for some reason thinks skb->cb holds
user data...  I will look into this.

> Also, we have:
> 
> void ieee80211_rx_list(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
>                        struct sk_buff *skb, struct list_head *list)
> {
> ...
>         if (WARN_ON(status->band >= NUM_NL80211_BANDS))
>                 goto drop;
> 
> 
> so I really don't think this patch is needed?

The problem with that is that ->cb is an array of char so Smatch doesn't
track status->band across function boundaries.

regards,
dan carpenter

Re: [PATCH] mac80211: prevent out of bounds access in ieee80211_rx_h_action()

[Dan Carpenter]

On Wed, Jan 19, 2022 at 06:07:33PM +0300, Dan Carpenter wrote:
> So, yeah.  It's skb->cb and Smatch for some reason thinks skb->cb holds
> user data...  I will look into this.

I fixed this bug.  I'll test the fix for a while before pushing it.

regards,
dan carpenter