linux-kernel - Re: [PATCH 08/10] x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE

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:   Mon, 17 Jul 2023 06:35:40 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Hansen, Dave" <dave.hansen@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Subject: Re: [PATCH 08/10] x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE_CALL
 assembly


> > +/* Called from __tdx_hypercall() for unrecoverable failure */
> > +static noinstr void __tdx_hypercall_failed(void)
> > +{
> > +	instrumentation_begin();
> > +	panic("TDVMCALL failed. TDX module bug?");
> > +}
> 
> So what's the deal with this instrumentation here. The instruction is 
> noinstr, so you want to make just the panic call itself instrumentable?, 
> if so where's the instrumentation_end() cal;?No instrumentation_end() 
> call. Actually is this complexity really worth it for the failure case?
> 
> AFAICS there is a single call site for __tdx_hypercall_failed so why 
> noot call panic() directly ?

W/o this patch, the __tdx_hypercall_failed() is called from the TDX_HYPERCALL
assembly, which is in .noinstr.text, and 'instrumentation_begin()' was needed to
avoid the build warning I suppose.

However now with this patch __tdx_hypercall_failed() is called from
__tdx_hypercall() which is a C function w/o 'noinstr' annotation, thus I believe
instrumentation_begin() and 'noinstr' annotation are not needed anymore.

I didn't notice this while moving this function around and my kernel build test
didn't warn me about this.  I'll change in next version.

In fact, perhaps this patch perhaps is too big for review.  I will also try to
split it to smaller ones.

> 
> > +
> > +static inline u64 __tdx_hypercall(struct tdx_module_args *args)
> > +{
> > +	u64 ret;
> > +
> > +	args->rcx = TDVMCALL_EXPOSE_REGS_MASK;
> > +	ret = __tdcall_saved_ret(TDG_VP_VMCALL, args);
> > +
> > +	/*
> > +	 * RAX!=0 indicates a failure of the TDVMCALL mechanism itself and that
> 
> nit: Why mention the register explicitly, just say that if 
> __tdcall_saved_ret returns non-zero ....

OK will do.  I basically moved the comment from assembly to here.