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@xxxxxxxxx> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@xxxxxxxxxxxxxxx> > --- > 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
