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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 2/23/24 18:02, Dan Williams wrote:
> 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@amd.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@lists.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?

I was thinking something along the lines of the following for the configfs
support:

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 18677cd4e62f..c224060c1b6b 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -589,12 +589,20 @@ static int populate_attrs(struct config_item *item)
  		return -EINVAL;
  	if (t->ct_attrs) {
  		for (i = 0; (attr = t->ct_attrs[i]) != NULL; i++) {
+			if (attr->ca_is_visible && !attr->ca_is_visible(attr))
+				continue;
+
  			if ((error = configfs_create_file(item, attr)))
  				break;
  		}
  	}
-	if (t->ct_bin_attrs) {
+	if (t->ct_bin_attrs && !error) {
  		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
+			attr = &bin_attr->cb_attr;
+
+			if (attr->ca_is_visible && !attr->ca_is_visible(attr))
+				continue;
+
  			error = configfs_create_bin_file(item, bin_attr);
  			if (error)
  				break;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2606711adb18..93d346e2afc1 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -112,39 +112,63 @@ static inline void configfs_add_default_group(struct config_group *new_group,
  	list_add_tail(&new_group->group_entry, &group->default_groups);
  }
  
+typedef bool (*configfs_is_visible_t)(const struct configfs_attribute *attr);
+
  struct configfs_attribute {
  	const char		*ca_name;
  	struct module 		*ca_owner;
  	umode_t			ca_mode;
+	configfs_is_visible_t	ca_is_visible;
  	ssize_t (*show)(struct config_item *, char *);
  	ssize_t (*store)(struct config_item *, const char *, size_t);
  };
  
-#define CONFIGFS_ATTR(_pfx, _name)			\
+#define __CONFIGFS_ATTR(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IRUGO | S_IWUSR,		\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.show		= _pfx##_name##_show,		\
  	.store		= _pfx##_name##_store,		\
  }
  
-#define CONFIGFS_ATTR_RO(_pfx, _name)			\
+#define __CONFIGFS_ATTR_RO(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IRUGO,			\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.show		= _pfx##_name##_show,		\
  }
  
-#define CONFIGFS_ATTR_WO(_pfx, _name)			\
+#define __CONFIGFS_ATTR_WO(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IWUSR,			\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.store		= _pfx##_name##_store,		\
  }
  
+#define CONFIGFS_ATTR(_pfx, _name)			\
+	__CONFIGFS_ATTR(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_RO(_pfx, _name)			\
+	__CONFIGFS_ATTR_RO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_WO(_pfx, _name)			\
+	__CONFIGFS_ATTR_WO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_VISIBLE(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_RO(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR_RO(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_WO(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR_WO(_pfx, _name, _vis)
+
  struct file;
  struct vm_area_struct;
  
@@ -156,43 +180,64 @@ struct configfs_bin_attribute {
  	ssize_t (*write)(struct config_item *, const void *, size_t);
  };
  
-#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz)		\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IRUGO | S_IWUSR,		\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.read		= _pfx##_name##_read,			\
-	.write		= _pfx##_name##_write,			\
+#define __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)		\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IRUGO | S_IWUSR,			\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.read		= _pfx##_name##_read,				\
+	.write		= _pfx##_name##_write,				\
  }
  
-#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz)	\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IRUGO,			\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.read		= _pfx##_name##_read,			\
+#define __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)	\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IRUGO,				\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.read		= _pfx##_name##_read,				\
  }
  
-#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz)	\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IWUSR,			\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.write		= _pfx##_name##_write,			\
+#define __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)	\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IWUSR,				\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.write		= _pfx##_name##_write,				\
  }
  
+#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz)			\
+	__CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz)		\
+	__CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz)		\
+	__CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE(_pfx, _name, _priv, _maxs, _vis)	\
+	__CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_RO(_pfx, _name, _priv, _maxsz, _vis)	\
+	__CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_WO(_pfx, _name, _priv, _maxsz, _vis)	\
+	__CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)
+
  /*
   * If allow_link() exists, the item can symlink(2) out to other
   * items.  If the item is a group, it may support mkdir(2).


And then the following for implementing it for tsm, which allows for as
simple or complicated an is_visible() function as required:

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 4ac62c896670..f29310a4a357 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -1022,6 +1022,11 @@ static int sev_report_new(struct tsm_report *report, void *data)
  	return 0;
  }
  
+static bool svsm_make_visible(const struct configfs_attribute *attr)
+{
+	return true;
+}
+
  static struct tsm_ops sev_tsm_ops = {
  	.name = KBUILD_MODNAME,
  	.report_new = sev_report_new,
@@ -1116,6 +1121,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
  	if (ret)
  		goto e_free_cert_data;
  
+	if (snp_get_vmpl()) {
+		/* Make the SVSM-related attributes visible */
+		tsm_set_visibility("svsm", svsm_make_visible);
+		tsm_set_visibility("service_guid", svsm_make_visible);
+		tsm_set_visibility("service_manifest_version", svsm_make_visible);
+		tsm_set_visibility("manifestblob", svsm_make_visible);
+	}
+
  	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
  	if (ret)
  		goto e_free_cert_data;
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
index 51e02001bb4d..ebb3c642f548 100644
--- a/drivers/virt/coco/tsm.c
+++ b/drivers/virt/coco/tsm.c
@@ -64,6 +64,11 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
  	return container_of(report, struct tsm_report_state, report);
  }
  
