Hi Boris, On 8/27/2024 5:02 PM, Borislav Petkov wrote: > On Wed, Jul 31, 2024 at 08:37:57PM +0530, Nikunj A Dadhania wrote: >> Address the ignored failures from snp_init() in sme_enable(). Add error >> handling for scenarios where snp_init() fails to retrieve the SEV-SNP CC >> blob or encounters issues while parsing the CC blob. > > Is this a real issue you've encountered or? As per you comment [1], you had suggested to error out early in snp_init() instead of waiting till snp_init_platform_device(). As snp_init() was ignoring the failure case, I have added this patch. Following patch adds secrets page parsing from CC blob. When the parsing fails, snp_init() will return failure. > >> This change ensures > > Avoid having "This patch" or "This commit" or "This <whatever>" in the commit > message. It is tautologically useless. Sure, will do. >> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c >> index ac33b2263a43..e83b363c5e68 100644 >> --- a/arch/x86/mm/mem_encrypt_identity.c >> +++ b/arch/x86/mm/mem_encrypt_identity.c >> @@ -535,6 +535,13 @@ void __head sme_enable(struct boot_params *bp) >> if (snp && !(msr & MSR_AMD64_SEV_SNP_ENABLED)) >> snp_abort(); >> >> + /* >> + * The SEV-SNP CC blob should be present and parsing CC blob should >> + * succeed when SEV-SNP is enabled. >> + */ >> + if (!snp && (msr & MSR_AMD64_SEV_SNP_ENABLED)) >> + snp_abort(); > > Any chance you could combine the above and this test? > > Perhaps look around at the code before adding your check - there might be some > opportunity for aggregation and improvement... Sure, how about the below patch ? From: Nikunj A Dadhania <nikunj@xxxxxxx> Date: Wed, 22 May 2024 12:43:42 +0530 Subject: [PATCH] x86/sev: Handle failures from snp_init() Address the ignored failures from snp_init() in sme_enable(). Add error handling for scenarios where snp_init() fails to retrieve the SEV-SNP CC blob or encounters issues while parsing the CC blob. Ensure that SNP guests will error out early, preventing delayed error reporting or undefined behavior. Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx> --- arch/x86/mm/mem_encrypt_identity.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index ac33b2263a43..a0124a479972 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -495,7 +495,7 @@ void __head sme_enable(struct boot_params *bp) unsigned int eax, ebx, ecx, edx; unsigned long feature_mask; unsigned long me_mask; - bool snp; + bool snp, snp_enabled; u64 msr; snp = snp_init(bp); @@ -529,10 +529,17 @@ void __head sme_enable(struct boot_params *bp) /* Check the SEV MSR whether SEV or SME is enabled */ RIP_REL_REF(sev_status) = msr = __rdmsr(MSR_AMD64_SEV); - feature_mask = (msr & MSR_AMD64_SEV_ENABLED) ? AMD_SEV_BIT : AMD_SME_BIT; + snp_enabled = msr & MSR_AMD64_SEV_SNP_ENABLED; + feature_mask = snp_enabled ? AMD_SEV_BIT : AMD_SME_BIT; - /* The SEV-SNP CC blob should never be present unless SEV-SNP is enabled. */ - if (snp && !(msr & MSR_AMD64_SEV_SNP_ENABLED)) + /* + * The SEV-SNP CC blob should never be present unless SEV-SNP is enabled. + * + * The SEV-SNP CC blob should be present and parsing CC blob should + * succeed when SEV-SNP is enabled. + */ + if ((snp && !snp_enabled) || + (!snp && snp_enabled)) snp_abort(); /* Check if memory encryption is enabled */ -- 2.34.1 1. https://lore.kernel.org/lkml/20240416144542.GFZh6PFjPNT9Zt3iUl@fat_crate.local/
