[RFC,2/2] minstrel_ht: Mitigate BTI gadget minstrel_ht_get_expected_throughput()

  Static analysis indicate that indirect target
minstrel_ht_get_expected_throughput() could be used as a disclosure
gadget for Intra-mode Branch Target Injection (IMBTI) and Branch History
Injection (BHI).

ASM generated by compilers indicate a construct of a typical disclosure
gadget, where an adversary-controlled register contents can be used to
transiently access an arbitrary memory location.

Although there are no known ways to exploit this, but to be on safer
side mitigate it by adding a speculation barrier.

Reported-by: Scott D. Constable <scott.d.constable@intel.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 net/mac80211/rc80211_minstrel_ht.c | 9 +++++++++
 1 file changed, 9 insertions(+)
  

On Mon, Oct 24, 2022 at 03:57:47PM -0700, Pawan Gupta wrote:
> Static analysis indicate that indirect target
> minstrel_ht_get_expected_throughput() could be used as a disclosure
> gadget for Intra-mode Branch Target Injection (IMBTI) and Branch History
> Injection (BHI).

You define these new TLAs here, but the code comment below does not,
making this code now impossible to understand :(

> ASM generated by compilers indicate a construct of a typical disclosure
> gadget, where an adversary-controlled register contents can be used to
> transiently access an arbitrary memory location.

If you have an "adveraray-controlled register contents", why would you
waste that on a mere speculation attack and not do something better,
like get root instead?

> Although there are no known ways to exploit this, but to be on safer
> side mitigate it by adding a speculation barrier.
> 
> Reported-by: Scott D. Constable <scott.d.constable@intel.com>
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  net/mac80211/rc80211_minstrel_ht.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> index 3d91b98db099..7cf90666a865 100644
> --- a/net/mac80211/rc80211_minstrel_ht.c
> +++ b/net/mac80211/rc80211_minstrel_ht.c
> @@ -11,6 +11,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/ieee80211.h>
>  #include <linux/minmax.h>
> +#include <linux/nospec.h>
>  #include <net/mac80211.h>
>  #include "rate.h"
>  #include "sta_info.h"
> @@ -1999,6 +2000,14 @@ static u32 minstrel_ht_get_expected_throughput(void *priv_sta)
>  	struct minstrel_ht_sta *mi = priv_sta;
>  	int i, j, prob, tp_avg;
>  
> +	/*
> +	 * Protect against IMBTI/BHI.

This makes no sense here, right?

And you are NOT following the proper networking comment style, didn't
checkpatch complain about this?

> +	 *
> +	 * Transiently executing this function with an adversary controlled
> +	 * argument may disclose secrets. Speculation barrier prevents that.
> +	 */
> +	barrier_nospec();

So how much did you just slow down the normal use of the system?

> +
>  	i = MI_RATE_GROUP(mi->max_tp_rate[0]);
>  	j = MI_RATE_IDX(mi->max_tp_rate[0]);

These are all internal structures, can't you just bounds-prevent the
speculation instead of the hard barrier?

thanks,

greg k-h
  

On Tue, Oct 25, 2022 at 09:36:56AM +0200, Greg KH wrote:
>On Mon, Oct 24, 2022 at 03:57:47PM -0700, Pawan Gupta wrote:
>> Static analysis indicate that indirect target
>> minstrel_ht_get_expected_throughput() could be used as a disclosure
>> gadget for Intra-mode Branch Target Injection (IMBTI) and Branch History
>> Injection (BHI).
>
>You define these new TLAs here, but the code comment below does not,
>making this code now impossible to understand :(

I will expand the TLAs in the comment.

>> ASM generated by compilers indicate a construct of a typical disclosure
>> gadget, where an adversary-controlled register contents can be used to
>> transiently access an arbitrary memory location.
>
>If you have an "adveraray-controlled register contents", why would you
>waste that on a mere speculation attack and not do something better,
>like get root instead?

In the non-transient path those registers can contain system call
arguments that are checked for illegal accesses, thus are harmless. But
when executing transiently those registers could be interpreted as
(completely unrelated) arguments of a disclosure gadget.

>> Although there are no known ways to exploit this, but to be on safer
>> side mitigate it by adding a speculation barrier.
>>
>> Reported-by: Scott D. Constable <scott.d.constable@intel.com>
>> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
>> ---
>>  net/mac80211/rc80211_minstrel_ht.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
>> index 3d91b98db099..7cf90666a865 100644
>> --- a/net/mac80211/rc80211_minstrel_ht.c
>> +++ b/net/mac80211/rc80211_minstrel_ht.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/moduleparam.h>
>>  #include <linux/ieee80211.h>
>>  #include <linux/minmax.h>
>> +#include <linux/nospec.h>
>>  #include <net/mac80211.h>
>>  #include "rate.h"
>>  #include "sta_info.h"
>> @@ -1999,6 +2000,14 @@ static u32 minstrel_ht_get_expected_throughput(void *priv_sta)
>>  	struct minstrel_ht_sta *mi = priv_sta;
>>  	int i, j, prob, tp_avg;
>>
>> +	/*
>> +	 * Protect against IMBTI/BHI.
>
>This makes no sense here, right?

I will expand those and add some more explanation.

>And you are NOT following the proper networking comment style, didn't
>checkpatch complain about this?

checkpatch did complain, but I noticed that this file is following
regular commenting style everywhere. I can changed that to networking
style but it will differ from the rest of the file.

>> +	 *
>> +	 * Transiently executing this function with an adversary controlled
>> +	 * argument may disclose secrets. Speculation barrier prevents that.
>> +	 */
>> +	barrier_nospec();
>
>So how much did you just slow down the normal use of the system?

I don't have data for this. As I understand this function is not called
frequently, so perf impact is not expected to be significant.

>> +
>>  	i = MI_RATE_GROUP(mi->max_tp_rate[0]);
>>  	j = MI_RATE_IDX(mi->max_tp_rate[0]);
>
>These are all internal structures, can't you just bounds-prevent the
>speculation instead of the hard barrier?

The valid bound in this case is large enough (bits 15:6 IIRC) to still
pose a risk. As this function is not called frequently adding a
speculation barrier looks to be the best choice.