Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Sep 2023 09:38:11 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>,
        Dan Williams <dan.j.williams@intel.com>,
        <linux-coco@lists.linux.dev>
CC:     Borislav Petkov <bp@alien8.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Dionna Glaze <dionnaglaze@google.com>,
        Brijesh Singh <brijesh.singh@amd.com>, <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>, <tglx@linutronix.de>
Subject: Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for
 SNP_{GET, GET_EXT}_REPORT
Message-ID: <64f2137362305_4c1f3294f4@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com>
 <169342402274.3934343.11704206926819634506.stgit@dwillia2-xfh.jf.intel.com>
 <4e4407b0-179d-4be4-bf0b-0d674103d4f6@linux.microsoft.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4e4407b0-179d-4be4-bf0b-0d674103d4f6@linux.microsoft.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?nxZNdWlf0ueFdYNRogbuSXNRWcxvOUIBZ6X0zzBxmghVR4ogtOSr/na44kiz?=
 =?us-ascii?Q?PcFZdUVUBUaybBN5ovzHMooYzIC/SFn6pSA0g+p2cN16yNJVGbR5e/dBcq2p?=
 =?us-ascii?Q?3NnxvtSuXvPwe5oKgQWFjkdliMoy2NWsQr12B/UVOhwgt+MFdY/giApHFKDQ?=
 =?us-ascii?Q?vTdCb6g2KqJbEqJUeQlbfCjloGnbQaOK/mAUChM6UC3XOoegREBPm7O6eVJJ?=
 =?us-ascii?Q?RNF7MwIzzty5J3rekMZvuSqkyoR69QEIbyyvQgG4OOhCpy46YMnZSY0zSbXo?=
 =?us-ascii?Q?6ZlvngDUMLYvZJZV9wSfYUAvU7/2WfHOCP2IuBQHfxAC/OesUGs5dLbbG3c6?=
 =?us-ascii?Q?6wmn6MvWDzjH4b9ulV/sFuD96CWXN1PRPXtiuRje7UgvEpfjJlWAY6cfpKxb?=
 =?us-ascii?Q?3RGddZYFT3QlcWX3igoYCxO2jI7HiDGqmRd6seRLPNXkYUf2uKjWdDzKTLjD?=
 =?us-ascii?Q?/uBiYCyvawcHJhbKLURLQaqr4TSatzc9WlTsqQoW5sf/5nO3GIW1pKmpxv+K?=
 =?us-ascii?Q?PhIQPcs6NK74VSqd9vC8e4cuSd5xklc4v3hs34m2yT/dmFlo2WhZ172QX9js?=
 =?us-ascii?Q?mJ5vSjTDyMOAGIWuO/svPZjbFFnRDIzl2rzbdgnGvsn0ce98BmcnpsxVnWGV?=
 =?us-ascii?Q?CIajr+hRc8e/IPFxxPkENQZnoDCd7xRyWfqjILMkpyH/9ajuKoozawKr44Fb?=
 =?us-ascii?Q?FhwItkWC3CynBnohDWv/r8Jk5Sqb2sEbA3waUafL/lfFuPTzCM82u27mByYm?=
 =?us-ascii?Q?VKtyNOe3UIDsc7OmXv8Rc4VJr9gLfgQORwII0hLuez4tyrfH+qLaBw8Ncgny?=
 =?us-ascii?Q?pNNLLq6m24vDNTPDSfwvAZD/jtkBeY5520Dt2Zq4OwBOxcTCn/HBrCXV1fB6?=
 =?us-ascii?Q?DaykOS8F9lUWK6/2mEeSekR1RgOE9qtdXsCJ1dYoEDrEe2Kd7FohFbHKNkX4?=
 =?us-ascii?Q?RLB2kko8EEdyoZ83VEzscL+cTYzuIf1H7AMrd2p0NRGkzd5cogms0iVBRNSx?=
 =?us-ascii?Q?SfgLj/L6cHbBhTmf0RK7nWF8VcrO4hnhNBCjYiypI5BWeXlJ0oK+FG/InkyU?=
 =?us-ascii?Q?37fKo15Tjf5hzDe+iStf1+QZu7uKqe4IrMjL7DYzQQhsbhYIU019pHlkCPZA?=
 =?us-ascii?Q?qTSLTSAQuwLEjh3k+GOjyyXf63MBWQQ0OySEZpanIuvb8Hgun2MixKDaFYYI?=
 =?us-ascii?Q?bKFaJtswpEBl8saahvk5Wydb3uHNp59JaeF7oUf7VHMRrciiB63w0GgrwDPC?=
 =?us-ascii?Q?n93Xjnw0EicXwghW8ej8QT4SaHzhEbAC0mIzKv2XF6wAkit/Q5VuoFEFuwUK?=
 =?us-ascii?Q?uCtAB5LAjVY02wH7w/hsDesh7h/t5CLrUFt1IL1gMPZiZKRjH7+62ltP6flo?=
 =?us-ascii?Q?pU3RNcliI3lMS/jh3qD0A8VCjOZVx1ZZzqf26KIJfuaw41lIQZEuQTrmhAp4?=
 =?us-ascii?Q?ObtLDcNvTIlq41XYrmbSQLFw1LR1uPT+aEc59j9xpnFi7WacoCOuoOJZFx74?=
 =?us-ascii?Q?XcxpalDe2rUOWOPSdz1xqwZGODNmRMQ486efOPHt4Swjsaq8nqML6MmvNxYq?=
 =?us-ascii?Q?lnwO+nNu4qNF/vvJcox+IBnKImx7zT/PKOvsm0PmOKKXytDterkRdKKroIjj?=
 =?us-ascii?Q?0w=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c33352de-fb6d-4f20-abc7-08dbab09d67a
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Sep 2023 16:38:14.5952
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JXzj2HpsS/C1hCOnidgMGD7d6IfSIdMDKaPXQCUlk1FGNbxXRn5Pe4fd9iUKmmX0b5vqh/bVXOIvqgo46T9Hiz4sYgqC9bf7T8rati4kkkA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5154
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Jeremi Piotrowski wrote:
> On 8/30/2023 9:33 PM, 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 follows for
> > retrieving the SNP_GET_REPORT blob via the TSM interface utility,
> > assuming no nonce and VMPL==2:
> > 
> >     report=/sys/kernel/config/tsm/report/report0
> >     mkdir $report
> >     echo 2 > $report/privlevel
> >     dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >     hexdump -C $report/outblob
> >     rmdir $report
> > 
> > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the
> > format to "extended":
> > 
> >     report=/sys/kernel/config/tsm/report/report1
> >     mkdir $report
> >     echo extended > $report/format
> >     dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >     hexdump -C $report/outblob
> >     rmdir $report
> > 
> > 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 collaboration.
> > 
> > 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 |   83 +++++++++++++++++++++++++++++++
> >  2 files changed, 84 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 c3c9e9ea691f..c7bbb8f372a3 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -16,10 +16,12 @@
> >  #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>
> >  #include <linux/sockptr.h>
> > +#include <linux/cleanup.h>
> >  #include <uapi/linux/sev-guest.h>
> >  #include <uapi/linux/psp-sev.h>
> >  
> > @@ -759,6 +761,79 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> >  	return key;
> >  }
> >  
> > +static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> > +{
> > +	struct snp_guest_dev *snp_dev = data;
> > +	const int report_size = 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);
> > +
> > +	guard(mutex)(&snp_cmd_mutex);
> > +	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,
> > +		};
> > +		struct snp_req_resp io = {
> > +			.req_data = KERNEL_SOCKPTR(&ext_req),
> > +			.resp_data = KERNEL_SOCKPTR(buf),
> > +		};
> > +
> > +		ret = get_ext_report(snp_dev, &input, &io);
> > +	} 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,
> > +		};
> > +		struct snp_req_resp io = {
> > +			.req_data = KERNEL_SOCKPTR(&req),
> > +			.resp_data = KERNEL_SOCKPTR(buf),
> > +		};
> > +
> > +		ret = get_report(snp_dev, &input, &io);
> > +	}
> > +
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	*outblob_len = size;
> > +	return_ptr(buf);
> > +}
> > +
> > +static const struct tsm_ops sev_tsm_ops = {
> > +	.name = KBUILD_MODNAME,
> > +	.report_new = sev_report_new,
> > +};
> > +
> > +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;
> > @@ -832,6 +907,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, snp_dev, &tsm_report_ext_type);
> > +	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;
> > 
> 
> I tried this with the non-extended request

Thanks for testing!

> ...and realized it's a bit awkward from
> a uapi point of view. The returned outblob has a header prepended (table 23 in [1])
> and is arbitrarily sized at 4096. It would be more natural to only return the report
> field and the count bytes that the report actually has. I've attached a rough patch
> below to give you an idea of what I mean.

It makes sense, especially if that is what the legacy ioctl output
buffer is emitting.

> The extended guest request is another topic, since userspace has to be aware of
> where the kernel choses to put the extended data, and fixup all the offsets in the
> table (section 4.1.8.1 in [2]). It would be better to return this data through a
> separate file.

I notice that the TDX report also includes a certificate blob, so if
that is a common concept then yes, it makes sense to have a separate
file for that.