[RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Pawan Gupta]

Hi,

There is a theoretical possibility of using
minstrel_ht_get_expected_throughput() as a disclosure gadget for Branch
History Injection (BHI)/Intra-mode Branch Target Injection (IMBTI) [1].
Requesting feedback on the couple of patches that mitigates this.

First patch adds a generic speculation barrier. Second patch uses the
speculation barrier to mitigate BHI/IMBTI.

The other goal of this series is to start a discussion on whether such
hard to exploit, but theoretical possible attacks deems to be mitigated.

In general Branch Target Injection class of attacks involves an adversary
controlling an indirect branch target to misspeculate to a disclosure gadget.
For a successful attack an adversary also needs to control the register
contents used by the disclosure gadget.

Assuming preconditions are met, a disclosure gadget would transiently do
below:

  1. Loads an attacker chosen data from memory.
  2. Based on the data, modifies cache state that is observable by an attacker.

Although both these operations are architecturally invisible, the cache state
changes could be used to infer the data.

Disclosure gadget is mitigated by adding a speculation barrier.

Thanks,
Pawan

[1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/branch-history-injection.html

Pawan Gupta (2):
  nospec: Add a generic barrier_nospec()
  minstrel_ht: Mitigate BTI gadget minstrel_ht_get_expected_throughput()

 include/linux/nospec.h             | 4 ++++
 net/mac80211/rc80211_minstrel_ht.c | 9 +++++++++
 2 files changed, 13 insertions(+)

-- 
2.37.3

[RFC PATCH 1/2] nospec: Add a generic barrier_nospec()

[Pawan Gupta]

barrier_nospec() is a speculation barrier with an arch dependent
implementation. Architectures that don't need a speculation barrier
shouldn't have to define an arch specific version.

To be able to use barrier_nospec() in non-architecture code add a
generic version that does nothing. Architectures needing speculation
barrier can override the generic version in their asm/barrier.h.

Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 include/linux/nospec.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index c1e79f72cd89..60e040a5df27 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -60,6 +60,10 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 	(typeof(_i)) (_i & _mask);					\
 })
 
+#ifndef barrier_nospec
+#define barrier_nospec()	do { } while (0)
+#endif
+
 /* Speculation control prctl */
 int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which);
 int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
-- 
2.37.3

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

[Pawan Gupta]

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(+)

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.
+	 *
+	 * Transiently executing this function with an adversary controlled
+	 * argument may disclose secrets. Speculation barrier prevents that.
+	 */
+	barrier_nospec();
+
 	i = MI_RATE_GROUP(mi->max_tp_rate[0]);
 	j = MI_RATE_IDX(mi->max_tp_rate[0]);
 	prob = mi->groups[i].rates[j].prob_avg;
-- 
2.37.3

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

[Greg KH]

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

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

[Pawan Gupta]

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.

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Peter Zijlstra]

On Mon, Oct 24, 2022 at 03:57:45PM -0700, Pawan Gupta wrote:

> The other goal of this series is to start a discussion on whether such
> hard to exploit, but theoretical possible attacks deems to be mitigated.
> 
> In general Branch Target Injection class of attacks involves an adversary
> controlling an indirect branch target to misspeculate to a disclosure gadget.
> For a successful attack an adversary also needs to control the register
> contents used by the disclosure gadget.

I'm thinking this is going about it wrong. You're going to be randomly
sprinking LFENCEs around forever if you go down this path making stuff
slower and slower.

Not to mention that it's going to bitrot; the function might change but
the LFENCE will stay, protecting nothing but still being slow.

I think the focus should be on finding the source sites, not protecting
the target sites. Where can an attacker control the register content and
have an indirect jump/call.

Also, things like FineIBT will severely limit the viability of all this.

And how is sprinking random LFENCEs around better than running with
spectre_v2=eibrs,retpoline which is the current recommended mitigation
against all this IIRC (or even eibrs,lfence for lesser values of
paranoia).

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Pawan Gupta]

On Tue, Oct 25, 2022 at 01:07:52PM +0200, Peter Zijlstra wrote:
>On Mon, Oct 24, 2022 at 03:57:45PM -0700, Pawan Gupta wrote:
>
>> The other goal of this series is to start a discussion on whether such
>> hard to exploit, but theoretical possible attacks deems to be mitigated.
>>
>> In general Branch Target Injection class of attacks involves an adversary
>> controlling an indirect branch target to misspeculate to a disclosure gadget.
>> For a successful attack an adversary also needs to control the register
>> contents used by the disclosure gadget.
>
>I'm thinking this is going about it wrong. You're going to be randomly
>sprinking LFENCEs around forever if you go down this path making stuff
>slower and slower.

Right, an alternative to LFENCE is to mask the indexes(wherever
possible) for gadgets that are called frequently. But still its not a
clean solution.

>Not to mention that it's going to bitrot; the function might change but
>the LFENCE will stay, protecting nothing but still being slow.

Totally agree with this.

>I think the focus should be on finding the source sites, not protecting
>the target sites. Where can an attacker control the register content and
>have an indirect jump/call.

That is an interesting approach. I am wondering what mitigation can
be applied at source? LFENCE before an indirect branch can greatly
reduce the speculation window, but will not completely eliminate it.
Also LFENCE at sources could be costlier than masking the indexes at
targets or LFENCE at the targets.

