Message-ID: <20250217233928.k3oem3bm7w63jzfr@jpoimboe>
Date: Mon, 17 Feb 2025 15:39:28 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: "Kaplan, David" <David.Kaplan@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 20/35] x86/bugs: Define attack vectors

On Mon, Feb 17, 2025 at 08:38:07PM +0000, Kaplan, David wrote:
> > However... should these not actually be arch-generic options, like mitigations=
> > already is?  It would make for a more consistent user interface across arches.
> 
> That's what I had in my patch series up until this one.  Boris said to
> move them to x86-specific code because nobody else is using them yet
> and somebody down the road could move them.

Ah, I guess I missed the previous versions.  Sorry :-/

> I do agree that they can be arch-generic (hence why I originally put
> them in kernel/cpu.c) but I also don't know when (if ever) anyone from
> other archs will want to pick them up.

Well, for example, we already have the generic
prctl(PR_SET_SPECULATION_CTRL) which is used by arm64.  If somebody
boots with mitigate_user_user=off on arm64, they could reasonably expect
those context switch mitigations to be disabled.

> > They could even be integrated into the "mitigations=" interface.  The options could
> > be combined in any order (separated by commas):
> >
> >   mitigations=user_kernel,user_user
> >   mitigations=guest_host,user_kernel
> >   ...etc...
> >
> > And e.g., "mitigations=off" would simply disable all the vectors.
> 
> Hmm, that's an interesting idea.  I assume that any vectors not listed
> would be considered 'off', unless no mitigations= was specified, or
> mitigations=auto was specified in which case they'd default to 'on'
> like they do today.
>
> In other words:
> mitigations=auto
>   => all 4 vectors are 'on'
> mitigations=user_kernel
>   => user_kernel is 'on', all others are 'off'
>
> That would also imply that:
> mitigations=user_kernel mitigations=user_user
> 
> Would actually mean that user_user is 'on' and everything is 'off'.
> Not sure if that's an issue or would be sufficiently obvious.

Hm, so I was actually thinking that multiple "mitigations=" options
would combine.  I guess either way is confusing.  The problem is that
they have side effects.  Another idea below.

> Then a question is how to integrate the mitigate_smt option we were
> just discussing since that needs a 3-way select.  Or perhaps keep that
> one as a separate command line option.

Yeah, the smt tri-state would need some kind of interface as well.

> > That would remove ambiguity created by combining mitigations= with
> > mitigate_* and would make it easier for all the current
> > cpu_mitigations_off() callers: only one global enable/disable interface to call instead
> > of two.  Any code calling cpu_mitigations_off() should probably be calling something
> > like cpu_mitigate_attack_vector() instead.
> >
> > cpu_mitigations_off() and cpu_mitigations_auto_nosmt() could be deprecated in
> > favor of more vector-specific interfaces, and could be removed once all the arches
> > stop using them.  They could be gated by a temporary
> > ARCH_USES_MITIGATION_VECTORS option.  As could the per-vector cmdline
> > options.
> >
> > Thoughts?
> >
> 
> I'm not sure there is really that much ambiguity...the global
> mitigations=off is the big button that disables everything.

Well sure, but that's because you already know it's the big button ;-)
If you don't know that, it's nonobvious.

Joe Admin might assume the cmdline options are processed in order, e.g:

  mitigations=off mitigate_user_kernel=on

in which case they might reasonably expect to have only user->kernel
mitigations enabled.  Otherwise there would be no point in combining
them in the first place.

How about negative and positive versions of each:

  mitigations=[no_]user_kernel
  migitations=[no_]user_user

etc.

And then the mitigations= cmdline could simply be processed in order,
without side effects, to give the user full flexibility.  To opt-in to
specific vectors:

  mitigations=off mitigations=user_kernel,host_guest

which is equivalent to:

  mitigations=off,user_kernel,host_guest

Or, if one prefers to opt-out:

  mitigations=auto,no_user_user,no_guest_guest

where the "auto" is optional for default configs.

> I think the other issue here may be that the attack vectors are
> defined to be rather low-priority in terms of selection.  That is, you
> can disable all the attack vectors but then still enable an individual
> bug fix.
> 
> In other words, if you were to replace cpu_mitigations_off() with a
> function looking for all attack vectors to be off, that isn't quite
> correct because of the priority difference.

Hm.  So IIUC, the priority (in descending order) is:

  1) mitigations=

  2) individual mitigations, e.g. spectre_v2=

  3) mitigate_*=

  4) defaults

That seems overly complex and nonobvious, where most of the complexity
comes from handling rarely (or never?) used edge cases.

Does the current "big button" behavior for mitigations=off even make
sense?  Why would somebody do "mitigations=off spectre_v2=eibrs" and
expect the spectre_v2 mitigation to be *disabled*?  Do we really think
anybody relies on that and gets the result they were expecting?

The priority could be simplified:

  1) individual mitigations (=auto means use system-wide default)

  2) system-wide defaults (tweaked by mitigations=/mitigate_*=)

So the system-wide defaults would be defined by mitigations=whatever,
and those can be overriden by the individual mitigations.  That seems a
lot more simple and logical.

And since you're already introducing "=auto" for the individual
mitigations, I think that would be easy enough to implement.

-- 
Josh

Powered by blists - more mailing lists