Message-ID: <65d9322a53e90_2509b29456@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Fri, 23 Feb 2024 16:02:50 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Tom Lendacky <thomas.lendacky@....com>, Dan Williams
	<dan.j.williams@...el.com>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>
CC: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, "Peter
 Zijlstra" <peterz@...radead.org>, Michael Roth <michael.roth@....com>, Ashish
 Kalra <ashish.kalra@....com>
Subject: Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support
 for an SVSM

Tom Lendacky wrote:
> On 2/12/24 20:34, Dan Williams wrote:
> > Tom Lendacky wrote:
> >> On 2/2/24 01:10, Dan Williams wrote:
> >>> Tom Lendacky wrote:
> >>>> When an SVSM is present, the guest can also request attestation reports
> >>>> from the SVSM. These SVSM attestation reports can be used to attest the
> >>>> SVSM and any services running within the SVSM.
> >>>>
> >>>> Extend the config-fs attestation support to allow for an SVSM attestation
> >>>> report. This involves creating four (4) new config-fs attributes:
> >>>>
> >>>>     - 'svsm' (input)
> >>>>       This attribute is used to determine whether the attestation request
> >>>>       should be sent to the SVSM or to the SEV firmware.
> >>>>
> >>>>     - 'service_guid' (input)
> >>>>       Used for requesting the attestation of a single service within the
> >>>>       SVSM. A null GUID implies that the SVSM_ATTEST_SERVICES call should
> >>>>       be used to request the attestation report. A non-null GUID implies
> >>>>       that the SVSM_ATTEST_SINGLE_SERVICE call should be used.
> >>>>
> >>>>     - 'service_version' (input)
> >>>>       Used with the SVSM_ATTEST_SINGLE_SERVICE call, the service version
> >>>>       represents a specific service manifest version be used for the
> >>>>       attestation report.
> >>>>
> >>>>     - 'manifestblob' (output)
> >>>>       Used to return the service manifest associated with the attestation
> >>>>       report.
> >>>>
> >>>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
> >>>> ---
> >>>>    Documentation/ABI/testing/configfs-tsm  |  55 ++++++++++
> >>>>    arch/x86/include/asm/sev.h              |  31 +++++-
> >>>>    arch/x86/kernel/sev.c                   |  50 +++++++++
> >>>>    drivers/virt/coco/sev-guest/sev-guest.c | 137 ++++++++++++++++++++++++
> >>>>    drivers/virt/coco/tsm.c                 |  95 +++++++++++++++-
> >>>>    include/linux/tsm.h                     |  11 ++
> >>>>    6 files changed, 376 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
> >>>> index dd24202b5ba5..c5423987d323 100644
> >>>> --- a/Documentation/ABI/testing/configfs-tsm
> >>>> +++ b/Documentation/ABI/testing/configfs-tsm
> >>>> @@ -31,6 +31,21 @@ Description:
> >>>>    		Standardization v2.03 Section 4.1.8.1 MSG_REPORT_REQ.
> >>>>    		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf
> >>>>    
> >>>> +What:		/sys/kernel/config/tsm/report/$name/manifestblob
> >>>> +Date:		January, 2024
> >>>> +KernelVersion:	v6.9
> >>>> +Contact:	linux-coco@...ts.linux.dev
> >>>> +Description:
> >>>> +		(RO) Optional supplemental data that a TSM may emit, visibility
> >>>> +		of this attribute depends on TSM, and may be empty if no
> >>>> +		manifest data is available.
> >>>> +
> >>>> +		When @provider is "sev_guest" and the "svsm" attribute is set
> >>>> +		this file contains the service manifest used for the SVSM
> >>>> +		attestation report from Secure VM Service Module for SEV-SNP
> >>>> +		Guests v1.00 Section 7.
> >>>> +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
> >>>
> >>> I wish configfs had better dynamic visibility so that this could be
> >>> hidden when not active... oh well.
> >>
> >> So do I. I played with the idea of having two different structs and
> >> registering one or the other based on whether an SVSM was present. Thoughts?
> > 
> > That's the status quo for the differences between TDX vs SEV
> > (tsm_report_default_type vs tsm_report_extra_type). I think a
> > "tsm_report_service_type " can be a new superset of
> > tsm_report_extra_type. That that just starts to get messy if
> > implementations start to pick and choose on a finer granularity what
> > they support. For example, what if TDX supports these new service
> > attributes, but not privlevel.
> > 
> > It seems straightforward to add an is_visible() callback to
> > 'struct configfs_item_operations'. Then a common superset of all the
> > attributes could be specified, but with an is_visible() implementation
> > that consults with data registered with tsm_register() rather than the
> > @type argument directly.
> 
> I've been playing with this a bit.
> 
> For the configfs support I was thinking of doing a per attribute 
> is_visible() callback field since the TSM support is currently all in one 
> group. The callback field would be checked in fs/configfs/dir.c 
> populate_attrs(). A simple bool return value should suffice, I'm not sure 
> we need to get into umode changes. If the field is NULL, then the 
> attribute is displayed. If non-NULL, it depends on the callback return value.
> 
> In order to keep tsm.c as vendor neutral as possible, a way to 
> provide/override a default callback would be needed. The new SVSM related 
> fields would have a callback assigned that always returns false by 
> default, so that these attributes wouldn't be visible. A new tsm.c 
> interface that takes the attribute name and a callback function can be 
> used to override the requested attribute. For example, the SEV guest 
> driver could register a callback for these attributes that returns true 
> when running under an SVSM or false otherwise. Or it could leave the 
> default in place and register a callback when running under an SVSM that 
> always returns true.
> 
> Thoughts?

Sounds like a patch I want to see, yes. So the idea is the low-level
driver registers the is_visible() callback to the core and that gets to
filter attributes?

Hmm, as long as it ends up looking similar to sysfs is_visible()
prototype.

It could even just be a bitmask of attributes that gets passed in by the
provider, something like:

static struct configfs_attribute *tsm_report_attrs[] = {
        [TSM_REPORT_ATTR_GENERATION] = &tsm_report_attr_generation,
        [TSM_REPORT_ATTR_PROVIDER] = &tsm_report_attr_provider
        [TSM_REPORT_ATTR_PRIVLEVEL] = &tsm_report_attr_privlevel,
        [TSM_REPORT_ATTR_FLOOR] = &tsm_report_attr_privlevel_floor,
        NULL,
};

bool tsm_report_is_visible(struct config_item *cfg, struct configfs_attribute *attr, int n)
{
	return test_bit(n, &provider.attr_mask);
}

..and in is_bin_visible() for the binary attributes?
 

Powered by blists - more mailing lists