linux-kernel - Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM

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]

Message-ID: <3d02956a10ecab06e634ffdf6820c600f02deb17.camel@intel.com>
Date:   Mon, 11 Sep 2023 10:00:42 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Williams, Dan J" <dan.j.williams@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC:     "Yu, Guorui" <guorui.yu@...ux.alibaba.com>,
        "qinkun@...che.org" <qinkun@...che.org>,
        "wander@...hat.com" <wander@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "dionnaglaze@...gle.com" <dionnaglaze@...gle.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using
 TSM_REPORTS

On Fri, 2023-09-08 at 09:19 -0700, Dan Williams wrote:
> Huang, Kai wrote:
> > 
> > > 
> > > Changes since previous version:
> > > * Used ConfigFS interface instead of IOCTL interface.
> > > * Used polling model for Quote generation and dropped the event notification IRQ support.
> > 
> > Can you elaborate why the notification IRQ is dropped?
> 
> Because it was a pile of hacks and non-idiomatic complexity. It can come
> back when / if driver code can treat it like a typical interrupt.

Thanks.  Agreed.

> 
> [..]
> > >  
> > > +/*
> > > + * Intel's SGX QE implementation generally uses Quote size less
> > > + * than 8K (2K Quote data + ~5K of ceritificate blob).
> > > + */
> > > +#define GET_QUOTE_BUF_SIZE		SZ_8K
> > 
> > SZ_8K is defined in <linux/sizes.h>.  It seems it's not explicitly included. 
> > It's better to explicitly include it.
> > 
> > Btw, although the size of the certificate blob shouldn't change dramatically,
> > the Quote can also include the "QE Authentication Data", which can vary a lot
> > depending on different QE implementation (e.g., containing geography
> > information, etc).
> > 
> > I wish eventually there's some /sysfs entry to configure the size of Quote
> > buffer, but I guess it can be done in the future.
> 
> How would userspace have any idea of how big the quote buffer is to be
> able to set it? The output format at least needs to standardized within
> a given vendor's implementation, and future variation should be
> de-emphasized relative to getting to a common report format across
> vendors.

The Quoting enclave implements what to be included into the Quote.  The admin
who deploys the Quoting enclave knows the Quote size.

> 
> [..]
> > > +/*
> > > + * wait_for_quote_completion() - Wait for Quote request completion
> > > + * @quote_buf: Address of Quote buffer.
> > > + * @timeout: Timeout in seconds to wait for the Quote generation.
> > > + *
> > > + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> > > + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> > > + * while VMM processes the GetQuote request, and will change it to success
> > > + * or error code after processing is complete. So wait till the status
> > > + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> > > + */
> > > +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> > > +{
> > > +	int i = 0;
> > > +
> > > +	/*
> > > +	 * Quote requests usually take a few seconds to complete, so waking up
> > > +	 * once per second to recheck the status is fine for this use case.
> > > +	 */
> > > +	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> > > +		ssleep(1);
> > > +
> > > +	return (i == timeout) ? -ETIMEDOUT : 0;
> > > +}
> > 
> > Why can't we use wait_for_completion_timeout() provided by the kernel?
> > 
> > Btw, can we use _killable() variant?  Supporting timeout brings extra
> > complication, and I think supporting timeout isn't mandatory for the first
> > version.
> 
> It definitely is required even if interrupts were supported. The kernel
> needs to give up on stalled operations in a reasonable amount of time.

Thanks for explaining.