linux-kernel - Re: [PATCH] KVM: SVM: fix missing sev_decommission in sev_receive

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Sep 2021 15:26:19 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Mingwei Zhang <mizhang@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Alper Gun <alpergun@...gle.com>,
        Borislav Petkov <bp@...en8.de>,
        Brijesh Singh <brijesh.singh@....com>,
        David Rienjes <rientjes@...gle.com>,
        Marc Orr <marcorr@...gle.com>, John Allen <john.allen@....com>,
        Peter Gonda <pgonda@...gle.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Vipin Sharma <vipinsh@...gle.com>
Subject: Re: [PATCH] KVM: SVM: fix missing sev_decommission in
 sev_receive_start

On Sun, Sep 12, 2021, Mingwei Zhang wrote:
> sev_decommission

It's not a ubiquitous "requirement", but adding parantheses after function names
in changelogs is generally preferred as it succinctly identifies a function call.

> is needed in the error path of sev_bind_asid. The purpose

"error path of sev_bind_asid()" could be interpreted as meaning DECOMMISSION is
needed within sev_bind_asid(), whereas you mean when sev_bind_asid() fails.  One
way to avoid confusion, and a good habit in general, is to avoid talking in
terms of code details when possible.  There are certainly cases where talking
about the code itself is absolutely necessary, but this could be worded as:

  DECOMMISSION the current SEV context if binding an ASID fails after
  RECEIVE_START.  Per AMD's SEV API, RECEIVE_START generates a new guest
  context and thus needs to be paired with DECOMMISSION:

     The RECEIVE_START command is the only command other than the LAUNCH_START
     command that generates a new guest context and guest handle.

  The missing DECOMMISSION can result in subsequent SEV launch failures due to
  <fill in this part>.

  Note, LAUNCH_START suffered the same bug, but was previously fixed by
  934002cd660b ("KVM: SVM: Call SEV Guest Decommission if ASID binding fails").

> of this function is to clear the firmware context. Missing this step may

Kind of a nit: "this function" is ambiguous.  I'm pretty sure you're referring
to sev_decommission(), but it's trivially easy to be 100% unambiguous (see above).

> cause subsequent SEV launch failures.
>
> Although missing sev_decommission issue has previously been found and was
> fixed in sev_launch_start function. It is supposed to be fixed on all
> scenarios where a firmware context needs to be freed.

More nits: provide the commit ID of the previous fix so that folks don't have
to hunt it down (even though it's an easy git blame away).  And this is better
as a footnote of sorts as it's not relevant to the justification of the patch in
the sense that it's best to avoid "we do x there, so we should do x here".  That
might be a true statement, as is the case here, but the patch still needs to be
justified without that type of reasoning.

> According to the AMD SEV API v0.24 Section 1.3.3:
>
> "The RECEIVE_START command is the only command other than the LAUNCH_START
> command that generates a new guest context and guest handle."
>
> The above indicates that RECEIVE_START command also requires calling
> sev_decommission if ASID binding fails after RECEIVE_START succeeds.
>
> So add the sev_decommission function in sev_receive_start.

And more nits :-)  As alluded to above the, last few sentences are somewhat
redundant and can be dropped and/or worded into the statement about what the
patch is doing.

> Cc: Alper Gun <alpergun@...gle.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Brijesh Singh <brijesh.singh@....com>
> Cc: David Rienjes <rientjes@...gle.com>
> Cc: Marc Orr <marcorr@...gle.com>
> Cc: John Allen <john.allen@....com>
> Cc: Peter Gonda <pgonda@...gle.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: Vipin Sharma <vipinsh@...gle.com>
> 
> Reviewed-by: Marc Orr <marcorr@...gle.com>
> Acked-by: Brijesh Singh <brijesh.singh@....com>
> Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command")

Cc: stable@...r.kernel.org

> Signed-off-by: Mingwei Zhang <mizhang@...gle.com>
> ---

With a cleaned up changelog,

Reviewed-by: Sean Christopherson <seanjc@...gle.com>