+static bool not_visible(const struct configfs_attribute *attr)
+{
+	return false;
+}
+
  static int try_advance_write_generation(struct tsm_report *report)
  {
  	struct tsm_report_state *state = to_state(report);
@@ -139,7 +144,7 @@ static ssize_t tsm_report_svsm_store(struct config_item *cfg,
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, svsm);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, svsm, not_visible);
  
  static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
  					     const char *buf, size_t len)
@@ -168,7 +173,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, service_guid);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, not_visible);
  
  static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
  							 const char *buf, size_t len)
@@ -189,7 +194,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, not_visible);
  
  static ssize_t tsm_report_inblob_write(struct config_item *cfg,
  				       const void *buf, size_t count)
@@ -336,7 +341,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
  
  	return tsm_report_read(report, buf, count, TSM_MANIFEST);
  }
-CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
+CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, not_visible);
  
  #define TSM_DEFAULT_ATTRS() \
  	&tsm_report_attr_generation, \
@@ -444,6 +449,43 @@ static struct configfs_subsystem tsm_configfs = {
  	.su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
  };
  
+int tsm_set_visibility(const char *name, configfs_is_visible_t func)
+{
+	struct configfs_bin_attribute *bin_attr;
+	struct configfs_attribute *attr;
+	const struct config_item_type *t;
+	unsigned int i;
+
+	guard(rwsem_write)(&tsm_rwsem);
+
+	t = provider.type;
+
+	if (t->ct_attrs) {
+		for (i = 0; (attr = t->ct_attrs[i]) != NULL; i++) {
+			if (strcmp(attr->ca_name, name))
+				continue;
+
+			attr->ca_is_visible = func;
+			return 0;
+		}
+	}
+
+	if (t->ct_bin_attrs) {
+		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
+			attr = &bin_attr->cb_attr;
+
+			if (strcmp(attr->ca_name, name))
+				continue;
+
+			attr->ca_is_visible = func;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tsm_set_visibility);
+
  int tsm_register(const struct tsm_ops *ops, void *priv,
  		 const struct config_item_type *type)
  {
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
index c4aed3059500..01b075a4debc 100644
--- a/include/linux/tsm.h
+++ b/include/linux/tsm.h
@@ -5,6 +5,7 @@
  #include <linux/sizes.h>
  #include <linux/types.h>
  #include <linux/uuid.h>
+#include <linux/configfs.h>
  
  #define TSM_INBLOB_MAX 64
  #define TSM_OUTBLOB_MAX SZ_32K
@@ -77,4 +78,7 @@ extern const struct config_item_type tsm_report_extra_type;
  int tsm_register(const struct tsm_ops *ops, void *priv,
  		 const struct config_item_type *type);
  int tsm_unregister(const struct tsm_ops *ops);
+
+int tsm_set_visibility(const char *name, configfs_is_visible_t func);
+
  #endif /* __TSM_H */

Thanks,
Tom

>   
>