Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Rafał Miłecki]

On 11 March 2018 at 08:12, Kalle Valo  wrote:
> Rafał Miłecki  writes:
>
>> On 14 December 2017 at 14:21, Kalle Valo  wrote:
>>> Christian Lamparter  writes:
>>>
 On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
> Christian Lamparter  writes:
>
> > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall 
> > wrote:
> >> a additional array bounds check would be good
> >
> > Ah, about that:
> >
> > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
> > in the following way [0]:
> > |  bw = info2 & 3;
> >
> > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by 
> > [1]:
> > |  txrate.bw = ATH10K_HW_BW(peer_stats->flags);
> >
> > ATH10K_HW_BW is a macro defined as [2]:
> > |  #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
> >
> > In both cases the bandwidth values already are limited to 0-3 by
> > the "and 3" operation.
>
> Until someone changes that part of the code (and the firmware
> interface). IMHO a switch is safer as there we don't have any risk of
> out of bands access.

 The kbuild-bot/CI can catch this too.

 For example, it will look like this:
 drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid
 access past the end of 'ath10k_bw_to_mac80211' (4 4)
>>>
>>> Sure, but after reading about all these security vulnerabilities I have
>>> become even more cautious and try to avoid all tricky stuff.
>>>
 BTW:
 Have you noticed:

 

 Is this really your signed-off-by or not?
>>>
>>> I suspect that patch is taken from my pending branch.
>>>
 In any case, you - as the maintainer - can modify the patch as
 you see fit. So, please do so.
>>>
>>> Ok, we'll send v2.
>>
>> Hi Kalle,
>>
>> I'm trying to figure out the fate of that LEDE's patch. I don't think
>> you ever sent V2.
>>
>> Is that fix still needed? Are you planning to send V2?
>
> Anil now sent v2 (he just forgot to mark it as such):
>
> https://patchwork.kernel.org/patch/10273445/
>
> Thanks for the reminder.

Thanks!

-- 
Rafał

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Kalle Valo]

Rafał Miłecki  writes:

> On 14 December 2017 at 14:21, Kalle Valo  wrote:
>> Christian Lamparter  writes:
>>
>>> On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
 Christian Lamparter  writes:

 > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
 >> a additional array bounds check would be good
 >
 > Ah, about that:
 >
 > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
 > in the following way [0]:
 > |  bw = info2 & 3;
 >
 > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by 
 > [1]:
 > |  txrate.bw = ATH10K_HW_BW(peer_stats->flags);
 >
 > ATH10K_HW_BW is a macro defined as [2]:
 > |  #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
 >
 > In both cases the bandwidth values already are limited to 0-3 by
 > the "and 3" operation.

 Until someone changes that part of the code (and the firmware
 interface). IMHO a switch is safer as there we don't have any risk of
 out of bands access.
>>>
>>> The kbuild-bot/CI can catch this too.
>>>
>>> For example, it will look like this:
>>> drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid
>>> access past the end of 'ath10k_bw_to_mac80211' (4 4)
>>
>> Sure, but after reading about all these security vulnerabilities I have
>> become even more cautious and try to avoid all tricky stuff.
>>
>>> BTW:
>>> Have you noticed:
>>>
>>> 
>>>
>>> Is this really your signed-off-by or not?
>>
>> I suspect that patch is taken from my pending branch.
>>
>>> In any case, you - as the maintainer - can modify the patch as
>>> you see fit. So, please do so.
>>
>> Ok, we'll send v2.
>
> Hi Kalle,
>
> I'm trying to figure out the fate of that LEDE's patch. I don't think
> you ever sent V2.
>
> Is that fix still needed? Are you planning to send V2?

Anil now sent v2 (he just forgot to mark it as such):

https://patchwork.kernel.org/patch/10273445/

Thanks for the reminder.

-- 
Kalle Valo

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Rafał Miłecki]

On 14 December 2017 at 14:21, Kalle Valo  wrote:
> Christian Lamparter  writes:
>
>> On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
>>> Christian Lamparter  writes:
>>>
>>> > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
>>> >> a additional array bounds check would be good
>>> >
>>> > Ah, about that:
>>> >
>>> > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
>>> > in the following way [0]:
>>> > |  bw = info2 & 3;
>>> >
>>> > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
>>> > |  txrate.bw = ATH10K_HW_BW(peer_stats->flags);
>>> >
>>> > ATH10K_HW_BW is a macro defined as [2]:
>>> > |  #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
>>> >
>>> > In both cases the bandwidth values already are limited to 0-3 by
>>> > the "and 3" operation.
>>>
>>> Until someone changes that part of the code (and the firmware
>>> interface). IMHO a switch is safer as there we don't have any risk of
>>> out of bands access.
>>
>> The kbuild-bot/CI can catch this too.
>>
>> For example, it will look like this:
>> drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid
>> access past the end of 'ath10k_bw_to_mac80211' (4 4)
>
> Sure, but after reading about all these security vulnerabilities I have
> become even more cautious and try to avoid all tricky stuff.
>
>> BTW:
>> Have you noticed:
>>
>> 
>>
>> Is this really your signed-off-by or not?
>
> I suspect that patch is taken from my pending branch.
>
>> In any case, you - as the maintainer - can modify the patch as
>> you see fit. So, please do so.
>
> Ok, we'll send v2.

