Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Fri, Jan 12, 2018 at 12:01 PM, Christian Lamparter
 wrote:
> On Friday, January 12, 2018 7:39:50 PM CET Dan Williams wrote:
>> On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter  
>> wrote:
>> > On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
>> >> Static analysis reports that 'queue' may be a user controlled value that
>> >> is used as a data dependency to read from the 'ar9170_qmap' array. In
>> >> order to avoid potential leaks of kernel memory values, block
>> >> speculative execution of the instruction stream that could issue reads
>> >> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
>> >> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
>> >> 'ar->edcf' array.
>> >>
>> >> Based on an original patch by Elena Reshetova.
>> >>
>> >> Cc: Christian Lamparter 
>> >> Cc: Kalle Valo 
>> >> Cc: linux-wirel...@vger.kernel.org
>> >> Cc: netdev@vger.kernel.org
>> >> Signed-off-by: Elena Reshetova 
>> >> Signed-off-by: Dan Williams 
>> >> ---
>> > This patch (and p54, cw1200) look like the same patch?!
>> > Can you tell me what happend to:
>> >
>> > On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
>> >> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  
>> >> wrote:
>> >> > And Furthermore a invalid queue (param->ac) would cause a crash in
>> >> > this line in mac80211 before it even reaches the driver [1]:
>> >> > |   sdata->tx_conf[params->ac] = p;
>> >> > |   
>> >> > |   if (drv_conf_tx(local, sdata,  params->ac , )) {
>> >> > |^^ (this is a wrapper for the *_op_conf_tx)
>> >> >
>> >> > I don't think these chin-up exercises are needed.
>> >>
>> >> Quite the contrary, you've identified a better place in the call stack
>> >> to sanitize the input and disable speculation. Then we can kill the
>> >> whole class of the wireless driver reports at once it seems.
>> > 
>>
>> I didn't see where ac is being validated against the driver specific
>> 'queues' value in that earlier patch.
> The link to the check is right there in the earlier post. It's in
> parse_txq_params():
> 
> |   if (txq_params->ac >= NL80211_NUM_ACS)
> |   return -EINVAL;
>
> NL80211_NUM_ACS is 4
> 
>
> This check was added ever since mac80211's ieee80211_set_txq_params():
> | sdata->tx_conf[params->ac] = p;
>
> For cw1200: the driver just sets the dev->queue to 4.
> In carl9170 dev->queues is set to __AR9170_NUM_TXQ and
> p54 uses P54_QUEUE_AC_NUM.
>
> Both __AR9170_NUM_TXQ and P54_QUEUE_AC_NUM are 4.
> And this is not going to change since all drivers
> have to follow mac80211's queue API:
> 
>
> Some background:
> In the old days (linux 2.6 and early 3.x), the parse_txq_params() function did
> not verify the "queue" value. That's why these drivers had to do it.
>
> Here's the relevant code from 2.6.39:
> 
> You'll notice that the check is missing there.
> Here's mac80211's ieee80211_set_txq_params for reference:
> 
>
> However over time, the check in the driver has become redundant.
>

Excellent, thank you for pointing that out and the background so clearly.

What this tells me though is that we want to inject an ifence() at
this input validation point, i.e.:

if (txq_params->ac >= NL80211_NUM_ACS) {
ifence();
return -EINVAL;
}

...but the kernel, in these patches, only has ifence() defined for
x86. The only way we can sanitize the 'txq_params->ac' value without
ifence() is to do it at array access time, but then we're stuck
touching all drivers when standard kernel development practice says
'refactor common code out of drivers'.

Ugh, the bigger concern is that this driver is being flagged and not
that initial bounds check. Imagine if cw1200 and p54 had already been
converted to assume that they can just trust 'queue'. It would never
have been flagged.

I think we should focus on the get_user path and  __fcheck_files for v3.

Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Friday, January 12, 2018 7:39:50 PM CET Dan Williams wrote:
> On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter  
> wrote:
> > On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
> >> Static analysis reports that 'queue' may be a user controlled value that
> >> is used as a data dependency to read from the 'ar9170_qmap' array. In
> >> order to avoid potential leaks of kernel memory values, block
> >> speculative execution of the instruction stream that could issue reads
> >> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> >> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> >> 'ar->edcf' array.
> >>
> >> Based on an original patch by Elena Reshetova.
> >>
> >> Cc: Christian Lamparter 
> >> Cc: Kalle Valo 
> >> Cc: linux-wirel...@vger.kernel.org
> >> Cc: netdev@vger.kernel.org
> >> Signed-off-by: Elena Reshetova 
> >> Signed-off-by: Dan Williams 
> >> ---
> > This patch (and p54, cw1200) look like the same patch?!
> > Can you tell me what happend to:
> >
> > On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
> >> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  
> >> wrote:
> >> > And Furthermore a invalid queue (param->ac) would cause a crash in
> >> > this line in mac80211 before it even reaches the driver [1]:
> >> > |   sdata->tx_conf[params->ac] = p;
> >> > |   
> >> > |   if (drv_conf_tx(local, sdata,  params->ac , )) {
> >> > |^^ (this is a wrapper for the *_op_conf_tx)
> >> >
> >> > I don't think these chin-up exercises are needed.
> >>
> >> Quite the contrary, you've identified a better place in the call stack
> >> to sanitize the input and disable speculation. Then we can kill the
> >> whole class of the wireless driver reports at once it seems.
> > 
> 
> I didn't see where ac is being validated against the driver specific
> 'queues' value in that earlier patch.
The link to the check is right there in the earlier post. It's in 
parse_txq_params():

|   if (txq_params->ac >= NL80211_NUM_ACS)
|   return -EINVAL;

NL80211_NUM_ACS is 4


This check was added ever since mac80211's ieee80211_set_txq_params():
| sdata->tx_conf[params->ac] = p;

For cw1200: the driver just sets the dev->queue to 4.
In carl9170 dev->queues is set to __AR9170_NUM_TXQ and
p54 uses P54_QUEUE_AC_NUM.

Both __AR9170_NUM_TXQ and P54_QUEUE_AC_NUM are 4.
And this is not going to change since all drivers
have to follow mac80211's queue API:


Some background:
In the old days (linux 2.6 and early 3.x), the parse_txq_params() function did
not verify the "queue" value. That's why these drivers had to do it.

Here's the relevant code from 2.6.39:

You'll notice that the check is missing there.
Here's mac80211's ieee80211_set_txq_params for reference:


However over time, the check in the driver has become redundant.

> > Anyway, I think there's an easy way to solve this: remove the
> > "if (queue < ar->hw->queues)" check altogether. It's no longer needed
> > anymore as the "queue" value is validated long before the driver code
> > gets called.
> > 
> > And from my understanding, this will fix the "In this case
> > the value of 'ar9170_qmap[queue]' is immediately reused as an index to
> > the 'ar->edcf' array." gripe your tool complains about.
> >
> > This is here's a quick test-case for carl9170.:
> > ---
> > diff --git a/drivers/net/wireless/ath/carl9170/main.c 
> > b/drivers/net/wireless/ath/carl9170/main.c
> > index 988c8857d78c..2d3afb15bb62 100644
> > --- a/drivers/net/wireless/ath/carl9170/main.c
> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw 
> > *hw,
> > int ret;
> >
> > mutex_lock(>mutex);
> > -   if (queue < ar->hw->queues) {
> > -   memcpy(>edcf[ar9170_qmap[queue]], param, 
> > sizeof(*param));
> > -   ret = carl9170_set_qos(ar);
> > -   } else {
> > -   ret = -EINVAL;
> > -   }
> > -
> > +   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
> > +   ret = carl9170_set_qos(ar);
> > mutex_unlock(>mutex);
> > return ret;
> >  }
> > ---
> > What does your tool say about this?
> 
> If you take away the 'if' then I don't the tool will report on this.
> 
> > (If necessary, the "queue" value could be even sanitized with a
> > 

Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter  wrote:
> On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
>> Static analysis reports that 'queue' may be a user controlled value that
>> is used as a data dependency to read from the 'ar9170_qmap' array. In
>> order to avoid potential leaks of kernel memory values, block
>> speculative execution of the instruction stream that could issue reads
>> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
>> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
>> 'ar->edcf' array.
>>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: Christian Lamparter 
>> Cc: Kalle Valo 
>> Cc: linux-wirel...@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Elena Reshetova 
>> Signed-off-by: Dan Williams 
>> ---
> This patch (and p54, cw1200) look like the same patch?!
> Can you tell me what happend to:
>
> On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
>> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  
>> wrote:
>> > And Furthermore a invalid queue (param->ac) would cause a crash in
>> > this line in mac80211 before it even reaches the driver [1]:
>> > |   sdata->tx_conf[params->ac] = p;
>> > |   
>> > |   if (drv_conf_tx(local, sdata,  params->ac , )) {
>> > |^^ (this is a wrapper for the *_op_conf_tx)
>> >
>> > I don't think these chin-up exercises are needed.
>>
>> Quite the contrary, you've identified a better place in the call stack
>> to sanitize the input and disable speculation. Then we can kill the
>> whole class of the wireless driver reports at once it seems.
> 

I didn't see where ac is being validated against the driver specific
'queues' value in that earlier patch.

>
> Anyway, I think there's an easy way to solve this: remove the
> "if (queue < ar->hw->queues)" check altogether. It's no longer needed
> anymore as the "queue" value is validated long before the driver code
> gets called.

Can you point me to where that validation happens?

> And from my understanding, this will fix the "In this case
> the value of 'ar9170_qmap[queue]' is immediately reused as an index to
> the 'ar->edcf' array." gripe your tool complains about.
>
> This is here's a quick test-case for carl9170.:
> ---
> diff --git a/drivers/net/wireless/ath/carl9170/main.c 
> b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..2d3afb15bb62 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> int ret;
>
> mutex_lock(>mutex);
> -   if (queue < ar->hw->queues) {
> -   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
> -   ret = carl9170_set_qos(ar);
> -   } else {
> -   ret = -EINVAL;
> -   }
> -
> +   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
> +   ret = carl9170_set_qos(ar);
> mutex_unlock(>mutex);
> return ret;
>  }
> ---
> What does your tool say about this?

If you take away the 'if' then I don't the tool will report on this.

> (If necessary, the "queue" value could be even sanitized with a
> queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)

That is what array_ptr() is doing in a more sophisticated way.

Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
> Static analysis reports that 'queue' may be a user controlled value that
> is used as a data dependency to read from the 'ar9170_qmap' array. In
> order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue reads
> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> 'ar->edcf' array.
> 
> Based on an original patch by Elena Reshetova.
> 
> Cc: Christian Lamparter 
> Cc: Kalle Valo 
> Cc: linux-wirel...@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Dan Williams 
> ---
This patch (and p54, cw1200) look like the same patch?! 
Can you tell me what happend to:

On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  
> wrote:
> > And Furthermore a invalid queue (param->ac) would cause a crash in
> > this line in mac80211 before it even reaches the driver [1]:
> > |   sdata->tx_conf[params->ac] = p;
> > |   
> > |   if (drv_conf_tx(local, sdata,  params->ac , )) {
> > |^^ (this is a wrapper for the *_op_conf_tx)
> >
> > I don't think these chin-up exercises are needed.
> 
> Quite the contrary, you've identified a better place in the call stack
> to sanitize the input and disable speculation. Then we can kill the
> whole class of the wireless driver reports at once it seems.


Anyway, I think there's an easy way to solve this: remove the 
"if (queue < ar->hw->queues)" check altogether. It's no longer needed
anymore as the "queue" value is validated long before the driver code
gets called. And from my understanding, this will fix the "In this case
the value of 'ar9170_qmap[queue]' is immediately reused as an index to
the 'ar->edcf' array." gripe your tool complains about.

This is here's a quick test-case for carl9170.:
---
diff --git a/drivers/net/wireless/ath/carl9170/main.c 
b/drivers/net/wireless/ath/carl9170/main.c
index 988c8857d78c..2d3afb15bb62 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
int ret;
 
mutex_lock(>mutex);
-   if (queue < ar->hw->queues) {
-   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
-   ret = carl9170_set_qos(ar);
-   } else {
-   ret = -EINVAL;
-   }
-
+   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
+   ret = carl9170_set_qos(ar);
mutex_unlock(>mutex);
return ret;
 }
---
What does your tool say about this? 

(If necessary, the "queue" value could be even sanitized with a
queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)

[PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution

[Dan Williams]

Static analysis reports that 'queue' may be a user controlled value that
is used as a data dependency to read from the 'ar9170_qmap' array. In
order to avoid potential leaks of kernel memory values, block
speculative execution of the instruction stream that could issue reads
based on an invalid result of 'ar9170_qmap[queue]'. In this case the
value of 'ar9170_qmap[queue]' is immediately reused as an index to the
'ar->edcf' array.

Based on an original patch by Elena Reshetova.

Cc: Christian Lamparter 
Cc: Kalle Valo 
Cc: linux-wirel...@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Elena Reshetova 
Signed-off-by: Dan Williams 
---
 drivers/net/wireless/ath/carl9170/main.c |7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/main.c 
b/drivers/net/wireless/ath/carl9170/main.c
index 988c8857d78c..0acfa8c22b7d 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "hw.h"
@@ -1384,11 +1385,13 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
   const struct ieee80211_tx_queue_params *param)
 {
struct ar9170 *ar = hw->priv;
+   const u8 *elem;
int ret;
 
mutex_lock(>mutex);
-   if (queue < ar->hw->queues) {
-   memcpy(>edcf[ar9170_qmap[queue]], param, sizeof(*param));
+   elem = array_ptr(ar9170_qmap, queue, ar->hw->queues);
+   if (elem) {
+   memcpy(>edcf[*elem], param, sizeof(*param));
ret = carl9170_set_qos(ar);
} else {
ret = -EINVAL;