Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Saturday, January 6, 2018 4:06:21 PM CET Alan Cox wrote:
> > The only way a user can set this in any meaningful way would be via
> > a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> > vetted there by cfg80211's parse_txq_params [0]. This is long before
> 
> Far more than a couple of hundred instructions ?
Well, the user would have to send a netlink message each time. So
cfg80211 can parse it (this is where the initial "if queue >= 4 " check
happen). So the CPU would have to continue through and into 
rdev_set_txq_params() to get to mac80211's ieee80211_set_txq_params().
Then pass through that before gets to the driver's op_tx_conf. Once
there the driver code aquires a mutex_lock too before it gets to
check the queue value again.

Is this enough and how would the mutex_lock fit in there? Or can
the CPU skip past this as well? 
> The problem is that the processor will speculate that the parameter
> is valid and continue on that basis until the speculation resolves
> or it hits some other limit that stops it speculating further.
> That can be quite a distance on a modern x86 processor, and for all
> we know could be even more on some of the other CPUs involved.
 
> > it reaches any of the *_op_conf_tx functions.
> > 
> > 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;
> > |   
> 
> Firstly it might not because the address you get as a result could be
> valid kernel memory. In fact your attackers wants it to be valid kernel
> memory since they are trying to find the value of a piece of that memory.
> 
> Secondly the processor is doing this speculatively so it won't fault. It
> will eventually decide it went the wrong way and throw all the
> speculative work away - leaving footprints. It won't fault unless the
> speculative resolves that was the real path the code took.
> 
> If it's not a performance critical path then it's better to be safe.
Thank you for reading the canary too.

Christian

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Saturday, January 6, 2018 4:06:21 PM CET Alan Cox wrote:
> > The only way a user can set this in any meaningful way would be via
> > a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> > vetted there by cfg80211's parse_txq_params [0]. This is long before
> 
> Far more than a couple of hundred instructions ?
Well, the user would have to send a netlink message each time. So
cfg80211 can parse it (this is where the initial "if queue >= 4 " check
happen). So the CPU would have to continue through and into 
rdev_set_txq_params() to get to mac80211's ieee80211_set_txq_params().
Then pass through that before gets to the driver's op_tx_conf. Once
there the driver code aquires a mutex_lock too before it gets to
check the queue value again.

Is this enough and how would the mutex_lock fit in there? Or can
the CPU skip past this as well? 
> The problem is that the processor will speculate that the parameter
> is valid and continue on that basis until the speculation resolves
> or it hits some other limit that stops it speculating further.
> That can be quite a distance on a modern x86 processor, and for all
> we know could be even more on some of the other CPUs involved.
 
> > it reaches any of the *_op_conf_tx functions.
> > 
> > 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;
> > |   
> 
> Firstly it might not because the address you get as a result could be
> valid kernel memory. In fact your attackers wants it to be valid kernel
> memory since they are trying to find the value of a piece of that memory.
> 
> Secondly the processor is doing this speculatively so it won't fault. It
> will eventually decide it went the wrong way and throw all the
> speculative work away - leaving footprints. It won't fault unless the
> speculative resolves that was the real path the code took.
> 
> If it's not a performance critical path then it's better to be safe.
Thank you for reading the canary too.

Christian

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  wrote:
> On Saturday, January 6, 2018 2:10:37 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: net...@vger.kernel.org
>> Signed-off-by: Elena Reshetova 
>> Signed-off-by: Dan Williams 
>> ---
>>  drivers/net/wireless/ath/carl9170/main.c |6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/carl9170/main.c 
>> b/drivers/net/wireless/ath/carl9170/main.c
>> index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
>> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {
>> + memcpy(>edcf[*elem], param, sizeof(*param));
>>   ret = carl9170_set_qos(ar);
>>   } else {
>>   ret = -EINVAL;
>>
>>
> About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx:
>
> The only way a user can set this in any meaningful way would be via
> a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> vetted there by cfg80211's parse_txq_params [0]. This is long before
> it reaches any of the *_op_conf_tx functions.
>
> 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.

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter  wrote:
> On Saturday, January 6, 2018 2:10:37 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: net...@vger.kernel.org
>> Signed-off-by: Elena Reshetova 
>> Signed-off-by: Dan Williams 
>> ---
>>  drivers/net/wireless/ath/carl9170/main.c |6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/carl9170/main.c 
>> b/drivers/net/wireless/ath/carl9170/main.c
>> index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
>> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {
>> + memcpy(>edcf[*elem], param, sizeof(*param));
>>   ret = carl9170_set_qos(ar);
>>   } else {
>>   ret = -EINVAL;
>>
>>
> About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx:
>
> The only way a user can set this in any meaningful way would be via
> a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> vetted there by cfg80211's parse_txq_params [0]. This is long before
> it reaches any of the *_op_conf_tx functions.
>
> 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.

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Alan Cox]

> The only way a user can set this in any meaningful way would be via
> a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> vetted there by cfg80211's parse_txq_params [0]. This is long before

Far more than a couple of hundred instructions ? The problem is that the
processor will speculate that the parameter is valid and continue on that
basis until the speculation resolves or it hits some other limit that
stops it speculating further. That can be quite a distance on a modern
x86 processor, and for all we know could be even more on some of the
other CPUs involved.

> it reaches any of the *_op_conf_tx functions.
> 
> 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;
> |   

Firstly it might not because the address you get as a result could be
valid kernel memory. In fact your attackers wants it to be valid kernel
memory since they are trying to find the value of a piece of that memory.

Secondly the processor is doing this speculatively so it won't fault. It
will eventually decide it went the wrong way and throw all the
speculative work away - leaving footprints. It won't fault unless the
speculative resolves that was the real path the code took.

If it's not a performance critical path then it's better to be safe.

Alan

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Alan Cox]

