linux-kernel - Re: [PATCHv5, REBASED 3/4] x86/tdx: Dynamically disable SEPT violations from causing #VEs

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] [day] [month] [year] [list]

Message-ID: <17ee53d0-37ce-48d7-a9e7-ee3ca83ddfb1@intel.com>
Date: Wed, 28 Aug 2024 12:44:33 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
CC: Dave Hansen <dave.hansen@...ux.intel.com>, Thomas Gleixner
	<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
	<bp@...en8.de>, <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	<linux-coco@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<stable@...r.kernel.org>
Subject: Re: [PATCHv5, REBASED 3/4] x86/tdx: Dynamically disable SEPT
 violations from causing #VEs



On 27/08/2024 10:04 pm, Kirill A. Shutemov wrote:
> On Wed, Aug 21, 2024 at 01:52:49PM +1200, Huang, Kai wrote:
>>> + * attribute is no longer reliable. It reflects the initial state of the
>>> + * control for the TD, but it will not be updated if someone (e.g. bootloader)
>>> + * changes it before the kernel starts. Kernel must check TDCS_TD_CTLS bit to
>>> + * determine if SEPT #VEs are enabled or disabled.
>>> + */
>>> +static void disable_sept_ve(u64 td_attr)
>>> +{
>>> +	const char *msg = "TD misconfiguration: SEPT #VE has to be disabled";
>>
>> The original msg was:
>>
>> 	"TD misconfiguration: SEPT_VE_DISABLE attribute must be set."
>>
>> Any reason to change?
> 
> Because the attribute is not the only way to control if #VE is going to be
> injected.
> 
>>
>>
>>> +	bool debug = td_attr & ATTR_DEBUG;
>>> +	u64 config, controls;
>>> +
>>> +	/* Is this TD allowed to disable SEPT #VE */
>>> +	tdg_vm_rd(TDCS_CONFIG_FLAGS, &config);
>>> +	if (!(config & TDCS_CONFIG_FLEXIBLE_PENDING_VE)) {
>>
>> Does this field ID exist in TDX1.0?  I.e., whether it can fail here and
>> should we check the return value first?
> 
> See TDG.VM.RD definition:
> 
> R8  Contents of the field
>      In case of no success, as indicated by RAX, R8 returns 0.
> 
> No need in error checking here.

OK. Thanks.

> 
>>> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
>>> index 7e12cfa28bec..fecb2a6e864b 100644
>>> --- a/arch/x86/include/asm/shared/tdx.h
>>> +++ b/arch/x86/include/asm/shared/tdx.h
>>> @@ -19,9 +19,17 @@
>>>    #define TDG_VM_RD			7
>>>    #define TDG_VM_WR			8
>>> -/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
>>> +/* TDX TD-Scope Metadata. To be used by TDG.VM.WR and TDG.VM.RD */
>>
>> I am not sure whether this change is necessary.
> 
> It is more in-line with spec json dump.
> 
>>> +#define TDCS_CONFIG_FLAGS		0x1110000300000016
>>> +#define TDCS_TD_CTLS			0x1110000300000017
>>
>> The TDX 1.5 spec 'td_scope_metadata.json' says they are 0x9110000300000016
>> and 0x9110000300000017.
> 
> The spec is broken. It is going to be fixed. I use correct values.

OK.  I didn't know they are going to change the value in the JSON file.

> 
>> I know the bit 63 is ignored by the TDX module, but since (IIUC) those two
>> fields are introduced in TDX1.5, it's just better to follow what TDX1.5 spec
>> says.
> 
> Newer modules will ignore this bit and both values are going to
> acceptable.

Yeah.

Acked-by: Kai Huang <kai.huang@...el.com>