linux-kernel - Re: [PATCH 25/24] x86/traps: Rewrite native_load_gs

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]

Message-ID: <9fdb04b1-dbb8-069d-f5ef-d4e8c0f2a83e@zytor.com>
Date:   Tue, 7 Sep 2021 18:38:53 -0700
From:   "H. Peter Anvin" <hpa@...or.com>
To:     Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     Lai Jiangshan <laijs@...ux.alibaba.com>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Dave Jiang <dave.jiang@...el.com>,
        Ben Widawsky <ben.widawsky@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Arvind Sankar <nivedita@...m.mit.edu>
Subject: Re: [PATCH 25/24] x86/traps: Rewrite native_load_gs_index in C code

On 9/2/21 3:50 AM, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@...ux.alibaba.com>
> 
> There is no constrain/limition to force native_load_gs_index() to be in
> ASM code.

Hi,

First of all, let me say I really like your patchset, and I will try to
review it in detail ASAP (most of the initial read pass looks very sane
to me.

However, I would like to object in part this specific patch. It adds a
fair bit of extra code to the exception path, and adds jumps between
files which makes the code much harder to read.

You end up doing one swapgs in assembly and one in C, which would seem
to be a very good indication that really isn't an improvement.

Note that this entire sequence is scheduled to be obsoleted by a single
atomic hardware instruction, LKGS, which will replace ALL of
native_load_gs_index(); it will no longer be necessary even to disable
interrupts as there is no non-atomic state. In that sense, doing this as
an out-of-line C function (with some inline assembly) is great, because
it makes it far easier to use LKGS as an alternative; the only (small)
disadvantage is that it ends up clobbering a lot of registers
unnecessarily (in assembly it can be implemented clobbering only two
registers; one if one uses pushf/popf to save the interrupt flag.)

noinstr void native_load_gs_index(unsigned int selector)
{
	unsigned long flags;

	local_irq_save(flags);
	native_swapgs();
redo:
	asm goto("1: movl %0,%%gs\n",
		   "2:\n"
		   _ASM_EXTABLE(%1)
		   : : "r" (selector) : "i" (&&bad_seg);
	alternative("", "mfence", X86_BUG_SWAPGS_FENCE);
	native_swapgs();
	local_irq_restore(flags);
	return;

bad_seg:
	/* Exception entry will have restored kernel GS */
	native_swapgs();

	if (static_cpu_has(X86_BUG_NULL_SEG)) {
		asm volatile("movl %0,%%gs"
			: : "r" (__USER_DS));
	}
	selector = 0;
	goto redo;
}

	-hpa