linux-kernel - Re: [PATCH v12 10/18] x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87v9ktw1ev.fsf@nanos.tec.linutronix.de>
Date:   Mon, 18 May 2020 20:20:08 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Sasha Levin <sashal@...nel.org>, linux-kernel@...r.kernel.org,
        bp@...en8.de, luto@...nel.org
Cc:     hpa@...or.com, dave.hansen@...el.com, tony.luck@...el.com,
        ak@...ux.intel.com, ravi.v.shankar@...el.com,
        chang.seok.bae@...el.com, Sasha Levin <sashal@...nel.org>,
        Andrew Cooper <andrew.cooper3@...rix.com>, x86@...nel.org
Subject: Re: [PATCH v12 10/18] x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions

Sasha Levin <sashal@...nel.org> writes:
> +unsigned long x86_gsbase_read_cpu_inactive(void)
> +{
> +	unsigned long gsbase;
> +
> +	if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
> +		bool need_restore = false;
> +		unsigned long flags;
> +
> +		/*
> +		 * We read the inactive GS base value by swapping
> +		 * to make it the active one. But we cannot allow
> +		 * an interrupt while we switch to and from.
> +		 */
> +		if (!irqs_disabled()) {
> +			local_irq_save(flags);
> +			need_restore = true;
> +		}
> +
> +		native_swapgs();
> +		gsbase = rdgsbase();
> +		native_swapgs();
> +
> +		if (need_restore)
> +			local_irq_restore(flags);

Where does this crap come from?

This conditional irqsave gunk is clearly NOT what was in the tip tree
before it got reverted:

  a86b4625138d ("x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions")

In https://lore.kernel.org/r/87ftcrtckg.fsf@nanos.tec.linutronix.de I
explicitely asked for this:

     - Made sure that the cleanups I did when merging them initially have
       been picked up. I'm not going to waste another couple of days on
       this mess just to revert it because it hadn't seen any serious
       testing in development.

and you confirmed in https://lore.kernel.org/r/20200426025243.GJ13035@sasha-vm

       Based on your revert
       (https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/cpu&id=049331f277fef1c3f2527c2c9afa1d285e9a1247)
       I believe that we have all the relevant patches in the series.

And the above while it might not have exploded yet, is simply broken
because the 'swapgs rd/wr swapgs' sequence is not protected against
kprobes. There is even a big fat comment in that original commit:

 /*
  * Out of line to be protected from kprobes. It is not used on Xen
  * paravirt. When paravirt support is needed, it needs to be renamed
  * with native_ prefix.
  */

Yes, you surely got all patches from the git tree and made sure that the
result reflects that.

I've just extracted the original commits from git and applied them and
fixed the trivial rejects. Then I diffed the result against this lot:

 - That above gunk, which is the worst of all

 - In paranoid_exit()

-	TRACE_IRQS_IRETQ_DEBUG
+	TRACE_IRQS_OFF_DEBUG

 - Dropped comments vs. FENCE_SWAPGS and a gazillion of comment
   changes to make reading the diff harder.

Then I gave up looking at it.

It took me ~ 20 minutes (ignoring selftests and documentation) to fixup
the rejects and create a patch queue which is reflecting the state
before the revert and does not have complete crap in it.

This required to add one preparatory patch dealing with the changes in
copy_thread_tls() and no, not by inlining all of that twice.

It took me another 5 minutes to get rid of the local_irq_save/restore()
in save_fsgs() on top without any conditional crap.

I'm seriously tired of this FSGSBASE mess. Every single version I've
looked at in several years was a trainwreck.

Don't bother to send out a new version of this in a frenzy. For my
mental sake I'm not going to look at yet another cobbled together
trainwreck anytime soon.

If you read the above carefully you might find a recipe of properly
engineering this so it's easy to verify against the old version.

Your's seriously grumpy

       tglx