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.

Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.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;
+	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,
+		};
+
+		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,
+		};
+
+		ret = get_report(snp_dev, &input, SNP_KARG);
+	}
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	*outblob_len = size;
+	no_free_ptr(buf);
+	return buf;
+}
+
+static const struct tsm_ops sev_tsm_ops = {
+	.name = KBUILD_MODNAME,
+	.report_new = sev_report_new,
+};
+
+static const struct attribute_group *sev_tsm_attribute_groups[] = {
+	&tsm_default_attribute_group,
+	&tsm_extra_attribute_group,
+	NULL,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+	unregister_tsm(&sev_tsm_ops);
+}
+
 static int __init sev_guest_probe(struct platform_device *pdev)
 {
 	struct snp_secrets_page_layout *layout;
@@ -842,6 +915,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 	snp_dev->input.resp_gpa = __pa(snp_dev->response);
 	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
 
+	ret = register_tsm(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups);
+	if (ret)
+		goto e_free_cert_data;
+
+	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
+	if (ret)
+		goto e_free_cert_data;
+
 	ret =  misc_register(misc);
 	if (ret)
 		goto e_free_cert_data;

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 
chance.

> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.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.

> +	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()?

> +
> +		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.

Thanks,
Tom

> +
> +		ret = get_report(snp_dev, &input, SNP_KARG);
> +	}
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	*outblob_len = size;
> +	no_free_ptr(buf);
> +	return buf;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = sev_report_new,
> +};
> +
> +static const struct attribute_group *sev_tsm_attribute_groups[] = {
> +	&tsm_default_attribute_group,
> +	&tsm_extra_attribute_group,
> +	NULL,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> +	unregister_tsm(&sev_tsm_ops);
> +}
> +
>   static int __init sev_guest_probe(struct platform_device *pdev)
>   {
>   	struct snp_secrets_page_layout *layout;
> @@ -842,6 +915,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	snp_dev->input.resp_gpa = __pa(snp_dev->response);
>   	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>   
> +	ret = register_tsm(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups);
> +	if (ret)
> +		goto e_free_cert_data;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> +	if (ret)
> +		goto e_free_cert_data;
> +
>   	ret =  misc_register(misc);
>   	if (ret)
>   		goto e_free_cert_data;
>

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@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.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!

On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote:
> +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc,

> +			  size_t *outblob_len)
> +{

> +
> +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> +

> +
> +	*outblob_len = size;
> +	no_free_ptr(buf);
> +	return buf;

This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no
atomics). So the above would end up being:

	return NULL;

What you want to write is somehting like:

	return no_free_ptr(buf);

or, a convenient shorthand:

	return_ptr(buf);

Peter Zijlstra wrote:
> On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote:
> > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc,
> 
> > +			  size_t *outblob_len)
> > +{
> 
> > +
> > +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > +
> 
> > +
> > +	*outblob_len = size;
> > +	no_free_ptr(buf);
> > +	return buf;
> 
> This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no
> atomics). So the above would end up being:
> 
> 	return NULL;
> 
> What you want to write is somehting like:
> 
> 	return no_free_ptr(buf);
> 
> or, a convenient shorthand:
> 
> 	return_ptr(buf);
> 

Oh, I indeed did not realize that no_free_ptr() had side effects beyond
canceling the free when the variable goes out of scope. Will switch to
return_ptr().

On Mon, Aug 14, 2023 at 09:25:06AM -0700, Dan Williams wrote:
> Peter Zijlstra wrote:
> > On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote:
> > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc,
> > 
> > > +			  size_t *outblob_len)
> > > +{
> > 
> > > +
> > > +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > > +
> > 
> > > +
> > > +	*outblob_len = size;
> > > +	no_free_ptr(buf);
> > > +	return buf;
> > 
> > This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no
> > atomics). So the above would end up being:
> > 
> > 	return NULL;
> > 
> > What you want to write is somehting like:
> > 
> > 	return no_free_ptr(buf);
> > 
> > or, a convenient shorthand:
> > 
> > 	return_ptr(buf);
> > 
> 
> Oh, I indeed did not realize that no_free_ptr() had side effects beyond
> canceling the free when the variable goes out of scope. Will switch to
> return_ptr().

