diff mbox series

[v2] wifi: rtl8xxxu: Fix skb misuse in TX queue selection

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

Commit Message

Bitterblue Smith Aug. 31, 2022, 4:12 p.m. UTC
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(-)

Comments

Kalle Valo Sept. 7, 2022, 8:02 a.m. UTC | #1
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
Jes Sorensen Sept. 7, 2022, 3:02 p.m. UTC | #2
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
Bitterblue Smith Sept. 9, 2022, 3:03 p.m. UTC | #3
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.
Jes Sorensen Sept. 9, 2022, 3:17 p.m. UTC | #4
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 mbox series

Patch

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) {