MIME-Version: 1.0
References: <b57280b5562893e2616257ac9c2d4525a9aeeb42.1638471124.git.thomas.lendacky@amd.com>
 <YapIMYiJ+iIfHI+c@google.com> <9ac5cf9c-0afb-86d1-1ef2-b3a7138010f2@amd.com>
In-Reply-To: <9ac5cf9c-0afb-86d1-1ef2-b3a7138010f2@amd.com>
From:   Marc Orr <marcorr@google.com>
Date:   Fri, 3 Dec 2021 21:14:19 -0800
Message-ID: <CAA03e5EYV785FSEh2mXK=jzEhp-wM8XcaBk0S4vx7h-f7jevjA@mail.gmail.com>
Subject: Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB
 validation failure
To:     Tom Lendacky <Thomas.Lendacky@amd.com>
Cc:     Sean Christopherson <seanjc@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Brijesh Singh <brijesh.singh@amd.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 3, 2021 at 11:00 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 12/3/21 10:39 AM, Sean Christopherson wrote:
> > On Thu, Dec 02, 2021, Tom Lendacky wrote:
>
> >>
> >> -    return -EINVAL;
> >> +    return false;
> >
> > I'd really prefer that this helper continue to return 0/-EINVAL, there's no hint
> > in the function name that this return true/false.  And given the usage, there's
> > no advantage to returning true/false.  On the contrary, if there's a future
> > condition where this needs to exit to userspace, we'll end up switching this all
> > back to int.
>
> I don't have any objection to that.

I think Sean's review makes a pretty compelling case that we should
keep the int return value for `setup_vmgexit_scratch()`. In
particular, failing to allocate a host kernel buffer definitely seems
like a host error that should return to userspace. Though, failing to
read the guest GPA seems less clear cut on who is at fault (host vs.
guest), as Tom mentioned. My understanding from the commit description
is that the entire point of the patch is to protect the guest from
mis-behaving guest userspace code. So I would think that if we have a
case like mapping the guest GPA that could fail due to the guest or
the host, we should probably go ahead and use the new GHCB error codes
to return back to the guest in this case. But either way, having an
int return code seems like the way to go for
`setup_vmgexit_scratch()`. Because if we are wrong in either
direction, it's a trivial fix.

It's not as obvious to me that converting `sev_es_validate_vmgexit()`
to return a bool is not cleaner than returning an int. This function
seems to pretty much just process the GHCB buffer as a self-contained
set of bits. So it's hard to imagine how this could fail in a way
where exiting to userspace is the right thing to do. That being said,
I do not have a strong objection to returning an int. Sean is right
that an int is definitely more future proof. And I'm sure Sean (and
Tom) have much better insight into how validating the bits written
into the GHCB could potentially require code that could justify
exiting out to userspace.