by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH v2 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM

On 3/23/24 12:24, Kuppuswamy, Sathyanarayanan wrote:
>
> On 3/8/24 10:35 AM, Tom Lendacky wrote:
>> Config-fs provides support to hide individual attribute entries. Using
>> this support, base the display of the SVSM related entries on the presence
>> of an SVSM.
>>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/coco/core.c | 4 ++++
>> drivers/virt/coco/tsm.c | 13 +++++++++----
>> include/linux/cc_platform.h | 8 ++++++++
>> 3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index d07be9d05cd0..efa0f648f754 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -12,6 +12,7 @@
>>
>> #include <asm/coco.h>
>> #include <asm/processor.h>
>> +#include <asm/sev.h>
>>
>> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
>> u64 cc_mask __ro_after_init;
>> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>> case CC_ATTR_GUEST_STATE_ENCRYPT:
>> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>>
>> + case CC_ATTR_GUEST_SVSM_PRESENT:
>> + return snp_get_vmpl();
>> +
>> /*
>> * With SEV, the rep string I/O instructions need to be unrolled
>> * but SEV-ES supports them through the #VC handler.
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>> index 07b4c95ce704..2efa6e578477 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 umode_t svsm_visibility(const struct config_item *item, const struct configfs_attribute *attr)
>> +{
>> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT) ? attr->ca_mode : 0;
>> +}
>> +
>
> Instead of directly checking for CC flags here, I am wondering if it would make
> sense to add a callback to vendor drivers and let the callback decide whether
> the attribute is valid or not? We can't add a CC flag for every ConfigFS attribute,
> right? For example, privlevel is not used by TDX. If there is a callback, then
> TDX driver can make this attribute invalid for it.

I think that's something that can be looked at after this series.

Thanks,
Tom

>
>> 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, svsm_visibility);
>>
>> 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, svsm_visibility);
>>
>> 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, svsm_visibility);
>>
>> 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, svsm_visibility);
>>
>> #define TSM_DEFAULT_ATTRS() \
>> &tsm_report_attr_generation, \
>> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
>> index cb0d6cd1c12f..f1b4266c1484 100644
>> --- a/include/linux/cc_platform.h
>> +++ b/include/linux/cc_platform.h
>> @@ -90,6 +90,14 @@ enum cc_attr {
>> * Examples include TDX Guest.
>> */
>> CC_ATTR_HOTPLUG_DISABLED,
>> +
>> + /**
>> + * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
>> + *
>> + * The platform/OS is running as a guest/virtual machine and is
>> + * running under a Secure VM Service Module (SVSM).
>> + */
>> + CC_ATTR_GUEST_SVSM_PRESENT,
>> };
>>
>> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>

2024-03-26 01:11:59

by Kuppuswamy Sathyanarayanan

[permalink] [raw]

Subject: Re: [PATCH v2 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM

Hi,

On 3/25/24 7:05 AM, Tom Lendacky wrote:
> On 3/23/24 12:24, Kuppuswamy, Sathyanarayanan wrote:
>>
>> On 3/8/24 10:35 AM, Tom Lendacky wrote:
>>> Config-fs provides support to hide individual attribute entries. Using
>>> this support, base the display of the SVSM related entries on the presence
>>> of an SVSM.
>>>
>>> Cc: Joel Becker <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> Signed-off-by: Tom Lendacky <[email protected]>
>>> ---
>>> arch/x86/coco/core.c        | 4 ++++
>>> drivers/virt/coco/tsm.c     | 13 +++++++++----
>>> include/linux/cc_platform.h | 8 ++++++++
>>> 3 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>>> index d07be9d05cd0..efa0f648f754 100644
>>> --- a/arch/x86/coco/core.c
>>> +++ b/arch/x86/coco/core.c
>>> @@ -12,6 +12,7 @@
>>> #include <asm/coco.h>
>>> #include <asm/processor.h>
>>> +#include <asm/sev.h>
>>> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
>>> u64 cc_mask __ro_after_init;
>>> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>>>       case CC_ATTR_GUEST_STATE_ENCRYPT:
>>>           return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>>> +    case CC_ATTR_GUEST_SVSM_PRESENT:
>>> +        return snp_get_vmpl();
>>> +
>>>       /*
>>>        * With SEV, the rep string I/O instructions need to be unrolled
>>>        * but SEV-ES supports them through the #VC handler.
>>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>>> index 07b4c95ce704..2efa6e578477 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 umode_t svsm_visibility(const struct config_item *item, const struct configfs_attribute *attr)
>>> +{
>>> +    return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT) ? attr->ca_mode : 0;
>>> +}
>>> +
>>
>> Instead of directly checking for CC flags here, I am wondering if it would make
>> sense to add a callback to vendor drivers and let the callback decide whether
>> the attribute is valid or not? We can't add a CC flag for every ConfigFS attribute,
>> right? For example, privlevel is not used by TDX. If there is a callback, then
>> TDX driver can make this attribute invalid for it.
>
> I think that's something that can be looked at after this series.
>

Ok. Lets see what Dan also thinks about it.

IMO, CC flag is generally added when there is common code that can be shared
across multiple vendors. And this particular use case is specific to SEV for now.
if we move this check to the vendor driver, you can use vendor specific call to
check for SVSM and don't require a new CC flag.

> Thanks,
> Tom
>
>>
>>> 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, svsm_visibility);
>>> 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, svsm_visibility);
>>> 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, svsm_visibility);
>>> 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, svsm_visibility);
>>> #define TSM_DEFAULT_ATTRS() \
>>>       &tsm_report_attr_generation, \
>>> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
>>> index cb0d6cd1c12f..f1b4266c1484 100644
>>> --- a/include/linux/cc_platform.h
>>> +++ b/include/linux/cc_platform.h
>>> @@ -90,6 +90,14 @@ enum cc_attr {
>>>        * Examples include TDX Guest.
>>>        */
>>>       CC_ATTR_HOTPLUG_DISABLED,
>>> +
>>> +    /**
>>> +     * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
>>> +     *
>>> +     * The platform/OS is running as a guest/virtual machine and is
>>> +     * running under a Secure VM Service Module (SVSM).
>>> +     */
>>> +    CC_ATTR_GUEST_SVSM_PRESENT,
>>> };
>>> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer