Re: [PATCH v2] x86/bugs: Break down mitigations configurations

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Tue, Jun 27, 2023 at 10:36:10AM -0700, Breno Leitao wrote:
> Hello Pawan,
> 
> On Wed, Jun 21, 2023 at 12:41:01PM -0700, Pawan Gupta wrote:
> > On Wed, Jun 21, 2023 at 11:36:53AM -0700, Breno Leitao wrote:
> > > If I understand where you want to go, you think we should create a
> > > single patchset that creates a CONFIG_<MITIGATION> for each mitigation,
> > > and move get it under CONFIG_SPECULATION_MITIGATIONS.
> > 
> > Yes, a single series (or a patch) that adds config for each mitigation
> > would be good.
> 
> I've been working on this request, and I may need your help to validate
> the wordings and dependencies (as in architecture/vendors where the
> problem needs to be mitigations) for each entry.

Kconfig text looks fine to me. (Some comments on arch/vendor dependency
are down below).

> Also, I want to make sure I am not missing anything. Here is what I have
> so far. Is it in the right direction?
> 
> --
> Author: Breno Leitao <leitao@debian.org>
> Date:   Thu Jun 15 08:04:16 2023 -0700
> 
>     x86/bugs: Break down mitigations configurations

How about this?

x86/bugs: Add a separate config for each mitigation

>     Create an entry for each CPU mitigation under
>     CONFIG_SPECULATION_MITIGATIONS. This allow users to enable or disable
>     them at compilation time.
>     
>     If a mitigation is disabled at compilation time, it could be enabled at
>     runtime using kernel command line arguments.
>     
>     Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 53bab123a8ee..10ea7884eddd 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2649,6 +2649,100 @@ config SLS
>  	  against straight line speculation. The kernel image might be slightly
>  	  larger.
>  
> +config MITIGATE_MDS
> +	bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"
> +	depends on CPU_SUP_INTEL && X86_64

Architecture/vendor dependency is resolved at runtime during bug
enumeration (using CPU family/model). I don't think there is a need to
add explicit dependency here unless it creates runtime issues. And for
these configs it doesn't.

MDS and some of the other mitigations works for 32-bit kernel as well.
Dependency on X86_64 here is not correct, it makes 32-bit systems
vulnerable.

> +	default y
> +	help
> +	  Enable mitigation for Microarchitectural Data Sampling (MDS). MDS is
> +	  a hardware vulnerability which allows unprivileged speculative access
> +	  to data which is available in various CPU internal buffer. Deeper
> +	  technical information is available in the MDS specific x86 architecture
> +	  section: Documentation/arch/x86/mds.rst.
> +
> +config MITIGATE_TAA
> +	bool "Mitigate TSX Asynchronous Abort (TAA) hardware bug"
> +	depends on CPU_SUP_INTEL && X86_64

Ditto.

> +	default y
> +	help
> +	  Enable mitigation for TSX Asynchronous Abort (TAA). TAA is a hardware
> +	  vulnerability that allows unprivileged speculative access to data
> +	  which is available in various CPU internal buffers by using
> +	  asynchronous aborts within an Intel TSX transactional region.
> +
> +config MITIGATE_MMIO_STALE_DATA
> +	bool "Mitigate MMIO Stale Data hardware bug"
> +	depends on CPU_SUP_INTEL && X86_64

Ditto for and all the others.

[...]
> @@ -1286,7 +1316,7 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
>  
>  	ret = cmdline_find_option(boot_command_line, "spectre_v2", arg, sizeof(arg));
>  	if (ret < 0)
> -		return SPECTRE_V2_CMD_AUTO;
> +		return cmd;

In the same function, below code also needs to return the compile time
default:

        if (i >= ARRAY_SIZE(mitigation_options)) {
                pr_err("unknown option (%s). Switching to AUTO select\n", arg);
                return SPECTRE_V2_CMD_AUTO;
        }

[...]
> @@ -2119,7 +2153,12 @@ EXPORT_SYMBOL_GPL(itlb_multihit_kvm_mitigation);
>  #define pr_fmt(fmt)	"L1TF: " fmt
>  
>  /* Default mitigation for L1TF-affected CPUs */
> +

Extra newline.

> +#if IS_ENABLED(CONFIG_MITIGATE_L1TF)
>  enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH;
> +#else
> +enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_OFF;
> +#endif
>  #if IS_ENABLED(CONFIG_KVM_INTEL)
>  EXPORT_SYMBOL_GPL(l1tf_mitigation);
>  #endif