linux-kernel - Re: [PATCH v6 07/14] x86: Secure Launch kernel early boot stub

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: <d0f4704a-8050-9274-e4ac-b2b3157cf482@oracle.com>
Date:   Mon, 15 May 2023 16:13:31 -0400
From:   Ross Philipson <ross.philipson@...cle.com>
To:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
        x86@...nel.org, linux-integrity@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-crypto@...r.kernel.org,
        iommu@...ts.linux-foundation.org, kexec@...ts.infradead.org,
        linux-efi@...r.kernel.org
Cc:     dpsmith@...rtussolutions.com, mingo@...hat.com, bp@...en8.de,
        hpa@...or.com, ardb@...nel.org, mjg59@...f.ucam.org,
        James.Bottomley@...senpartnership.com, luto@...capital.net,
        nivedita@...m.mit.edu, kanth.ghatraju@...cle.com,
        trenchboot-devel@...glegroups.com,
        Ross Philipson <ross.philipson@...cle.com>
Subject: Re: [PATCH v6 07/14] x86: Secure Launch kernel early boot stub

On 5/12/23 14:04, Thomas Gleixner wrote:
> 
> On Thu, May 04 2023 at 14:50, Ross Philipson wrote:
>> +
>> +/* CPUID: leaf 1, ECX, SMX feature bit */
>> +#define X86_FEATURE_BIT_SMX	(1 << 6)
>> +
>> +/* Can't include apiddef.h in asm */
> 
> Why not? All it needs is a #ifndef __ASSEMBLY__ guard around the C parts.

I guess I was reluctant to fiddle with another header file but I can do 
this.

> 
>> +#define XAPIC_ENABLE	(1 << 11)
>> +#define X2APIC_ENABLE	(1 << 10)
>> +
>> +/* Can't include traps.h in asm */
> 
> NMI_VECTOR is defined in irq_vectors.h which just has a include
> <linux/threads.h> for no real good reason.

Ack

> 
>> +#define X86_TRAP_NMI	2
> 
> <SNIP>
> 
>> +/*
>> + * See the comment in head_64.S for detailed informatoin on what this macro
>> + * is used for.
>> + */
>> +#define rva(X) ((X) - sl_stub_entry)
> 
> I'm having a hard time to find that comment in head_64.S. At least it's
> not in this patch.

The is a macro very much like this one and large comment in head_64.S. I 
am just referencing that. If you would rather see that comment 
duplicated here, I can.

> 
>> +.Lsl_ap_cs:
>> +	/* Load the relocated AP IDT */
> [ 11 more citation lines. Click/Enter to show. ]
>> +	lidt	(sl_ap_idt_desc - sl_txt_ap_wake_begin)(%ecx)
>> +
>> +	/* Fixup MTRRs and misc enable MSR on APs too */
>> +	call	sl_txt_load_regs
>> +
>> +	/* Enable SMI with GETSEC[SMCTRL] */
>> +	GETSEC $(SMX_X86_GETSEC_SMCTRL)
>> +
>> +	/* IRET-to-self can be used to enable NMIs which SENTER disabled */
>> +	leal	rva(.Lnmi_enabled_ap)(%ebx), %eax
>> +	pushfl
>> +	pushl	$(__SL32_CS)
>> +	pushl	%eax
>> +	iret
> 
> So from here on any NMI which hits the AP before it can reach the wait
> loop will corrupt EDX...
> 
>> +/* This is the beginning of the relocated AP wake code block */
>> +	.global sl_txt_ap_wake_begin
> [ 10 more citation lines. Click/Enter to show. ]
>> +sl_txt_ap_wake_begin:
>> +
>> +	/*
>> +	 * Wait for NMI IPI in the relocated AP wake block which was provided
>> +	 * and protected in the memory map by the prelaunch code. Leave all
>> +	 * other interrupts masked since we do not expect anything but an NMI.
>> +	 */
>> +	xorl	%edx, %edx
>> +
>> +1:
>> +	hlt
>> +	testl	%edx, %edx
>> +	jz	1b
> 
> This really makes me nervous. A stray NMI and the AP starts going.
> 
> Can't this NMI just bring the AP out of HLT w/o changing any state and
> the AP evaluates a memory location which indicates whether it should
> start up or not.

Given you comments on patch 09, perhaps all this NMI business becomes 
obsolete.

> 
>> +	/*
>> +	 * This is the long absolute jump to the 32b Secure Launch protected
>> +	 * mode stub code in the rmpiggy. The jump address will be fixed in
> 
> Providing an actual name for the stub might spare to rummage through
> code to figure out where this is supposed to jump to.

That can be done.

> 
>> +	 * the SMP boot code when the first AP is brought up. This whole area
>> +	 * is provided and protected in the memory map by the prelaunch code.
> [ 2 more citation lines. Click/Enter to show. ]
>> +	 */
>> +	.byte	0xea
>> +sl_ap_jmp_offset:
>> +	.long	0x00000000
>> +	.word	__SL32_CS
> 
> Thanks,
> 
> 	tglx

Thanks
Ross