linux-kernel - Re: [PATCHv4 07/30] x86/traps: Add #VE support for TDX guest

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:   Fri, 25 Feb 2022 11:46:04 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        luto@...nel.org, peterz@...radead.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
        ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
        joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
        pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
        tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
        thomas.lendacky@....com, brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>
Subject: Re: [PATCHv4 07/30] x86/traps: Add #VE support for TDX guest

On 2/25/22 11:30, Kirill A. Shutemov wrote:
> On Thu, Feb 24, 2022 at 10:36:02AM -0800, Dave Hansen wrote:
>> On 2/24/22 07:56, Kirill A. Shutemov wrote:
>>> Virtualization Exceptions (#VE) are delivered to TDX guests due to
>>> specific guest actions which may happen in either user space or the
>>> kernel:
>>>
>>>  * Specific instructions (WBINVD, for example)
>>>  * Specific MSR accesses
>>>  * Specific CPUID leaf accesses
>>>  * Access to unmapped pages (EPT violation)
>>
>> Considering that you're talking partly about userspace, it would be nice
>> to talk about what "unmapped" really means here.
> 
> I'm not sure what you want to see here. Doesn't EPT violation describe it?
> 
> It can happen to userspace too, but we don't expect it to be use used and
> SIGSEGV the process if it happens.

How about just:

 * Access to specific guest physical addresses

That makes it clear that we're not really talking about userspace
unmapped pages.

...
>>> +	 * module also treats virtual NMIs as inhibited if the #VE valid flag is
>>> +	 * set, e.g. so that NMI=>#VE will not result in a #DF.
>>> +	 */
>>
>> Are we missing anything valuable if we just trim the comment down to
>> something like:
>>
>> 	/*
>> 	 * Called during #VE handling to retrieve the #VE info from the
>> 	 * TDX module.
>>  	 *
>> 	 * This should called done early in #VE handling.  A "nested"
>> 	 * #VE which occurs before this will raise a #DF and is not
>> 	 * recoverable.
>> 	 */
> 
> This variant of the comment lost information about #VE-valid flag and
> doesn't describe how NMI is inhibited.

IMNHO, the "#VE valid" flag is a super-fine implementation detail.  I'd
personally deal with that in Documentation or the changelog instead of a
comment.

>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>> +
>>> +#define VE_FAULT_STR "VE fault"
>>> +
>>> +static void ve_raise_fault(struct pt_regs *regs, long error_code)
>>> +{
>>> +	if (user_mode(regs)) {
>>> +		gp_user_force_sig_segv(regs, X86_TRAP_VE, error_code, VE_FAULT_STR);
>>> +		return;
>>> +	}
>>> +
>>> +	if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code, VE_FAULT_STR))
>>> +		return;
>>> +
>>> +	die_addr(VE_FAULT_STR, regs, error_code, 0);
>>> +}
>>> +
>>> +/*
>>> + * Virtualization Exceptions (#VE) are delivered to TDX guests due to
>>> + * specific guest actions which may happen in either user space or the
>>> + * kernel:
>>> + *
>>> + *  * Specific instructions (WBINVD, for example)
>>> + *  * Specific MSR accesses
>>> + *  * Specific CPUID leaf accesses
>>> + *  * Access to unmapped pages (EPT violation)
>>> + *
>>> + * In the settings that Linux will run in, virtualization exceptions are
>>> + * never generated on accesses to normal, TD-private memory that has been
>>> + * accepted.
>>
>> This actually makes a lot more sense as a code comment than changelog.
>> It would be really nice to circle back here and actually refer to the
>> functions that accept memory.
> 
> We don't have such functions at this point in the patchset. Do you want
> the comment to be updated once we get them introduced?

Yes, please.  Supplement the comment when the functions are introduced
later.