Hi Kalle,

I'm trying to figure out the fate of that LEDE's patch. I don't think
you ever sent V2.

Is that fix still needed? Are you planning to send V2?

-- 
Rafał

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Kalle Valo]

Christian Lamparter  writes:

> On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
>> Christian Lamparter  writes:
>> 
>> > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
>> >> a additional array bounds check would be good
>> >
>> > Ah, about that:
>> >
>> > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
>> > in the following way [0]:
>> > |  bw = info2 & 3;
>> >
>> > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
>> > |  txrate.bw = ATH10K_HW_BW(peer_stats->flags);
>> >
>> > ATH10K_HW_BW is a macro defined as [2]:
>> > |  #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
>> >
>> > In both cases the bandwidth values already are limited to 0-3 by
>> > the "and 3" operation.
>> 
>> Until someone changes that part of the code (and the firmware
>> interface). IMHO a switch is safer as there we don't have any risk of
>> out of bands access.
>
> The kbuild-bot/CI can catch this too. 
>
> For example, it will look like this:
> drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid
> access past the end of 'ath10k_bw_to_mac80211' (4 4)

Sure, but after reading about all these security vulnerabilities I have
become even more cautious and try to avoid all tricky stuff.

> BTW:
> Have you noticed:
>
> 
>
> Is this really your signed-off-by or not?

I suspect that patch is taken from my pending branch.

> In any case, you - as the maintainer - can modify the patch as
> you see fit. So, please do so.

Ok, we'll send v2.

-- 
Kalle Valo

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Christian Lamparter]

On Monday, November 20, 2017 11:57:21 AM CET Kalle Valo wrote:
> Christian Lamparter  writes:
> 
> > On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
> >> a additional array bounds check would be good
> >
> > Ah, about that:
> >
> > the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
> > in the following way [0]:
> > |   bw = info2 & 3;
> >
> > the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
> > |   txrate.bw = ATH10K_HW_BW(peer_stats->flags);
> >
> > ATH10K_HW_BW is a macro defined as [2]:
> > |   #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
> >
> > In both cases the bandwidth values already are limited to 0-3 by
> > the "and 3" operation.
> 
> Until someone changes that part of the code (and the firmware
> interface). IMHO a switch is safer as there we don't have any risk of
> out of bands access.

The kbuild-bot/CI can catch this too. 

For example, it will look like this:
drivers/net/wireless/ath/ath10k//htt_rx.c:710:52: warning: invalid access past 
the end of 'ath10k_bw_to_mac80211' (4 4)

BTW:
Have you noticed:



Is this really your signed-off-by or not?

In any case, you - as the maintainer - can modify the patch as
you see fit. So, please do so.

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Johannes Berg]

On Thu, 2017-11-02 at 22:08 +0100, Sebastian Gottschall wrote:
> i know. saw that later too. code should be safe

It would be good if you could adhere to our mailing list customs and
start quoting properly, instead of just top-posting.

Thanks,
johannes

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Sebastian Gottschall]

i know. saw that later too. code should be safe

Am 02.11.2017 um 20:34 schrieb Christian Lamparter:

On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:

a additional array bounds check would be good

Ah, about that:

the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
in the following way [0]:
|   bw = info2 & 3;

the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
|   txrate.bw = ATH10K_HW_BW(peer_stats->flags);

ATH10K_HW_BW is a macro defined as [2]:
|   #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)

In both cases the bandwidth values already are limited to 0-3 by
the "and 3" operation.

[0] 


[1] 

[2] 




@@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
   
   #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
   
+static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, RATE_INFO_BW_40,

+   RATE_INFO_BW_80, RATE_INFO_BW_160 };
+
   static void ath10k_htt_rx_h_rates(struct ath10k *ar,
  struct ieee80211_rx_status *status,
  struct htt_rx_desc *rxd)
@@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
if (sgi)
status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
   
[...]

+   status->bw = ath10k_bw_to_mac80211[bw];
status->encoding = RX_ENC_VHT;
break;
default:
@@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
   
   	arsta->txrate.nss = txrate.nss;

