Message-ID:
 <LV3PR12MB9265041C9D8D4F3E5760F0B694FA2@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Tue, 18 Feb 2025 02:24:01 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
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

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@...nel.org>
> Sent: Monday, February 17, 2025 5:39 PM
> 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; H . Peter Anvin
> <hpa@...or.com>; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v3 20/35] x86/bugs: Define attack vectors
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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.

I'm not sure which this is an argument for actually :)

That is, I do agree that mitigate_user_user makes sense in an arch-agnostic way and a user might expect context switch mitigations disabled.  However this patch series doesn't implement these attack vector controls for anything other than x86.  So I guess I'm not sure if your argument is that because they aren't yet implemented for arm64, then keeping them x86-specific is better...or if we should make them generic to more easily support extension to other architectures.


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

Yeah I'm not a big fan of this either, since I don't think this is consistent with how other mitigation command lines are handled.

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

I'd probably vote for just making that a separate command line option.

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

I don't like this idea as much as the next, because as noted above, I think with most other command lines, a later version replaces an earlier one, it doesn't append to it.

That is, something like "spectre_v2=retpoline spectre_v2=ibrs" ends up just meaning ibrs.

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

This seems more appealing to me because I think it's clearer what 'on' vs 'off' is.  It retains the more compact form of the command line while also allowing for opt-in or opt-out style.  And if you specify multiple "mitigations=" command lines, the new one replaces the old one, like with most other options.

So I rather like this, and would be interested in what others think too.

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

Your understanding is correct, although I'd argue there isn't really a case 4, since there are default settings for the attack vectors (case 3) which step in if 1) or 2) do not exist.  And case 1 is only 'mitigations=off'.

Having individual mitigations override attack vectors to me makes sense because they're more specific.  I think the bigger question is the big mitigations=off hammer.

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

A valid question, but this gets into changing existing behavior and that makes me nervous.

I can't say for certain what people do, but I could potentially imagine a system that has some custom configuration of bug options, and then for testing somebody decides to just append 'mitigations=off' at the end of the command line.  Perhaps they might reasonably expect that to disable everything?  And that is how it'd work today.

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

Yes it wouldn't be very hard, and it does make logical sense.  But I think the big caveat is the change to the existing mitigations=off and that it would no longer override individual mitigations.  Changing that would seem to be a risk, and is that risk worth taking?  It doesn't seem like it's that much harder to implement the 3-tier scheme (mitigations=off, individual, attack-vectors), especially since everything already checks for cpu_mitigations_off().

--David Kaplan

Powered by blists - more mailing lists