linux-kernel - Re: [PATCHv5.1 04/30] x86/tdx: Extend the confidential computing API to support TDX guests

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, 9 Mar 2022 10:36:25 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:     aarcange@...hat.com, ak@...ux.intel.com, bp@...en8.de,
        brijesh.singh@....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,
        linux-kernel@...r.kernel.org, luto@...nel.org, mingo@...hat.com,
        pbonzini@...hat.com, peterz@...radead.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, sdeep@...are.com,
        seanjc@...gle.com, tglx@...utronix.de, thomas.lendacky@....com,
        tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
        x86@...nel.org
Subject: Re: [PATCHv5.1 04/30] x86/tdx: Extend the confidential computing API
 to support TDX guests

On 3/9/22 08:01, Kirill A. Shutemov wrote:
> +/*
> + * Wrapper for __tdx_module_call() for cases when the call doesn't suppose to
> + * fail. Panic if the call fails.
> + */
> +static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> +				   struct tdx_module_output *out)
> +{
> +	if (__tdx_module_call(fn, rcx, rdx, r8, r9, out))
> +		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> +}

That comment didn't do much for me.  I know it's a wrapper.  I know it
panics() if the call returns a failure.  That's what the code *does*.  I
want a comment to tell me *why* it does that.

I _think_ I may have been getting this confused with the TDVMCALL mechanism.

All TDVMCALLs that return with rax==0 are fatal, we jump right to a ud2
instruction.  A __tdx_module_call() (via TDCALL) with rax==0 doesn't
*have* to be fatal.  But, this establishes a policy that all TDCALLs via
tdx_module_call() *ARE* fatal.

How about this for a comment?

/*
 * Used for TDX guests to make calls directly to the TD module.  This
 * should only be used for calls that have no legitimate reason to fail
 * or where the kernel can not survive the call failing.
 */

That tells me a *LOT*: This is a guest -> TD module thing.  Not a host
thing, not a hypercall.  And, no the naming isn't good enough to tell me
that.  Also, it give me advice.  It tells me when I should use this
function.  If it look at the call site, it even makes sense.  A guest
can't even build a sane PTE without this call succeeding.  If *COURSE*
we panic() if the call fails.

You could even call this information out in the comment in get_info():

...
	 * The GPA width that comes out of this call is critical.  TDX
	 * guests can not meaningfully run without it.
	 */

Then it all kinda fits together.  Oh, this panic() is awfully harsh.
Oh, it's only supposed to be used for important things that the guest
really needs.  Then there's a comment about why it needs it so badly.

Otherwise the panic() just looks superfluous and mean.