historical-speck.lore.kernel.org archive mirror
 help / color / mirror / Atom feed
From: mark gross <mgross@linux.intel.com>
To: speck@linutronix.de
Subject: [MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2
Date: Fri, 27 Mar 2020 09:20:41 -0700	[thread overview]
Message-ID: <20200327162041.GA2707@mtg-dev.jf.intel.com> (raw)
In-Reply-To: <20200326031923.d7hwuq65bb7ymw6g@treble>

On Wed, Mar 25, 2020 at 10:19:23PM -0500, speck for Josh Poimboeuf wrote:
> 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.
Thanks for the reivew!

> 
> > ---
> >  .../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
> 
Looks like fine update to include in the next version to me, I'll use your
version.

> > +
> > +			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.

True.  I prefer having the complete state machine represented by the
enumeration but, if you feel strongly I'll try to remove it.

> 
> > +	[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.
We are configuring the meditation by honoring the kernel command line in this
function.

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

If you like but, I was mimicking the TAA implementation when choosing the name
of this function.

> 
> > +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?
VM vendors on keybase wanted it so.

> 
> 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.

The intended effect is to report "Unknown" for the vulnerability status of
SRBDS for all guest VM's running this code and to never touch the MSR
controlling the mitigation.

I think this is what happens.  I'll double check and confirm with testing.

> > +
> > +	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;
I have no clue if its equivalent but, I find your version much harder to parse
and my version matches more closely to what the white paper states.

> 
> > +	}
> > +
> > +	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?
Perhaps but, I was attempting to limit the extra chatter in the kernel logs.
If you feel it adds value to the code I'll put it in.  FWIW I think the chatter
from the other places should be removed.

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

I got some early review feedback to not use the shorter name "srbds" for the
sysfs inodes so I was trying to avoid the full spelled out the full names.

"srbds" and "special_register_data_buffer_sampling" do not imply HWRNG security
risk so IMO I'd like to just use srbds.  What do others think?


> 
> > +	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.

True, I'll remove the extern declaration its a left over from an earlier
implementation.


> > +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.
The SRBDS MSR needs to be poked from each CPU thread if you want to disable the
mitigation.  Does identify_secondary_cpu get called per HW thread?

I felt it would be nice to keep the call close to other mitigation setup
logic, in this case the TAA logic.

What do you think it will be more readable if I move it to
identify_secondary_cpu?

--mark

  reply	other threads:[~2020-03-27 16:20 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-18 21:27 [MODERATED] [PATCH 0/3] v4 more sampling fun 0 mark gross
2020-01-16 22:16 ` [MODERATED] [PATCH 2/3] v4 more sampling fun 2 mark gross
2020-01-30 19:12 ` [MODERATED] [PATCH 3/3] v4 more sampling fun 3 mark gross
2020-03-17  0:56 ` [MODERATED] [PATCH 1/3] v4 more sampling fun 1 mark gross
     [not found] ` <5e7296c7.1c69fb81.f9a2f.00ebSMTPIN_ADDED_BROKEN@mx.google.com>
2020-03-19  8:50   ` [MODERATED] " Greg KH
2020-03-19 15:40     ` mark gross
2020-03-19 15:50       ` Luck, Tony
2020-03-19 16:34         ` Greg KH
2020-03-19 18:13     ` Thomas Gleixner
2020-03-26  3:19 ` [MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2 Josh Poimboeuf
2020-03-27 16:20   ` mark gross [this message]
2020-03-27 17:23     ` Luck, Tony
2020-03-27 19:12       ` mark gross
2020-03-27 17:37     ` Josh Poimboeuf
2020-03-27 19:27       ` mark gross
2020-03-26  3:25 ` [MODERATED] Re: [PATCH 3/3] v4 more sampling fun 3 Josh Poimboeuf
2020-03-27 16:28   ` mark gross

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200327162041.GA2707@mtg-dev.jf.intel.com \
    --to=mgross@linux.intel.com \
    --cc=speck@linutronix.de \
    --subject='[MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).