-   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
+   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
   }
   
   static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,






--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Christian Lamparter]

On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
> a additional array bounds check would be good

Ah, about that:

the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
in the following way [0]:
|   bw = info2 & 3;

the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
|   txrate.bw = ATH10K_HW_BW(peer_stats->flags);

ATH10K_HW_BW is a macro defined as [2]:
|   #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)

In both cases the bandwidth values already are limited to 0-3 by
the "and 3" operation.

[0] 


[1] 

[2] 



> > @@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
> >   
> >   #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
> >   
> > +static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, 
> > RATE_INFO_BW_40,
> > +   RATE_INFO_BW_80, RATE_INFO_BW_160 };
> > +
> >   static void ath10k_htt_rx_h_rates(struct ath10k *ar,
> >   struct ieee80211_rx_status *status,
> >   struct htt_rx_desc *rxd)
> > @@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
> > if (sgi)
> > status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
> >   
> > [...]
> > +   status->bw = ath10k_bw_to_mac80211[bw];
> > status->encoding = RX_ENC_VHT;
> > break;
> > default:
> > @@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
> > arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> >   
> > arsta->txrate.nss = txrate.nss;
> > -   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
> > +   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
> >   }
> >   
> >   static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Sebastian Gottschall]

a additional array bounds check would be good

Am 01.11.2017 um 21:01 schrieb Christian Lamparter:

The commit "cfg80211: make RATE_INFO_BW_20 the default" changed
the index of RATE_INFO_BW_20, but the updates to ath10k missed
the special bandwidth calculation case in
ath10k_update_per_peer_tx_stats().

Fixes: 842be75c77cb ("cfg80211: make RATE_INFO_BW_20 the default")
Signed-off-by: Christian Lamparter 
---
  drivers/net/wireless/ath/ath10k/htt_rx.c | 23 +--
  1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c 
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a3f5dc78353f..26b0d201a698 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
  
  #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
  
+static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, RATE_INFO_BW_40,

+   RATE_INFO_BW_80, RATE_INFO_BW_160 };
+
  static void ath10k_htt_rx_h_rates(struct ath10k *ar,
  struct ieee80211_rx_status *status,
  struct htt_rx_desc *rxd)
@@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
if (sgi)
status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
  
-		switch (bw) {

-   /* 20MHZ */
-   case 0:
-   break;
-   /* 40MHZ */
-   case 1:
-   status->bw = RATE_INFO_BW_40;
-   break;
-   /* 80MHZ */
-   case 2:
-   status->bw = RATE_INFO_BW_80;
-   break;
-   case 3:
-   status->bw = RATE_INFO_BW_160;
-   break;
-   }
-
+   status->bw = ath10k_bw_to_mac80211[bw];
status->encoding = RX_ENC_VHT;
break;
default:
@@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
  
  	arsta->txrate.nss = txrate.nss;

-   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
+   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
  }
  
  static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,



--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

[Sebastian Gottschall]

true. good finding.

Am 01.11.2017 um 21:01 schrieb Christian Lamparter:

The commit "cfg80211: make RATE_INFO_BW_20 the default" changed
the index of RATE_INFO_BW_20, but the updates to ath10k missed
the special bandwidth calculation case in
ath10k_update_per_peer_tx_stats().

Fixes: 842be75c77cb ("cfg80211: make RATE_INFO_BW_20 the default")
Signed-off-by: Christian Lamparter 
---
  drivers/net/wireless/ath/ath10k/htt_rx.c | 23 +--
  1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c 
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a3f5dc78353f..26b0d201a698 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
  
  #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
  
+static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, RATE_INFO_BW_40,

+   RATE_INFO_BW_80, RATE_INFO_BW_160 };
+
  static void ath10k_htt_rx_h_rates(struct ath10k *ar,
  struct ieee80211_rx_status *status,
  struct htt_rx_desc *rxd)
@@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
if (sgi)
status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
  
-		switch (bw) {

-   /* 20MHZ */
-   case 0:
-   break;
-   /* 40MHZ */
-   case 1:
-   status->bw = RATE_INFO_BW_40;
-   break;
-   /* 80MHZ */
-   case 2:
-   status->bw = RATE_INFO_BW_80;
-   break;
-   case 3:
-   status->bw = RATE_INFO_BW_160;
-   break;
-   }
-
+   status->bw = ath10k_bw_to_mac80211[bw];
status->encoding = RX_ENC_VHT;
break;
default:
@@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
  
  	arsta->txrate.nss = txrate.nss;

-   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
+   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
  }
  
  static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,



--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565