linux-kernel - Re: [PATCHv3 2/3] x86/tdx: Clarify RIP adjustments in #VE handler

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:   Wed, 25 May 2022 09:02:48 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        luto@...nel.org, peterz@...radead.org
Cc:     ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, linux-kernel@...r.kernel.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, seanjc@...gle.com,
        thomas.lendacky@....com, x86@...nel.org
Subject: Re: [PATCHv3 2/3] x86/tdx: Clarify RIP adjustments in #VE handler

On 5/24/22 15:10, Kirill A. Shutemov wrote:
> +static int ve_instr_len(struct ve_info *ve)
> +{
> +	/*
> +	 * If the #VE happened due to instruction execution, GET_VEINFO
> +	 * provides info on the instruction.
> +	 *
> +	 * For #VE due to EPT violation, info provided by GET_VEINFO not usable
> +	 * and kernel has to decode instruction manually to find out its
> +	 * length. Catch such cases.
> +	 */
> +	if (WARN_ON_ONCE(ve->exit_reason == EXIT_REASON_EPT_VIOLATION))
> +		return 0;
> +
> +	return ve->instr_len;
> +}

I'm not super happy with how this comment ended up.  First, let's put
the comment next to the code to which it applies, like:

	/*
	 * ve->instr_len is not defined for EPT violations.  For those,
	 * the kernel must decode instructions manually and should not
	 * be using this function.
	 */
	if (WARN_ON_ONCE(ve->exit_reason == EXIT_REASON_EPT_VIOLATION))
		return 0;

	/*
	 * Assume that the #VE occurred due to instruction execution.
	 */
	return ve->instr_len;

Second, there also needs to be *something* here to link this back to the
TDX module spec and the VMCS documentation.  To make actual sense of
this, you need to tie together something like three or four pieces of
disparate information scattered across two massive documents.

Intel really made this quite the scavenger hunt.  Time to atone for the
sins of your colleagues by tying all of those things together in one place.