linux-kernel - Re: [PATCH] x86/sev: clean up initialization of variables info and v

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] [day] [month] [year] [list]

Message-ID: <YabjZbB8rbMHHX25@google.com>
Date:   Wed, 1 Dec 2021 02:52:21 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Colin Ian King <colin.i.king@...glemail.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/sev: clean up initialization of variables info and v

On Fri, Nov 26, 2021, Colin Ian King wrote:
> Currently variable info is being assigned twice, the second assignment
> is redundant. Clean up the code by making the assignments at declaration
> time and remove the latter two assignments.
> 
> Signed-off-by: Colin Ian King <colin.i.king@...il.com>

Heh, checkpatch complains about the different email domains as it doesn't know
that googlemail.com and gmail.com are more or less the same.  Doubt it matters
much, but probably worth having your SoB match your local git config.

> ---
>  arch/x86/kernel/sev-shared.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index ce987688bbc0..6083d6f658c8 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -104,10 +104,7 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
>  
>  	if (ret == 1) {
>  		u64 info = ghcb->save.sw_exit_info_2;
> -		unsigned long v;
> -
> -		info = ghcb->save.sw_exit_info_2;
> -		v = info & SVM_EVTINJ_VEC_MASK;
> +		unsigned long v = info & SVM_EVTINJ_VEC_MASK;

'v' should really be a u8, and IMO 'v' is a pointlessly short.  Opportunistically
squash this in?

---
 arch/x86/kernel/sev-shared.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 6083d6f658c8..2feb7359ed3a 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -104,13 +104,13 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt

 	if (ret == 1) {
 		u64 info = ghcb->save.sw_exit_info_2;
-		unsigned long v = info & SVM_EVTINJ_VEC_MASK;
+		u8 vector = info & SVM_EVTINJ_VEC_MASK;

 		/* Check if exception information from hypervisor is sane. */
 		if ((info & SVM_EVTINJ_VALID) &&
-		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
+		    ((vector == X86_TRAP_GP) || (vector == X86_TRAP_UD)) &&
 		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
-			ctxt->fi.vector = v;
+			ctxt->fi.vector = vector;

 			if (info & SVM_EVTINJ_VALID_ERR)
 				ctxt->fi.error_code = info >> 32;
--


>  		/* Check if exception information from hypervisor is sane. */

s/sane/mostly sane, this code doesn't check that the hypervisor correctly provided
an error.  It doesn't really matter since ctxt->fi.error_code is zeroed, i.e. will
have clean data for #GP, and is ignored by #UD.  And nothing downstream should
be doing anything more than reporting the error code anyways since the value is
fully VMM controlled.  But to be pedantic...

From: Sean Christopherson <seanjc@...gle.com>
Date: Tue, 30 Nov 2021 18:40:30 -0800
Subject: [PATCH] x86/sev: Verify that VMM provides error code on #GP, and not
 on #UD

When handling an "injected" exception after #VMGEXIT, verify that the VMM
did (not) provide an error code as appropriate for the signaled vector.
Treat an error code mismatch as a VMM error instead of synthesizing an
exception.

Fixes: 597cfe48212a ("x86/boot/compressed/64: Setup a GHCB-based VC Exception handler")
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kernel/sev-shared.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 2feb7359ed3a..139d88a76adc 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -105,11 +105,13 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
 	if (ret == 1) {
 		u64 info = ghcb->save.sw_exit_info_2;
 		u8 vector = info & SVM_EVTINJ_VEC_MASK;
+		bool want_error_code = (vector == X86_TRAP_GP);

 		/* Check if exception information from hypervisor is sane. */
 		if ((info & SVM_EVTINJ_VALID) &&
 		    ((vector == X86_TRAP_GP) || (vector == X86_TRAP_UD)) &&
-		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
+		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT) &&
+		    (!!(info & SVM_EVTINJ_VALID_ERR) == want_error_code)) {
 			ctxt->fi.vector = vector;

 			if (info & SVM_EVTINJ_VALID_ERR)
--