Re: [PATCH v3 11/14] x86/sev: Extend the config-fs attestation support for an SVSM

[Dan Williams]

Tom Lendacky wrote:
> On 4/16/24 00:37, 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:
> >>
> >>    - 'service-provider' (input)
> >>      This attribute is used to determine whether the attestation request
> >>      should be sent to the specified service provider or to the SEV
> >>      firmware. The SVSM service provider is represented by the value
> >>      'svsm'.
> >>
> >>    - 'service_guid' (input)
> >>      Used for requesting the attestation of a single service within the
> >>      service provider. 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_manifest_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@xxxxxxx>
[..]
> >> +What:		/sys/kernel/config/tsm/report/$name/service_provider
> >> +Date:		January, 2024
> >> +KernelVersion:	v6.10
> >> +Contact:	linux-coco@xxxxxxxxxxxxxxx
> >> +Description:
> >> +		(WO) Attribute is visible if a TSM implementation provider
> >> +		supports the concept of attestation reports from a service
> >> +		provider for TVMs, like SEV-SNP running under an SVSM.
> >> +		Specifying the service provider via this attribute will create
> >> +		an attestation report as specified by the service provider.
> >> +		Currently supported service-providers are:
> >> +			svsm
> >> +
> >> +		For the SVSM service provider, see the 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
> > 
> > Given "SVSM" is a cross vendor concept should this explicitly
> > callout: "For provider.service_provider == sev_guest.svsm" as
> > preparation for other implementations defining their "svsm" manifest
> > format?
> 
> I'm not sure. Do we need to get that specific? If SVSM is cross vendor, 
> will it be using / adding to the existing SVSM specification? If not, 
> then each vendor is likely to have its own name for the SVSM concept 
> that would be unique enough...

Yeah, I guess we can kick can that down the road and see what other
vendors do. I know the coconut-svsm project is cross vendor, but I do
not know the details.

> >> +
> >> +What:		/sys/kernel/config/tsm/report/$name/service_manifest_version
> >> +Date:		January, 2024
> >> +KernelVersion:	v6.10
> >> +Contact:	linux-coco@xxxxxxxxxxxxxxx
> >> +Description:
> >> +		(WO) Attribute is visible if a TSM implementation provider
> >> +		supports the concept of attestation reports from a service
> >> +		provider for TVMs, like SEV-SNP running under an SVSM.
> >> +		Indicates the service manifest version requested for the
> >> +		attestation report. If this field is not set by the user,
> >> +		the default manifest version of the service (the service's
> >> +		initial/first manifest version) is returned. The initial
> >> +		manifest version is always available.
> > 
> > ...and that number is zero? Is there any expectation that the kernel
> 
> Yes, that number is zero.
> 
> > sanity checks this version, or how does the user figure out they need to
> > roll this request back?
> 
> Right now there aren't any non-zero versions, so there is nothing for 
> the user to figure out. However, the service will determine when it 
> creates a new version and then the user will need to understand what the 
> reason for that is and decide. I'm not sure I can give you a specific 
> answer at this stage, but we need to allow for a change in the manifest 
> without affecting existing users.

Right, but it feels odd that userspace could write UINT_MAX to this
value and the kernel says, "yup, looks like a valid manifest version to
me".

> And since the spec has been approved already, I can't really go back and 
> add a requirement that manifest format always be additive.

Those breaking changes have not arrived yet so still a chance to
influence, no?

The documentation here at least guarantees that the initial manifest
version is always valid, and I expect the spec authors to realize that
the kernel has a role to play in not breaking existing userspace. I.e.
it is not in the spec's interest to disallow fallback to any
version between 0 and N-1 where N is the latest.

The kernel need not tolerate version induced breakage from ACPI
specification updates because that spec is committed to compatibility,
why does this spec need to be less disciplined than that?

> >> +
> >> +		For the SVSM service provider, see the 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
> > 
> 
> >> +static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
> >> +						 const char *buf, size_t len)
> >> +{
> >> +	struct tsm_report *report = to_tsm_report(cfg);
> >> +	size_t sp_len;
> >> +	char *sp;
> >> +	int rc;
> >> +
> >> +	guard(rwsem_write)(&tsm_rwsem);
> >> +	rc = try_advance_write_generation(report);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	sp_len = (buf[len - 1] != '\n') ? len : len - 1;
> > 
> > This feels like it wants a sysfs_strdup().
> 
> If there start to be more string oriented operations in the file, then 
> it might be worth it. For now I don't really see a reason.

To be clear I was thinking of occurrences of this patten outside of this
file. Turns out this pattern is not as prevalent as I would have
guessed, resource_alignment_store() was the only occurrence I could
find. Skip for now works for me.