| Message ID | 20240731150811.156771-7-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
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? > This change ensures Avoid having "This patch" or "This commit" or "This <whatever>" in the commit message. It is tautologically useless. Also, do $ git grep 'This patch' Documentation/process for more details. > that SNP guests will error out early, preventing delayed error reporting or > undefined behavior. > > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> > --- > arch/x86/mm/mem_encrypt_identity.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > 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...
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@amd.com> 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@amd.com> --- 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 */
On Wed, Aug 28, 2024 at 10:17:57AM +0530, Nikunj A. Dadhania wrote: > + if ((snp && !snp_enabled) || > + (!snp && snp_enabled)) > snp_abort(); And which boolean function is that? diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index e83b363c5e68..706cb59851b0 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -495,10 +495,10 @@ 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_en; u64 msr; - snp = snp_init(bp); + snp_en = snp_init(bp); /* Check for the SME/SEV support leaf */ eax = 0x80000000; @@ -531,15 +531,11 @@ void __head sme_enable(struct boot_params *bp) RIP_REL_REF(sev_status) = msr = __rdmsr(MSR_AMD64_SEV); feature_mask = (msr & MSR_AMD64_SEV_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)) - snp_abort(); - /* - * The SEV-SNP CC blob should be present and parsing CC blob should - * succeed when SEV-SNP is enabled. + * Any discrepancies between the presence of a CC blob and SNP + * enablement abort the guest. */ - if (!snp && (msr & MSR_AMD64_SEV_SNP_ENABLED)) + if (snp_en ^ (msr & MSR_AMD64_SEV_SNP_ENABLED)) snp_abort(); /* Check if memory encryption is enabled */
On 8/28/2024 3:19 PM, Borislav Petkov wrote: > On Wed, Aug 28, 2024 at 10:17:57AM +0530, Nikunj A. Dadhania wrote: >> + if ((snp && !snp_enabled) || >> + (!snp && snp_enabled)) >> snp_abort(); > > And which boolean function is that? Ah.. missed that. > /* > - * The SEV-SNP CC blob should be present and parsing CC blob should > - * succeed when SEV-SNP is enabled. > + * Any discrepancies between the presence of a CC blob and SNP > + * enablement abort the guest. > */ > - if (!snp && (msr & MSR_AMD64_SEV_SNP_ENABLED)) > + if (snp_en ^ (msr & MSR_AMD64_SEV_SNP_ENABLED)) > snp_abort(); > > /* Check if memory encryption is enabled */ > Do you want me to send the patch again with above change? Regards Nikunj
On Wed, Aug 28, 2024 at 03:46:23PM +0530, Nikunj A. Dadhania wrote:
> Do you want me to send the patch again with above change?
After I've gone through the whole set, sure.
Thx.
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(); + /* Check if memory encryption is enabled */ if (feature_mask == AMD_SME_BIT) { if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
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. This change ensures that SNP guests will error out early, preventing delayed error reporting or undefined behavior. Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- arch/x86/mm/mem_encrypt_identity.c | 7 +++++++ 1 file changed, 7 insertions(+)