linux-kernel - Re: [RFC PATCH 2/2] minstrel_ht: Mitigate BTI gadget minstrel_ht_get_expected

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2022 09:36:56 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
Cc:     scott.d.constable@...el.com, daniel.sneddon@...ux.intel.com,
        Jakub Kicinski <kuba@...nel.org>, dave.hansen@...el.com,
        Johannes Berg <johannes@...solutions.net>,
        Paolo Abeni <pabeni@...hat.com>,
        antonio.gomez.iglesias@...ux.intel.com,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        x86@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] minstrel_ht: Mitigate BTI gadget
 minstrel_ht_get_expected_throughput()

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@...el.com>
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.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