[MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Jan 16, 2020 at 02:16:07PM -0800, speck for mark gross wrote:
> From: mark gross <mgross@linux.intel.com>
> Subject: [PATCH 2/3] x86/speculation: Special Register Buffer Data Sampling
>  (SRBDS) mitigation control.
> 
> SRBDS is an MDS-like speculative side channel that can leak bits from
> the RNG across cores and threads. New microcode serializes the processor
> access during the execution of RDRAND and RDSEED.  This ensures that the
> shared buffer is overwritten before it is released for reuse.
> 
> While it is present on all affected CPU models, the microcode mitigation
> is not needed on models that enumerate ARCH_CAPABILITIES[MDS_NO] in the
> cases where TSX is not supported or has been disabled with TSX_CTRL.
> 
> The mitigation is activated by default on affected processors and it
> increases latency for RDRAND and RDSEED instructions.  Among other
> effects this will reduce throughput from /dev/urandom.
> 
> * enable administrator to configure the mitigation off when desired
>   using either mitigations=off or srbds=off.
> * export vulnerability status via sysfs
> * rename file scoped macros to apply for non-whitelist table
>   initializations.
> 
> Signed-off-by: Mark Gross <mgross@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>

Sorry for being a little late to the review.

> ---
>  .../ABI/testing/sysfs-devices-system-cpu      |   1 +
>  .../admin-guide/kernel-parameters.txt         |  11 ++
>  arch/x86/include/asm/cpufeatures.h            |   2 +
>  arch/x86/include/asm/msr-index.h              |   4 +
>  arch/x86/kernel/cpu/bugs.c                    | 112 +++++++++++++++
>  arch/x86/kernel/cpu/common.c                  | 128 +++++++++++-------
>  arch/x86/kernel/cpu/cpu.h                     |  13 ++
>  arch/x86/kernel/cpu/intel.c                   |   2 +
>  arch/x86/kernel/cpu/match.c                   |   6 +-
>  drivers/base/cpu.c                            |   8 ++
>  include/linux/mod_devicetable.h               |   2 +-
>  11 files changed, 236 insertions(+), 53 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 2e0e3b45d02a..a890b3769335 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -492,6 +492,7 @@ What:		/sys/devices/system/cpu/vulnerabilities
>  		/sys/devices/system/cpu/vulnerabilities/spec_store_bypass
>  		/sys/devices/system/cpu/vulnerabilities/l1tf
>  		/sys/devices/system/cpu/vulnerabilities/mds
> +		/sys/devices/system/cpu/vulnerabilities/special_register_data_sampling

That's not the actual name of the vulnerability.  The file should match
the actual name (or my personal preference, the "srbds" abbreviation).

>  		/sys/devices/system/cpu/vulnerabilities/tsx_async_abort
>  		/sys/devices/system/cpu/vulnerabilities/itlb_multihit
>  Date:		January 2018
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index c07815d230bc..71cf7e7eae45 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4659,6 +4659,17 @@
>  	spia_pedr=
>  	spia_peddr=
>  
> +	srbds=		[x86]

			[X86,INTEL]

(note the capital X for consistency with other entries and
kernel-parameters.rst)

> +			Special Register Buffer Data Sampling mitigation
> +			control.

			Control mitigation for the Special Register
			Buffer Data Sampling (SRBDS) mitigation.

> +
> +			On CPUs vulnerable to this issue and users impacted by
> +			the mitigation slowing down RDRAND and RDSEED can
> +			disable by setting this:

-ENOPARSE

How about:

			Certain CPUs are vulnerable to an MDS-like
			exploit which can leak bits from the random
			number generator.

			By default, this issue is mitigated by
			microcode.  However, the microcode fix can cause
			the RDRAND and RDSEED instructions to become
			much slower.  Among other effects, this will
			result in reduced throughput from /dev/urandom.

			The microcode mitigation can be disabled with
			the following option:

			off:	Disable mitigation and remove
				performance impact to RDRAND and RDSEED

> +
> +			off:	disable mitigation and remove performance
> +				impact to rdrand and rdseed
> +
>  	srcutree.counter_wrap_check [KNL]
>  			Specifies how frequently to check for
>  			grace-period sequence counter wrap for the
> @@ -397,6 +399,103 @@ static int __init tsx_async_abort_parse_cmdline(char *str)
>  }
>  early_param("tsx_async_abort", tsx_async_abort_parse_cmdline);
>  
> +#undef pr_fmt
> +#define pr_fmt(fmt)	"SRBDS: " fmt
> +
> +enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL;
> +static const char * const srbds_strings[] = {
> +	[SRBDS_NOT_AFFECTED]		= "Not affected",

I don't think the SRBDS_NOT_AFFECTED enum is needed.

The "Not affected" string is already printed by cpu_show_common().

And srbds_configure_mitigation() can just check for
boot_cpu_has_bug(X86_BUG_SRBDS) instead of this enum value.

> +	[SRBDS_MITIGATION_OFF]		= "Vulnerable",
> +	[SRBDS_MITIGATION_UCODE_NEEDED]	= "Vulnerable: no microcode",
> +	[SRBDS_MITIGATION_FULL]		= "Mitigated",
> +	[SRBDS_NOT_AFFECTED_TSX_OFF]	= "Not affected (TSX disabled)",
> +	[SRBDS_HYPERVISOR]		= "Unknown",
> +};
> +
> +static bool srbds_off;
> +
> +void srbds_configure_mitigation(void)

