Message-ID: <ZQr3bTtq6N+myZ4u@redhat.com>
Date:   Wed, 20 Sep 2023 14:45:17 +0100
From:   Daniel P. Berrangé <berrange@...hat.com>
To:     "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:     Kuppuswamy Sathyanarayanan 
        <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>,
        Dan Williams <dan.j.williams@...el.com>,
        "H . Peter Anvin" <hpa@...or.com>, Tony Luck <tony.luck@...el.com>,
        Wander Lairson Costa <wander@...hat.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Dionna Amalie Glaze <dionnaglaze@...gle.com>,
        Qinkun Bao <qinkun@...che.org>,
        Guorui Yu <GuoRui.Yu@...ux.alibaba.com>,
        linux-coco@...ts.linux.dev, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] virt: tdx-guest: Add Quote generation support using
 TSM_REPORTS

On Wed, Sep 20, 2023 at 04:16:33PM +0300, Kirill A . Shutemov wrote:
> On Thu, Sep 14, 2023 at 03:13:49AM +0000, Kuppuswamy Sathyanarayanan wrote:
> > In TDX guest, the attestation process is used to verify the TDX guest
> > trustworthiness to other entities before provisioning secrets to the
> > guest. The First step in the attestation process is TDREPORT
> 
> s/First/first/ ?
> 
> > generation, which involves getting the guest measurement data in the
> > format of TDREPORT, which is further used to validate the authenticity
> > of the TDX guest. TDREPORT by design is integrity-protected and can
> > only be verified on the local machine.
> > 
> > To support remote verification of the TDREPORT (in a SGX-based
> > attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
> 
> Parentheses can be dropped.
> 
> > (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > only run outside of the TDX guest (i.e. in a host process or in a
> > normal VM) and guest can use communication channels like vsock or
> > TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > TDX guest may not support these communication channels. To handle such
> > cases, TDX defines a GetQuote hypercall which can be used by the guest
> > to request the host VMM to communicate with the SGX QE. More details
> > about GetQuote hypercall can be found in TDX Guest-Host Communication
> > Interface (GHCI) for Intel TDX 1.0, section titled
> > "TDG.VP.VMCALL<GetQuote>".
> > 
> > Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > Computing Guest platforms to get the measurement data via ConfigFS.
> > Extend the TSM framework and add support to allow an attestation agent
> > to get the TDX Quote data (included usage example below).
> > 
> >   report=/sys/kernel/config/tsm/report/report0
> >   mkdir $report
> >   dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >   hexdump -C $report/outblob
> >   rmdir $report
> > 
> > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > with TDREPORT data as input, which is further used by the VMM to copy
> > the TD Quote result after successful Quote generation. To create the
> > shared buffer, allocate a large enough memory and mark it shared using
> > set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > for GetQuote requests in the TDX TSM handler.
> > 
> > Although this method reserves a fixed chunk of memory for GetQuote
> > requests, such one time allocation can help avoid memory fragmentation
> > related allocation failures later in the uptime of the guest.
> > 
> > Since the Quote generation process is not time-critical or frequently
> > used, the current version uses a polling model for Quote requests and
> > it also does not support parallel GetQuote requests.
> > 
> > Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> 
> The patch looks good to me. See one question below.

> > +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> > +{
> > +	struct tdx_quote_buf *quote_buf = quote_data;
> > +	int ret;
> > +	u8 *buf;
> > +	u64 err;
> > +
> > +	if (mutex_lock_interruptible(&quote_lock))
> > +		return ERR_PTR(-EINTR);
> > +
> > +	/*
> > +	 * If the previous request is timedout or interrupted, and the
> > +	 * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
> > +	 * VMM), don't permit any new request.
> > +	 */
> > +	if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
> > +		ret = -EBUSY;
> > +		goto done;
> > +	}
> > +
> > +	if (desc->inblob_len != TDX_REPORTDATA_LEN) {
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> > +	/* TDX attestation only supports default format request */
> > +	if (desc->outblob_format != TSM_FORMAT_DEFAULT) {
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> > +	u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> 
> __free() is new to me. Good to know.
> 
> But are we okay now with declaring variables in the middle of the
> function? Any reason we can't do at the top?

I expect that to be unsafe and result in uninitialized data from the
stack being passed to kfree, if a "goto done" in the lines prior to
this declaration is triggered.

The 'reportdata' variable will be in scope at the "done:" label,
thus triggering the __free callback, but the 'kmalloc' initializer
will not have executed.

Variables should always be declared prior to any 'goto' statement
within the same block of scope, if relying on __attribute__((cleanup))
callbacks.

> 
> > +	if (!reportdata) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
> > +	if (!tdreport) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	memcpy(reportdata, desc->inblob, desc->inblob_len);
> > +
> > +	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
> > +	ret = tdx_mcall_get_report0(reportdata, tdreport);
> > +	if (ret) {
> > +		pr_err("GetReport call failed\n");
> > +		goto done;
> > +	}
> > +
> > +	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> > +
> > +	/* Update Quote buffer header */
> > +	quote_buf->version = GET_QUOTE_CMD_VER;
> > +	quote_buf->in_len = TDX_REPORT_LEN;
> > +
> > +	memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
> > +
> > +	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> > +	if (err) {
> > +		pr_err("GetQuote hypercall failed, status:%llx\n", err);
> > +		ret = -EIO;
> > +		goto done;
> > +	}
> > +
> > +	ret = wait_for_quote_completion(quote_buf, getquote_timeout);
> > +	if (ret) {
> > +		pr_err("GetQuote request timedout\n");
> > +		goto done;
> > +	}
> > +
> > +	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
> > +	if (!buf) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	*outblob_len = quote_buf->out_len;
> > +
> > +done:
> > +	mutex_unlock(&quote_lock);
> > +	return ret ? ERR_PTR(ret) : buf;
> > +}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Powered by blists - more mailing lists