| Message ID | 20220119125923.GB9509@kili (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: prevent out of bounds access in ieee80211_rx_h_action() | expand |
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
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
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
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)
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(+)