linux-kernel - Re: [PATCH 16/29] x86/bugs: Disable Retpoline when IBT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Feb 2022 16:19:46 -0800
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, joao@...rdrivepizza.com, hjl.tools@...il.com,
        andrew.cooper3@...rix.com, linux-kernel@...r.kernel.org,
        ndesaulniers@...gle.com, keescook@...omium.org,
        samitolvanen@...gle.com, mark.rutland@....com,
        alyssa.milburn@...el.com
Subject: Re: [PATCH 16/29] x86/bugs: Disable Retpoline when IBT

On Tue, Feb 22, 2022 at 04:00:18PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 18, 2022 at 06:15:30PM -0800, Josh Poimboeuf wrote:
> 
> > This code is confusing, not helped by the fact that the existing code
> > already looks like spaghetti.
> 
> I'd say that's an insult to spaghetti.

:-)

> > Assuming IBT systems also have eIBRS (right?), I don't think the above
> > SPECTRE_V2_CMD_{FORCE,AUTO} cases would be possible.
> 
> Virt FTW.. if I don't handle it, some idiot will create a virtual
> machine that doesn't expose eIBRS but does do IBT just to spite me.

Ok, but in such a case, why not still do the warning, since the spectre
v2 mitigation isn't what the user might expect based on previous
behavior?

> 
> > AFAICT, if execution reached the retpoline_generic label, the user
> > specified either RETPOLINE or RETPOLINE_GENERIC.
> 
> Only RETPOLINE_GENERIC;

Hm?

	case SPECTRE_V2_CMD_RETPOLINE:
		if (IS_ENABLED(CONFIG_RETPOLINE))
			goto retpoline_auto;

retpoline_auto:
	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
		...
	} else {
	retpoline_generic:


> > I'm not sure it makes sense to put RETPOLINE in the "silent" list.  If
> > the user boots an Intel system with spectre_v2=retpoline on the cmdline,
> > they're probably expecting a traditional retpoline and should be warned
> > if that changes, especially if it's a "demotion".
> 
> too friggin bad as to expectations; retpoline == auto. Not saying that
> makes sense, just saying that's what it does.

Note quite.  Today it means "on Intel use the Intel retpoline; on AMD
use the AMD retpoline."

Intel doesn't recommend the AMD retpoline.  If you change that behavior
then it should be warned about so the user can adjust their mitigation
strategy accordingly.

> > In that case the switch statement isn't even needed.  It can instead
> > just unconditinoally print the warning.
> > 
> > 
> > Also, why "demote" retpoline to LFENCE rather than attempting to
> > "promote" it to eIBRS?  Maybe there's a good reason but it probably at
> > least deserves some mention in the commit log.
> 
> The current code will never select retpoline if eibrs is available.

Hm?  What do you think "spectre_v2=retpoline" does?

> The alternative is doing this in apply_retpolines(), but that might be
> even more nasty.

Hm?  Doing what in apply_retpolines()?

-- 
Josh