| Message ID | 7fa4819a-4f20-b2af-b7a6-8ee01ac49295@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | edd5747aa12ed61a5ecbfa58d3908623fddbf1e8 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: rtl8xxxu: Fix skb misuse in TX queue selection | expand |
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > rtl8xxxu_queue_select() selects the wrong TX queues because it's > reading memory from the wrong address. It expects to find ieee80211_hdr > at skb->data, but that's not the case after skb_push(). Move the call > to rtl8xxxu_queue_select() before the call to skb_push(). > > Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> Patch applied to wireless-next.git, thanks. edd5747aa12e wifi: rtl8xxxu: Fix skb misuse in TX queue selection
On 8/31/22 12:12, Bitterblue Smith wrote: > rtl8xxxu_queue_select() selects the wrong TX queues because it's > reading memory from the wrong address. It expects to find ieee80211_hdr > at skb->data, but that's not the case after skb_push(). Move the call > to rtl8xxxu_queue_select() before the call to skb_push(). > > Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > v2: > Add Fixes tag. > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Nice catch! > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 52240e945b58..8d6f693bf60b 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -5177,6 +5177,8 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, > if (control && control->sta) > sta = control->sta; > > + queue = rtl8xxxu_queue_select(hw, skb); > + > tx_desc = skb_push(skb, tx_desc_size); > > memset(tx_desc, 0, tx_desc_size); > @@ -5189,7 +5191,6 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, > is_broadcast_ether_addr(ieee80211_get_DA(hdr))) > tx_desc->txdw0 |= TXDESC_BROADMULTICAST; > > - queue = rtl8xxxu_queue_select(hw, skb); > tx_desc->txdw1 = cpu_to_le32(queue << TXDESC_QUEUE_SHIFT); This could actually be made more resilient from someone moving the code around by passing in 'hdr' instead of relying on using skb->data in rtl8xxxu_queue_select(). We could also get rid of the hw argument to that function since it isn't used. Cheers, Jes
On 07/09/2022 18:02, Jes Sorensen wrote: > On 8/31/22 12:12, Bitterblue Smith wrote: [...] >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> index 52240e945b58..8d6f693bf60b 100644 >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> @@ -5177,6 +5177,8 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, >> if (control && control->sta) >> sta = control->sta; >> >> + queue = rtl8xxxu_queue_select(hw, skb); >> + >> tx_desc = skb_push(skb, tx_desc_size); >> >> memset(tx_desc, 0, tx_desc_size); >> @@ -5189,7 +5191,6 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, >> is_broadcast_ether_addr(ieee80211_get_DA(hdr))) >> tx_desc->txdw0 |= TXDESC_BROADMULTICAST; >> >> - queue = rtl8xxxu_queue_select(hw, skb); >> tx_desc->txdw1 = cpu_to_le32(queue << TXDESC_QUEUE_SHIFT); > > This could actually be made more resilient from someone moving the code > around by passing in 'hdr' instead of relying on using skb->data in > rtl8xxxu_queue_select(). We could also get rid of the hw argument to > that function since it isn't used. > > Cheers, > Jes > Good ideas! I'll do that when I'm done fiddling with the RTL8188FU.
On 9/9/22 11:03, Bitterblue Smith wrote: > On 07/09/2022 18:02, Jes Sorensen wrote: >> On 8/31/22 12:12, Bitterblue Smith wrote: > [...] >>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> index 52240e945b58..8d6f693bf60b 100644 >>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> @@ -5177,6 +5177,8 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, >>> if (control && control->sta) >>> sta = control->sta; >>> >>> + queue = rtl8xxxu_queue_select(hw, skb); >>> + >>> tx_desc = skb_push(skb, tx_desc_size); >>> >>> memset(tx_desc, 0, tx_desc_size); >>> @@ -5189,7 +5191,6 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, >>> is_broadcast_ether_addr(ieee80211_get_DA(hdr))) >>> tx_desc->txdw0 |= TXDESC_BROADMULTICAST; >>> >>> - queue = rtl8xxxu_queue_select(hw, skb); >>> tx_desc->txdw1 = cpu_to_le32(queue << TXDESC_QUEUE_SHIFT); >> >> This could actually be made more resilient from someone moving the code >> around by passing in 'hdr' instead of relying on using skb->data in >> rtl8xxxu_queue_select(). We could also get rid of the hw argument to >> that function since it isn't used. >> >> Cheers, >> Jes >> > > Good ideas! I'll do that when I'm done fiddling with the RTL8188FU. Sounds good! Thanks, Jes
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index 52240e945b58..8d6f693bf60b 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -5177,6 +5177,8 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, if (control && control->sta) sta = control->sta; + queue = rtl8xxxu_queue_select(hw, skb); + tx_desc = skb_push(skb, tx_desc_size); memset(tx_desc, 0, tx_desc_size); @@ -5189,7 +5191,6 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw, is_broadcast_ether_addr(ieee80211_get_DA(hdr))) tx_desc->txdw0 |= TXDESC_BROADMULTICAST; - queue = rtl8xxxu_queue_select(hw, skb); tx_desc->txdw1 = cpu_to_le32(queue << TXDESC_QUEUE_SHIFT); if (tx_info->control.hw_key) {
rtl8xxxu_queue_select() selects the wrong TX queues because it's reading memory from the wrong address. It expects to find ieee80211_hdr at skb->data, but that's not the case after skb_push(). Move the call to rtl8xxxu_queue_select() before the call to skb_push(). Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- v2: Add Fixes tag. --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)