linux-kernel - Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}

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: <64dbf0d629d90_47b572942@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date:   Tue, 15 Aug 2023 14:40:38 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Tom Lendacky <thomas.lendacky@....com>,
        Dan Williams <dan.j.williams@...el.com>,
        <linux-coco@...ts.linux.dev>
CC:     Borislav Petkov <bp@...en8.de>,
        Dionna Glaze <dionnaglaze@...gle.com>,
        Brijesh Singh <brijesh.singh@....com>, <peterz@...radead.org>,
        <x86@...nel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for
 SNP_{GET, GET_EXT}_REPORT

Tom Lendacky wrote:
> On 8/14/23 02:43, Dan Williams wrote:
> > The sevguest driver was a first mover in the confidential computing
> > space. As a first mover that afforded some leeway to build the driver
> > without concern for common infrastructure.
> > 
> > Now that sevguest is no longer a singleton [1] the common operation of
> > building and transmitting attestation report blobs can / should be made
> > common. In this model the so called "TSM-provider" implementations can
> > share a common envelope ABI even if the contents of that envelope remain
> > vendor-specific. When / if the industry agrees on an attestation record
> > format, that definition can also fit in the same ABI. In the meantime
> > the kernel's maintenance burden is reduced and collaboration on the
> > commons is increased.
> > 
> > Convert sevguest to use CONFIG_TSM_REPORTS to retrieve the blobs that
> > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for
> > retrieving the SNP_GET_REPORT blob via the TSM interface utility,
> > assuming no nonce and VMPL==2:
> > 
> >      echo 2 > /sys/class/tsm/tsm0/privlevel
> >      dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex
> >      hexdump -C /sys/class/tsm/tsm0/outblob
> > 
> > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the
> > format to "extended":
> > 
> >      echo 2 > /sys/class/tsm/tsm0/privlevel
> >      echo extended > /sys/class/tsm/tsm0/format
> >      dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex
> >      hexdump -C /sys/class/tsm/tsm0/outblob
> > 
> > The old ioctls can be lazily deprecated, the main motivation of this
> > effort is to stop the proliferation of new ioctls, and to increase
> > cross-vendor colloboration.
> > 
> > Note, only compile-tested.
> 
> I just got back from vacation, so I'll apply and test as soon as I get a 

Appreciate it! Hold off on testing until v3 though since Peter
highlighted I am misusing no_free_ptr(), Jeremi pointed out that
sev-guest locking is being violated, and configfs may need to be
deployed for this to future proof the ABI for future use cases.

> chance.
> 
> > 
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@...en8.de>
> > Cc: Tom Lendacky <thomas.lendacky@....com>
> > Cc: Dionna Glaze <dionnaglaze@...gle.com>
> > Cc: Brijesh Singh <brijesh.singh@....com>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> > ---
> >   drivers/virt/coco/sev-guest/Kconfig     |    1
> >   drivers/virt/coco/sev-guest/sev-guest.c |   81 +++++++++++++++++++++++++++++++
> >   2 files changed, 82 insertions(+)
> > 
> > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> > index da2d7ca531f0..1cffc72c41cb 100644
> > --- a/drivers/virt/coco/sev-guest/Kconfig
> > +++ b/drivers/virt/coco/sev-guest/Kconfig
> > @@ -5,6 +5,7 @@ config SEV_GUEST
> >   	select CRYPTO
> >   	select CRYPTO_AEAD2
> >   	select CRYPTO_GCM
> > +	select TSM_REPORTS
> >   	help
> >   	  SEV-SNP firmware provides the guest a mechanism to communicate with
> >   	  the PSP without risk from a malicious hypervisor who wishes to read,
> > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > index f48c4764a7a2..5941081502e8 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/miscdevice.h>
> >   #include <linux/set_memory.h>
> >   #include <linux/fs.h>
> > +#include <linux/tsm.h>
> >   #include <crypto/aead.h>
> >   #include <linux/scatterlist.h>
> >   #include <linux/psp-sev.h>
> > @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> >   	return key;
> >   }
> >   
> > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc,
> > +			  size_t *outblob_len)
> > +{
> > +	struct snp_guest_dev *snp_dev = dev_get_drvdata(dev);
> > +	const int report_size = SZ_16K;
> 
> The response buffer from the PSP is limited to 4K, so the report size can 
> be SZ_4K.

Oh, ok, what about the extended case?

> 
> > +	const int ext_size = SZ_16K;
> > +	int ret, size;
> > +
> > +	if (desc->inblob_len != 64)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (desc->outblob_format == TSM_FORMAT_EXTENDED)
> > +		size = report_size + ext_size;
> > +	else
> > +		size = report_size;
> > +
> > +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > +
> > +	if (desc->outblob_format == TSM_FORMAT_EXTENDED) {
> > +		struct snp_ext_report_req ext_req = {
> > +			.data = { .vmpl = desc->privlevel },
> > +			.certs_address = (__u64)buf + report_size,
> > +			.certs_len = ext_size,
> > +		};
> > +		memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> > +
> > +		struct snp_guest_request_ioctl input = {
> > +			.msg_version = 1,
> > +			.req_data = (__u64)&ext_req,
> > +			.resp_data = (__u64)buf,
> > +		};
> 
> Won't the compiler complain about this declaration being after the memcpy()?

The memcpy is into @ext_req, @input is just referencing @ext_req.

> 
> > +
> > +		ret = get_ext_report(snp_dev, &input, SNP_KARG);
> > +	} else {
> > +		struct snp_report_req req = {
> > +			.vmpl = desc->privlevel,
> > +		};
> > +		memcpy(&req.user_data, desc->inblob, desc->inblob_len);
> > +
> > +		struct snp_guest_request_ioctl input = {
> > +			.msg_version = 1,
> > +			.req_data = (__u64) &req,
> > +			.resp_data = (__u64) buf,
> > +		};
> 
> Ditto here.
> 

I think its ok, but let me know if you think I am missing something.
Thanks for taking a look!