Indeed -- ideally no_free_ptr() would be combined with __must_check, but
I'm not immediately sure how to pull that off. Let me stick that on the
to-do list.

On Mon, Aug 14, 2023 at 06:48:04PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 14, 2023 at 09:25:06AM -0700, Dan Williams wrote:
> > Peter Zijlstra wrote:
> > > On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote:
> > > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc,
> > > 
> > > > +			  size_t *outblob_len)
> > > > +{
> > > 
> > > > +
> > > > +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > > > +
> > > 
> > > > +
> > > > +	*outblob_len = size;
> > > > +	no_free_ptr(buf);
> > > > +	return buf;
> > > 
> > > This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no
> > > atomics). So the above would end up being:
> > > 
> > > 	return NULL;
> > > 
> > > What you want to write is somehting like:
> > > 
> > > 	return no_free_ptr(buf);
> > > 
> > > or, a convenient shorthand:
> > > 
> > > 	return_ptr(buf);
> > > 
> > 
> > Oh, I indeed did not realize that no_free_ptr() had side effects beyond
> > canceling the free when the variable goes out of scope. Will switch to
> > return_ptr().
> 
> Indeed -- ideally no_free_ptr() would be combined with __must_check, but
> I'm not immediately sure how to pull that off. Let me stick that on the
> to-do list.

This seems to actually work. I'll try and write some coherent comments
tomorrow -- it's definitely too late for that now.

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 53f1a7a932b0..162052cef4c7 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -39,8 +39,12 @@
 
 #define __free(_name)	__cleanup(__free_##_name)
 
+static inline __must_check void * __no_free_ptr(void **pp)
+{ void *p = *pp; *pp = NULL; return p; }
+
 #define no_free_ptr(p) \
-	({ __auto_type __ptr = (p); (p) = NULL; __ptr; })
+	((void)__builtin_types_compatible_p(void *, typeof(p)), \
+	 ((typeof(p))__no_free_ptr((void **)&(p))))
 
 #define return_ptr(p)	return no_free_ptr(p)

On Tue, Aug 15, 2023 at 12:15:03AM +0200, Peter Zijlstra wrote:

> This seems to actually work. I'll try and write some coherent comments
> tomorrow -- it's definitely too late for that now.

Clearly I should have gone to bed before sending this patch, not after.
Let me try that again.

On 8/14/2023 9:43 AM, 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.
> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.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)

get_report and get_ext_report both have a:

    lockdep_assert_held(&snp_cmd_mutex);

so you'd need to take that lock somewhere here.

Jeremi

> +{
> +	struct snp_guest_dev *snp_dev = dev_get_drvdata(dev);
> +	const int report_size = SZ_16K;
> +	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,
> +		};
> +
> +		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,
> +		};
> +
> +		ret = get_report(snp_dev, &input, SNP_KARG);
> +	}
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	*outblob_len = size;
> +	no_free_ptr(buf);
> +	return buf;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = sev_report_new,
> +};
> +
> +static const struct attribute_group *sev_tsm_attribute_groups[] = {
> +	&tsm_default_attribute_group,
> +	&tsm_extra_attribute_group,
> +	NULL,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> +	unregister_tsm(&sev_tsm_ops);
> +}
> +
>  static int __init sev_guest_probe(struct platform_device *pdev)
>  {
>  	struct snp_secrets_page_layout *layout;
> @@ -842,6 +915,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>  	snp_dev->input.resp_gpa = __pa(snp_dev->response);
>  	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>  
> +	ret = register_tsm(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups);
> +	if (ret)
> +		goto e_free_cert_data;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> +	if (ret)
> +		goto e_free_cert_data;
> +
>  	ret =  misc_register(misc);
>  	if (ret)
>  		goto e_free_cert_data;
>

Jeremi Piotrowski wrote:
> On 8/14/2023 9:43 AM, 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.
> > 
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.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)
> 
> get_report and get_ext_report both have a:
> 
>     lockdep_assert_held(&snp_cmd_mutex);
> 
> so you'd need to take that lock somewhere here.

Got it, and in general this needs testing from someone with actual
hardware.