linux-kernel - Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall

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:   Tue, 19 Apr 2022 15:51:26 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>
Cc:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

On Mon, 2022-04-18 at 20:37 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/18/22 7:29 PM, Kai Huang wrote:
> > On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > > In TDX guest, attestation is mainly used to verify the trustworthiness
> > > of a TD to the 3rd party key servers.
> > > 
> > 
> > "key servers" is only a use case of using the attestation service. This sentence
> > looks not accurate.
> 
> I thought it is mainly used for this use case. If it is not accurate,
> how about following?
> 
> Attestation is used to verify the trustworthiness of a TD to the other
> 3rd party entities (like key servers) before exchanging sensitive
> information.

Fine to me, although not sure whether you need to mention key servers.  We Intel
guys has some first impression of what does "key servers" mean mainly because we
defined some use cases around here using attestation.  However for other people
"key servers" can be very generic and may not be the case we defined.

> 
> > 
> > > First step in attestation process
> > > is to get the TDREPORT data and the generated data is further used in
> > > subsequent steps of the attestation process. TDREPORT data contains
> > > details like TDX module version information, measurement of the TD,
> > > along with a TD-specified nonce
> > > 
> > > Add a wrapper function (tdx_mcall_tdreport()) to get the TDREPORT from
> > > the TDX Module.
> > > 
> > > More details about the TDREPORT TDCALL can be found in TDX Guest-Host
> > > Communication Interface (GHCI) for Intel TDX 1.5, section titled
> > > "TDCALL [MR.REPORT]".
> > 
> > Attestation is a must for TDX architecture, so The TDCALL[MR.REPORT] is
> > available in TDX 1.0.  I don't think we should use TDX 1.5 here.  And this
> 
> Yes. It is also part of v1.0. Since the feature is similar between v1.0
> and v1.5, I have included one link. If v1.0 reference is preferred, I
> will update it.

I think we should use 1.0.  Attestation is a essential part for TDX, which means
it must be included in TDX1.0, therefore it doesn't make sense to use TDX1.5 to
reference it.

[...]

> > > +/*
> > > + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> > > + *
> > > + * @data        : Address of 1024B aligned data to store
> > > + *                TDREPORT_STRUCT.
> > > + * @reportdata  : Address of 64B aligned report data
> > > + *
> > > + * return 0 on success or failure error number.
> > > + */
> > > +long tdx_mcall_tdreport(void *data, void *reportdata)
> > > +{
> > > +	u64 ret;
> > > +
> > > +	/*
> > > +	 * Check for a valid TDX guest to ensure this API is only
> > > +	 * used by TDX guest platform. Also make sure "data" and
> > > +	 * "reportdata" pointers are valid.
> > > +	 */
> > > +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > > +		return -EINVAL;
> > 
> > Do we need to manually check the alignment since it is mentioned in the comment
> > of this function?
> 
> Users are responsible to allocate aligned data. I don't think we need
> to add a check for it. If it is unaligned, TDCALL will return error.

Actually this is the kernel memory, but not user memory.  Otherwise
virt_to_phys() doesn't make sense.  You copied the user data to kernel memory in
the last patch.  So whether user memory is aligned doesn't matter, and in last
patch, you have guaranteed the alignment is met during kernel memory allocation.

Anyway like you said the TDCALL will fail if alignment doesn't meet, so I guess
is fine.