> The only way a user can set this in any meaningful way would be via
> a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
> vetted there by cfg80211's parse_txq_params [0]. This is long before

Far more than a couple of hundred instructions ? The problem is that the
processor will speculate that the parameter is valid and continue on that
basis until the speculation resolves or it hits some other limit that
stops it speculating further. That can be quite a distance on a modern
x86 processor, and for all we know could be even more on some of the
other CPUs involved.

> it reaches any of the *_op_conf_tx functions.
> 
> 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;
> |   

Firstly it might not because the address you get as a result could be
valid kernel memory. In fact your attackers wants it to be valid kernel
memory since they are trying to find the value of a piece of that memory.

Secondly the processor is doing this speculatively so it won't fault. It
will eventually decide it went the wrong way and throw all the
speculative work away - leaving footprints. It won't fault unless the
speculative resolves that was the real path the code took.

If it's not a performance critical path then it's better to be safe.

Alan

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Saturday, January 6, 2018 2:10:37 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: net...@vger.kernel.org
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Dan Williams 
> ---
>  drivers/net/wireless/ath/carl9170/main.c |6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/main.c 
> b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {
> + memcpy(>edcf[*elem], param, sizeof(*param));
>   ret = carl9170_set_qos(ar);
>   } else {
>   ret = -EINVAL;
> 
> 
About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx:

The only way a user can set this in any meaningful way would be via
a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
vetted there by cfg80211's parse_txq_params [0]. This is long before
it reaches any of the *_op_conf_tx functions.

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.

[0] 
[1] 

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Christian Lamparter]

On Saturday, January 6, 2018 2:10:37 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: net...@vger.kernel.org
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Dan Williams 
> ---
>  drivers/net/wireless/ath/carl9170/main.c |6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/main.c 
> b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {
> + memcpy(>edcf[*elem], param, sizeof(*param));
>   ret = carl9170_set_qos(ar);
>   } else {
>   ret = -EINVAL;
> 
> 
About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx:

The only way a user can set this in any meaningful way would be via
a NL80211_CMD_SET_WIPHY netlink message. However, the value will get
vetted there by cfg80211's parse_txq_params [0]. This is long before
it reaches any of the *_op_conf_tx functions.

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.

[0] 
[1] 

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Sergei Shtylyov]

Hello!

On 1/6/2018 4:10 AM, 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: net...@vger.kernel.org
Signed-off-by: Elena Reshetova 
Signed-off-by: Dan Williams 
---
  drivers/net/wireless/ath/carl9170/main.c |6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/main.c 
b/drivers/net/wireless/ath/carl9170/main.c
index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
+   if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {


   I bet this causes checkpatch.pl to complain. I don't see a dire need to 
upset it here, the assignment may well precede *if*...



+   memcpy(>edcf[*elem], param, sizeof(*param));
ret = carl9170_set_qos(ar);
} else {
ret = -EINVAL;



MBR, Sergei

Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution

[Sergei Shtylyov]

Hello!

On 1/6/2018 4:10 AM, 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: net...@vger.kernel.org
Signed-off-by: Elena Reshetova 
Signed-off-by: Dan Williams 
---
  drivers/net/wireless/ath/carl9170/main.c |6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/main.c 
b/drivers/net/wireless/ath/carl9170/main.c
index 988c8857d78c..0ff34cbe2b62 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,12 @@ 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));
+   if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {


   I bet this causes checkpatch.pl to complain. I don't see a dire need to 
upset it here, the assignment may well precede *if*...



+   memcpy(>edcf[*elem], param, sizeof(*param));
ret = carl9170_set_qos(ar);
} else {
ret = -EINVAL;



MBR, Sergei