[v4] Support SEV firmware hotloading

[PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Dionna Glaze 2 weeks, 5 days ago

Ensure that snp gctx page allocation is adequately deallocated on
failure during snp_launch_start.

Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 714c517dd4b72..f6e96ec0a5caa 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (sev->snp_context)
 		return -EINVAL;
 
-	sev->snp_context = snp_context_create(kvm, argp);
-	if (!sev->snp_context)
-		return -ENOTTY;
-
 	if (params.flags)
 		return -EINVAL;
 
@@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
 		return -EINVAL;
 
+	sev->snp_context = snp_context_create(kvm, argp);
+	if (!sev->snp_context)
+		return -ENOTTY;
+
 	start.gctx_paddr = __psp_pa(sev->snp_context);
 	start.policy = params.policy;
 	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Sean Christopherson 2 weeks, 3 days ago

KVM: SVM:

In the future, please post bug fixes separately from new features series, especially
when the fix has very little to do with the rest of the series (AFAICT, this has
no relation whatsoever beyond SNP).

On Tue, Nov 05, 2024, Dionna Glaze wrote:
> Ensure that snp gctx page allocation is adequately deallocated on
> failure during snp_launch_start.
> 
> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

This needs

Cc: stable@vger.kernel.org

especially if it doesn't get into 6.12.

> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Acked-by: Sean Christopherson <seanjc@google.com>

Paolo, do you want to grab this one for 6.12 too?

> ---
>  arch/x86/kvm/svm/sev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 714c517dd4b72..f6e96ec0a5caa 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (sev->snp_context)
>  		return -EINVAL;
>  
> -	sev->snp_context = snp_context_create(kvm, argp);
> -	if (!sev->snp_context)
> -		return -ENOTTY;
> -
>  	if (params.flags)
>  		return -EINVAL;
>  
> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>  		return -EINVAL;
>  
> +	sev->snp_context = snp_context_create(kvm, argp);
> +	if (!sev->snp_context)
> +		return -ENOTTY;

Related to this fix, the return values from snp_context_create() are garbage.  It
should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.

> +
>  	start.gctx_paddr = __psp_pa(sev->snp_context);
>  	start.policy = params.policy;
>  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> -- 
> 2.47.0.199.ga7371fff76-goog
>

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Dionna Amalie Glaze 2 weeks, 3 days ago

On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote:
>
> KVM: SVM:
>
> In the future, please post bug fixes separately from new features series, especially
> when the fix has very little to do with the rest of the series (AFAICT, this has
> no relation whatsoever beyond SNP).
>

Understood. Are dependent series best shared through links to a dev
branch containing all patches?

> On Tue, Nov 05, 2024, Dionna Glaze wrote:
> > Ensure that snp gctx page allocation is adequately deallocated on
> > failure during snp_launch_start.
> >
> > Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
>
> This needs
>
> Cc: stable@vger.kernel.org
>
> especially if it doesn't get into 6.12.
>
> > CC: Sean Christopherson <seanjc@google.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > CC: Ingo Molnar <mingo@redhat.com>
> > CC: Borislav Petkov <bp@alien8.de>
> > CC: Dave Hansen <dave.hansen@linux.intel.com>
> > CC: Ashish Kalra <ashish.kalra@amd.com>
> > CC: Tom Lendacky <thomas.lendacky@amd.com>
> > CC: John Allen <john.allen@amd.com>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Michael Roth <michael.roth@amd.com>
> > CC: Luis Chamberlain <mcgrof@kernel.org>
> > CC: Russ Weight <russ.weight@linux.dev>
> > CC: Danilo Krummrich <dakr@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > CC: "Rafael J. Wysocki" <rafael@kernel.org>
> > CC: Tianfei zhang <tianfei.zhang@intel.com>
> > CC: Alexey Kardashevskiy <aik@amd.com>
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>
> Acked-by: Sean Christopherson <seanjc@google.com>
>
> Paolo, do you want to grab this one for 6.12 too?
>
> > ---
> >  arch/x86/kvm/svm/sev.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 714c517dd4b72..f6e96ec0a5caa 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >       if (sev->snp_context)
> >               return -EINVAL;
> >
> > -     sev->snp_context = snp_context_create(kvm, argp);
> > -     if (!sev->snp_context)
> > -             return -ENOTTY;
> > -
> >       if (params.flags)
> >               return -EINVAL;
> >
> > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >       if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
> >               return -EINVAL;
> >
> > +     sev->snp_context = snp_context_create(kvm, argp);
> > +     if (!sev->snp_context)
> > +             return -ENOTTY;
>
> Related to this fix, the return values from snp_context_create() are garbage.  It
> should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
> as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.
>

I caught this too. I'll be changing that behavior with the new gctx
management API from ccp in v5, i.e.,

/**
 * sev_snp_create_context - allocates an SNP context firmware page
 *
 * Associates the created context with the ASID that an activation
 * call after SNP_LAUNCH_START will commit. The association is needed
 * to track active GCTX pages to refresh during firmware hotload.
 *
 * @asid:    The ASID allocated to the caller that will be used in a
subsequent SNP_ACTIVATE.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * A pointer to the SNP context page, or an ERR_PTR of
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
void *sev_snp_create_context(int asid, int *psp_ret);

/**
 * sev_snp_activate_asid - issues SNP_ACTIVATE for the asid and
associated GCTX page.
 *
 * @asid:    The ASID to activate.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * 0 if the SEV device successfully processed the command
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
int sev_snp_activate_asid(int asid, int *psp_ret);

/**
 * sev_snp_guest_decommission - issues SNP_DECOMMISSION for an asid's
GCTX page and frees it.
 *
 * @asid:    The ASID to activate.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * 0 if the SEV device successfully processed the command
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
int sev_snp_guest_decommission(int asid, int *psp_ret);

> > +
> >       start.gctx_paddr = __psp_pa(sev->snp_context);
> >       start.policy = params.policy;
> >       memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> > --
> > 2.47.0.199.ga7371fff76-goog
> >

-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Sean Christopherson 2 weeks, 3 days ago

On Wed, Nov 06, 2024, Dionna Amalie Glaze wrote:
> On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > KVM: SVM:
> >
> > In the future, please post bug fixes separately from new features series, especially
> > when the fix has very little to do with the rest of the series (AFAICT, this has
> > no relation whatsoever beyond SNP).
> >
> 
> Understood. Are dependent series best shared through links to a dev
> branch containing all patches?

I don't follow.  There is no dependency here.  If this series were moving
snp_context_create() out of KVM, then that would be a different story, i.e. then
it _would_ be appropriate to include the fix at the front of the series.

If you end up a situation where a dependency is created after the initial posting,
e.g. you post this fix, then later decide to move snp_context_create() out of KVM,
then simply call that out in the cover letter and provide a lore.kernel.org link.

For large scale dependencies, e.g. multi-patch series that build on other multi-patch
series, then providing a link to a git branch is helpful.  But for something this
trivial, it's overkill.

> > > ---
> > >  arch/x86/kvm/svm/sev.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > index 714c517dd4b72..f6e96ec0a5caa 100644
> > > --- a/arch/x86/kvm/svm/sev.c
> > > +++ b/arch/x86/kvm/svm/sev.c
> > > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >       if (sev->snp_context)
> > >               return -EINVAL;
> > >
> > > -     sev->snp_context = snp_context_create(kvm, argp);
> > > -     if (!sev->snp_context)
> > > -             return -ENOTTY;
> > > -
> > >       if (params.flags)
> > >               return -EINVAL;
> > >
> > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >       if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
> > >               return -EINVAL;
> > >
> > > +     sev->snp_context = snp_context_create(kvm, argp);
> > > +     if (!sev->snp_context)
> > > +             return -ENOTTY;
> >
> > Related to this fix, the return values from snp_context_create() are garbage.  It
> > should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
> > as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.
> 
> I caught this too. I'll be changing that behavior with the new gctx
> management API from ccp in v5, i.e.,

Please fix the KVM flaws before moving code out of KVM, i.e. ensure the flaws are
cleaned up even if we opt not to go the route of moving the code out of KVM (which
I assume is what you plan to do with sev_snp_create_context()).

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Tom Lendacky 2 weeks, 3 days ago

On 11/4/24 19:05, Dionna Glaze wrote:
> Ensure that snp gctx page allocation is adequately deallocated on
> failure during snp_launch_start.
> 
> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/svm/sev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 714c517dd4b72..f6e96ec0a5caa 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (sev->snp_context)
>  		return -EINVAL;
>  
> -	sev->snp_context = snp_context_create(kvm, argp);
> -	if (!sev->snp_context)
> -		return -ENOTTY;
> -
>  	if (params.flags)
>  		return -EINVAL;
>  
> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>  		return -EINVAL;
>  
> +	sev->snp_context = snp_context_create(kvm, argp);
> +	if (!sev->snp_context)
> +		return -ENOTTY;
> +
>  	start.gctx_paddr = __psp_pa(sev->snp_context);
>  	start.policy = params.policy;
>  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

Posted by Paolo Bonzini 2 weeks, 2 days ago

On 11/6/24 15:29, Tom Lendacky wrote:
> On 11/4/24 19:05, Dionna Glaze wrote:
>> Ensure that snp gctx page allocation is adequately deallocated on
>> failure during snp_launch_start.
>>
>> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
>>
>> CC: Sean Christopherson <seanjc@google.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Borislav Petkov <bp@alien8.de>
>> CC: Dave Hansen <dave.hansen@linux.intel.com>
>> CC: Ashish Kalra <ashish.kalra@amd.com>
>> CC: Tom Lendacky <thomas.lendacky@amd.com>
>> CC: John Allen <john.allen@amd.com>
>> CC: Herbert Xu <herbert@gondor.apana.org.au>
>> CC: "David S. Miller" <davem@davemloft.net>
>> CC: Michael Roth <michael.roth@amd.com>
>> CC: Luis Chamberlain <mcgrof@kernel.org>
>> CC: Russ Weight <russ.weight@linux.dev>
>> CC: Danilo Krummrich <dakr@redhat.com>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: "Rafael J. Wysocki" <rafael@kernel.org>
>> CC: Tianfei zhang <tianfei.zhang@intel.com>
>> CC: Alexey Kardashevskiy <aik@amd.com>
>>
>> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>> ---
>>   arch/x86/kvm/svm/sev.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 714c517dd4b72..f6e96ec0a5caa 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	if (sev->snp_context)
>>   		return -EINVAL;
>>   
>> -	sev->snp_context = snp_context_create(kvm, argp);
>> -	if (!sev->snp_context)
>> -		return -ENOTTY;
>> -
>>   	if (params.flags)
>>   		return -EINVAL;
>>   
>> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>>   		return -EINVAL;
>>   
>> +	sev->snp_context = snp_context_create(kvm, argp);
>> +	if (!sev->snp_context)
>> +		return -ENOTTY;
>> +
>>   	start.gctx_paddr = __psp_pa(sev->snp_context);
>>   	start.policy = params.policy;
>>   	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> 

Applied, thanks.

Paolo