>Also, things like FineIBT will severely limit the viability of all this.

Yes.

>And how is sprinking random LFENCEs around better than running with
>spectre_v2=eibrs,retpoline which is the current recommended mitigation
>against all this IIRC (or even eibrs,lfence for lesser values of
>paranoia).

Its a trade-off between performance and spot fixing (hopefully handful
of) gadgets. Even the gadget in question here is not demonstrated to be
exploitable. If this scenario changes, polluting the kernel all over is
definitely not the right approach.

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Johannes Berg]

On Tue, 2022-10-25 at 12:38 -0700, Pawan Gupta wrote:
> 
> > And how is sprinking random LFENCEs around better than running with
> > spectre_v2=eibrs,retpoline which is the current recommended mitigation
> > against all this IIRC (or even eibrs,lfence for lesser values of
> > paranoia).
> 
> Its a trade-off between performance and spot fixing (hopefully handful
> of) gadgets. Even the gadget in question here is not demonstrated to be
> exploitable. If this scenario changes, polluting the kernel all over is
> definitely not the right approach.
> 
Btw, now I'm wondering - you were detecting these with the compiler
based something, could there be a compiler pass to insert appropriate
things, perhaps as a gcc plugin or something?

Now honestly I have no idea if it's feasible, but since you're detecting
it that way, and presumably then we'd have to maintain the detection and
run it regularly to make sure that (a) things didn't bitrot and the
gadget is still there, and (b) no new places show up ... perhaps the
better way would be to combine both?

johannes

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Pawan Gupta]

On Tue, Oct 25, 2022 at 09:56:21PM +0200, Johannes Berg wrote:
>On Tue, 2022-10-25 at 12:38 -0700, Pawan Gupta wrote:
>>
>> > And how is sprinking random LFENCEs around better than running with
>> > spectre_v2=eibrs,retpoline which is the current recommended mitigation
>> > against all this IIRC (or even eibrs,lfence for lesser values of
>> > paranoia).
>>
>> Its a trade-off between performance and spot fixing (hopefully handful
>> of) gadgets. Even the gadget in question here is not demonstrated to be
>> exploitable. If this scenario changes, polluting the kernel all over is
>> definitely not the right approach.
>>
>Btw, now I'm wondering - you were detecting these with the compiler
>based something, could there be a compiler pass to insert appropriate
>things, perhaps as a gcc plugin or something?

I hear it could be a lot of work for gcc. I am not sure if its worth
especially when we can't establish the exploitability of these gadgets.
There are some other challenges like, hot-path sites would prefer to
mask the indexes instead of using a speculation barrier for performance
reasons. I assume adding this intelligence to compilers would be
extremely hard. Also hardware controls and features in newer processors
will make the software mitigations redundant.

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Peter Zijlstra]

On Tue, Oct 25, 2022 at 12:38:45PM -0700, Pawan Gupta wrote:

> > I think the focus should be on finding the source sites, not protecting
> > the target sites. Where can an attacker control the register content and
> > have an indirect jump/call.
> 
> That is an interesting approach. I am wondering what mitigation can
> be applied at source?

Limiting the value ranges for example. Or straight up killing the values
if they go unused -- like how we clear the registers in entry.

> LFENCE before an indirect branch can greatly
> reduce the speculation window, but will not completely eliminate it.

Depends on the part; there's a whole bunch of parts where LFENCE is
sufficient.

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Dave Hansen]

On 10/25/22 04:07, Peter Zijlstra wrote:
> I think the focus should be on finding the source sites, not protecting
> the target sites. Where can an attacker control the register content and
> have an indirect jump/call.

How would this work with something like 'struct file_operations' which
provide a rich set of indirect calls that frequently have fully
user-controlled values in registers?

It certainly wouldn't *hurt* to be zeroing out the registers that are
unused at indirect call sites.  But, the majority of gadgets in this
case used rdi and rsi, which are the least likely to be able to be
zapped at call sites.

Re: [RFC PATCH 0/2] Branch Target Injection (BTI) gadget in minstrel

[Peter Zijlstra]

On Tue, Oct 25, 2022 at 03:00:35PM -0700, Dave Hansen wrote:
> On 10/25/22 04:07, Peter Zijlstra wrote:
> > I think the focus should be on finding the source sites, not protecting
> > the target sites. Where can an attacker control the register content and
> > have an indirect jump/call.
> 
> How would this work with something like 'struct file_operations' which
> provide a rich set of indirect calls that frequently have fully
> user-controlled values in registers?
> 
> It certainly wouldn't *hurt* to be zeroing out the registers that are
> unused at indirect call sites.  But, the majority of gadgets in this
> case used rdi and rsi, which are the least likely to be able to be
> zapped at call sites.

Right; so FineIBT will limit the targets to the right set of functions,
and those functions must already assume the values are user controlled
and take appropriate measures.

If you really truly care about the old hardware, then one solution would
be to de-virtualize the call using LTO or something (yes, it will need
some compiler work and you might need to annotate the code a bit and
even have a fixed/predetermined set of loadable modules, but meh).

Barring that, you could perhaps put {min,max} range information next to
the function pointer such that you can impose value ranges before doing
the indirect call.

But given this is all theoretical and FineIBT solves a lot of it I can't
find myself to care too much.