To me, "configure" sounds like it could be a software thing.

How about update_srbds_msr(), which is also more consistent with some of
the other naming in the file.

> +static void __init srbds_select_mitigation(void)
> +{
> +	u64 ia32_cap;
> +
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> +		srbds_mitigation = SRBDS_HYPERVISOR;
> +		return;
> +	}

It's confusing that this comes before the X86_BUG_SRBDS check.  Is that
on purpose?

If so, I presume it doesn't have the intended effect, because if booting
in a guest on a non-vulnerable CPU, srbds_mitigation will get set to
SRBDS_HYPERVISOR, but the shown state will still be "Not affected"
because of how cpu_show_common() checks for the bug at the beginning.

> +
> +	if (!boot_cpu_has_bug(X86_BUG_SRBDS)) {
> +		srbds_mitigation = SRBDS_NOT_AFFECTED;
> +		return;
> +	}
> +
> +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL)) {
> +		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> +		return;
> +	}
> +
> +	if (boot_cpu_has_bug(X86_BUG_SRBDS)) {
> +		srbds_mitigation = SRBDS_MITIGATION_FULL;
> +		/*
> +		 * Check to see if this is one of the MDS_NO systems supporting
> +		 * TSX that are only exposed to SRBDS when TSX is enabled.
> +		 */
> +		ia32_cap = x86_read_arch_cap_msr();
> +		if (ia32_cap & ARCH_CAP_MDS_NO) {
> +			if (!boot_cpu_has(X86_FEATURE_RTM))
> +				srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF;
> +		}

I wonder if this would be equivalent?

		if (!boot_cpu_has_bug(X86_BUG_MDS) && boot_cpu_has(X86_BUG_TAA) &&
		    !boot_cpu_has(X86_FEATURE_RTM))
		    	srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF;

> +	}
> +
> +	if (cpu_mitigations_off() || srbds_off) {
> +		if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF)
> +			srbds_mitigation = SRBDS_MITIGATION_OFF;
> +	}
> +
> +	srbds_configure_mitigation();
> +}

The other mitigations do a printk with the mitigation status here.
Should we not do the same here?

> +
> +static int __init srbds_parse_cmdline(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strcmp(str, "off"))
> +		srbds_off = true;
> +
> +	return 0;
> +}
> +early_param("srbds", srbds_parse_cmdline);
> +
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "Spectre V1 : " fmt
>  
> @@ -1528,6 +1627,11 @@ static char *ibpb_state(void)
>  	return "";
>  }
>  
> +static ssize_t srbds_show_state(char *buf)
> +{
> +	return sprintf(buf, "%s\n", srbds_strings[srbds_mitigation]);
> +}
> +
>  static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
>  			       char *buf, unsigned int bug)
>  {
> @@ -1572,6 +1676,9 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
>  	case X86_BUG_ITLB_MULTIHIT:
>  		return itlb_multihit_show_state(buf);
>  
> +	case X86_BUG_SRBDS:
> +		return srbds_show_state(buf);
> +
>  	default:
>  		break;
>  	}
> @@ -1618,4 +1725,9 @@ ssize_t cpu_show_itlb_multihit(struct device *dev, struct device_attribute *attr
>  {
>  	return cpu_show_common(dev, attr, buf, X86_BUG_ITLB_MULTIHIT);
>  }
> +
> +ssize_t cpu_show_special_register_data_sampling(struct device *dev, struct device_attribute *attr, char *buf)
> +{

This function name can be renamed to match whatever the filename turns
out to be:

  cpu_show_srbds()

or

  cpu_show_special_register_data_buffer_sampling()

> +	return cpu_show_common(dev, attr, buf, X86_BUG_SRBDS);
> +}
>  #endif
> --- a/arch/x86/kernel/cpu/cpu.h
> +++ b/arch/x86/kernel/cpu/cpu.h
> @@ -44,7 +44,20 @@ struct _tlb_table {
>  extern const struct cpu_dev *const __x86_cpu_dev_start[],
>  			    *const __x86_cpu_dev_end[];
>  
> +enum srbds_mitigations {
> +	SRBDS_NOT_AFFECTED,
> +	SRBDS_MITIGATION_OFF,
> +	SRBDS_MITIGATION_UCODE_NEEDED,
> +	SRBDS_MITIGATION_FULL,
> +	SRBDS_NOT_AFFECTED_TSX_OFF,
> +	SRBDS_HYPERVISOR,
> +};
> +
> +extern __ro_after_init enum srbds_mitigations srbds_mitigation;

srbds_mitigation isn't used outside of bugs.c, so it can just be a
static variable and the enum definition can live there as well.

> +void srbds_configure_mitigation(void);
> +
>  #ifdef CONFIG_CPU_SUP_INTEL
> +
>  enum tsx_ctrl_states {
>  	TSX_CTRL_ENABLE,
>  	TSX_CTRL_DISABLE,
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index be82cd5841c3..1b083a2a415b 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -684,6 +684,8 @@ static void init_intel(struct cpuinfo_x86 *c)
>  		tsx_enable();
>  	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
>  		tsx_disable();
> +
> +	srbds_configure_mitigation();
>  }

I'm not sure this is the right place to call this.  On the boot CPU,
this gets called *before* the mitigation is selected, and then again
from srbds_select_mitigation() itself.

Maybe it should instead be called from identify_secondary_cpu(), while
keeping the existing call from srbds_select_mitigation() so the boot CPU
also gets updated.

-- 
Josh