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

[Sean Christopherson]

On Wed, Nov 06, 2024, Dionna Amalie Glaze wrote:
> On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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()).