| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <07131427-8dc0-ed47-03af-86060fea3937@amd.com>
Date: Wed, 14 Aug 2024 17:51:06 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Ashish Kalra <Ashish.Kalra@....com>, seanjc@...gle.com,
pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, hpa@...or.com, herbert@...dor.apana.org.au
Cc: x86@...nel.org, john.allen@....com, davem@...emloft.net,
michael.roth@....com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-crypto@...r.kernel.org
Subject: Re: [PATCH 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command
On 8/12/24 14:42, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@....com>
>
> The FEATURE_INFO command provides host and guests a programmatic means
> to learn about the supported features of the currently loaded firmware.
> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
> Instead of using the CPUID instruction to retrieve Fn8000_0024,
> software can use FEATURE_INFO.
Expand on this a bit. This should state that host/hypervisor would use
the FEATURE_INFO command, while guests would actually issue the CPUID
instruction. The idea being, that the hypervisor could add Fn8000_0024
values to the CPUID table provided to the guest.
>
> During CCP module initialization, after firmware update, the SNP
> platform status and feature information from CPUID 0x8000_0024,
> sub-function 0, are cached in the sev_device structure.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> ---
> drivers/crypto/ccp/sev-dev.c | 40 ++++++++++++++++++++++++++++++++++++
> drivers/crypto/ccp/sev-dev.h | 3 +++
> include/linux/psp-sev.h | 31 ++++++++++++++++++++++++++++
> 3 files changed, 74 insertions(+)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9810edbb272d..eefb481db5af 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
> case SEV_CMD_SNP_GUEST_REQUEST: return sizeof(struct sev_data_snp_guest_request);
> case SEV_CMD_SNP_CONFIG: return sizeof(struct sev_user_data_snp_config);
> case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit);
> + case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_feature_info);
> default: return 0;
> }
>
> @@ -1052,6 +1053,43 @@ static void snp_set_hsave_pa(void *arg)
> wrmsrl(MSR_VM_HSAVE_PA, 0);
> }
>
> +static void sev_cache_snp_platform_status_and_discover_features(void)
How about snp_get_platform_info or snp_get_platform_data. If anything is
ever added to this in the future, the name won't have to be changed.
> +{
> + struct sev_device *sev = psp_master->sev_data;
> + struct sev_snp_feature_info snp_feat_info;
> + struct sev_feature_info *feat_info;
> + struct sev_data_snp_addr buf;
> + int error = 0, rc;
> +
> + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> + return;
> +
> + buf.address = __psp_pa(&sev->snp_plat_status);
> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
This command is being called before SNP_INIT_EX (for now), so this is
currently safe. But you should probably guard against it being called
afterwards by checking the SNP state. If the SNP state is INIT, then
bail or use an intermediate firmware page for output.
> +
> + /*
> + * Do feature discovery of the currently loaded firmware,
Block comments in this directory start like this:
/* Do feature discovery...
> + * and cache feature information from CPUID 0x8000_0024,
> + * sub-function 0.
> + */
> + if (!rc && sev->snp_plat_status.feature_info) {
> + /*
> + * Use dynamically allocated structure for the SNP_FEATURE_INFO
Ditto.
> + * command to handle any alignment and page boundary check
> + * requirements.
> + */
> + feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
> + snp_feat_info.length = sizeof(snp_feat_info);
> + snp_feat_info.ecx_in = 0;
> + snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
> +
> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
> + if (!rc)
> + sev->feat_info = *feat_info;
Above, you go directly into the sev->snp_plat_status field, but here you
allocate memory and then copy. Any reason for the difference?
> + kfree(feat_info);
> + }
> +}
> +
> static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
> {
> struct sev_data_range_list *range_list = arg;
> @@ -2395,6 +2433,8 @@ void sev_pci_init(void)
> if (sev_update_firmware(sev->dev) == 0)
> sev_get_api_version();
>
> + sev_cache_snp_platform_status_and_discover_features();
> +
> /* Initialize the platform */
> args.probe = true;
> rc = sev_platform_init(&args);
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 3e4e5574e88a..11e571e87e18 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -57,6 +57,9 @@ struct sev_device {
> bool cmd_buf_backup_active;
>
> bool snp_initialized;
> +
> + struct sev_user_data_snp_status snp_plat_status;
> + struct sev_feature_info feat_info;
> };
>
> int sev_dev_init(struct psp_device *psp);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea8585..d46d73911a76 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -107,6 +107,7 @@ enum sev_cmd {
> SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
> SEV_CMD_SNP_COMMIT = 0x0CB,
> SEV_CMD_SNP_VLEK_LOAD = 0x0CD,
> + SEV_CMD_SNP_FEATURE_INFO = 0x0CE,
>
> SEV_CMD_MAX,
> };
> @@ -812,6 +813,36 @@ struct sev_data_snp_commit {
> u32 len;
> } __packed;
>
> +/**
> + * struct sev_snp_feature_info - SEV_SNP_FEATURE_INFO structure
> + *
> + * @length: len of the command buffer read by the PSP
> + * @ecx_in: subfunction index
> + * @feature_info_paddr : SPA of the FEATURE_INFO structure
> + */
> +struct sev_snp_feature_info {
You should be consistent with the other names and call this
sev_data_snp_feature_info.
> + u32 length;
> + u32 ecx_in;
> + u64 feature_info_paddr;
> +} __packed;
> +
> +/**
> + * struct feature_info - FEATURE_INFO structure
> + *
> + * @eax: output of SEV_SNP_FEATURE_INFO command
> + * @ebx: output of SEV_SNP_FEATURE_INFO command
> + * @ecx: output of SEV_SNP_FEATURE_INFO command
> + * #edx: output of SEV_SNP_FEATURE_INFO command
s/SEV_//
> + */
> +struct sev_feature_info {
> + u32 eax;
> + u32 ebx;
> + u32 ecx;
> + u32 edx;
> +} __packed;
> +
> +#define FEAT_CIPHERTEXTHIDING_SUPPORTED BIT(3)
How about SNP_CIPHER_TEXT_HIDING_SUPPORTED.
But, this shouldn't be defined until you use it.
Thanks,
Tom
> +
> #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>
> /**
Powered by blists - more mailing lists