linux-kernel - Re: [PATCH 2/2] KVM: SVM: Provide helpers to set the error code

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]

Message-ID: <Z2DMIWqSP3h0cV-F@google.com>
Date: Mon, 16 Dec 2024 16:56:01 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Melody Wang <huibo.wang@....com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, KVM <kvm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Tom Lendacky <thomas.lendacky@....com>, 
	Dhaval Giani <dhaval.giani@....com>
Subject: Re: [PATCH 2/2] KVM: SVM: Provide helpers to set the error code

On Fri, Dec 06, 2024, Melody Wang wrote:
> @@ -3577,8 +3576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>  	return 0;
>  
>  e_scratch:
> -	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
> +	svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_SCRATCH_AREA);
>  
>  	return 1;
>  }
> @@ -3671,7 +3669,11 @@ static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
>  	svm->sev_es.psc_inflight = 0;
>  	svm->sev_es.psc_idx = 0;
>  	svm->sev_es.psc_2m = false;
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> +	/*
> +	 * psc_ret contains 0 if all entries have been processed successfully

No, it doesn't.

  • SW_EXITINFO2 == 0x00000000
    The page state change request was interrupted, retry the request.

> +	 * or a reason code identifying why the request has not completed.

Honestly, even if it were correct, this comment does far more harm than good.
The literal '0' below has nothing to do with the '0' referenced in the comment,
and so all the comment does is add more confusion.  Furthermore, this is the
wrong place to document SW_EXITINFO2 return codes.  That's the responsibility of
the call sites and/or individual PSC-specific macros.

This code should instead document the weirdness with PSC's SW_EXITINFO1:

	/*
	 * Because the GHCB says so, PSC requests always get a "no action"
	 * response, with a PSC-specific return code in SW_EXITINFO2.
	 *
	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION, psc_ret);

The readers of _this_ code don't care about the individual return codes, they
care about knowing the semantics of the return itself.

Alternatively, it might even make sense to add a svm_vmgexit_no_action() helper
(along with svm_vmgexit_success()).  There are at least two VMGEXIT types that
roll their own error codes with NO_ACTION, and it might be better to have the
initial zeroing in sev_handle_vmgexit() use NO_ACTION, e.g.

	/*
	 * Assume no action is required as the vast majority of VMGEXITs don't
	 * require the guest to take action upon success, and initializing the
	 * GHCB for the happy case avoids the need to do so for exits that are
	 * handled via SVM's common exit handlers.
	 */
	svm_vmgexit_no_action(svm, 0);

> +	 */
> +	svm_vmgexit_set_return_code(svm, 0, psc_ret);
>  }
>  
>  static void __snp_complete_one_psc(struct vcpu_svm *svm)
> @@ -4067,8 +4069,12 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
>  		ret = -EIO;
>  		goto out_unlock;
>  	}
> -
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(0, fw_err));
> +	/*
> +	 * SNP_GUEST_ERR(0, fw_err): Upper 32-bits (63:32) will contain the
> +	 * return code from the hypervisor. Lower 32-bits (31:0) will contain
> +	 * the return code from the firmware call (0 = success)

Same thing here.  A comment documenting SNP_GUEST_ERR() belongs above the
definition of SNP_GUEST_ERR.  How the "guest error code" is constructed isn't
relevant/unique to this code, what's relevant is that *KVM* isn't requesting an
action.

	/*
	 * No action is requested from KVM, even if the request failed due to a
	 * firmware error.
	 */
	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION,
				    SNP_GUEST_ERR(0, fw_err));


> +	 */
> +	svm_vmgexit_set_return_code(svm, 0, SNP_GUEST_ERR(0, fw_err));
>  
>  	ret = 1; /* resume guest */
>  

...

> +static inline void svm_vmgexit_set_return_code(struct vcpu_svm *svm,
> +						u64 response, u64 data)
> +{
> +	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, response);
> +	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, data);
> +}
> +
> +static inline void svm_vmgexit_inject_exception(struct vcpu_svm *svm, u8 vector)
> +{
> +	u64 data = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT | vector;
> +
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_ISSUE_EXCEPTION, data);
> +}
> +
> +static inline void svm_vmgexit_bad_input(struct vcpu_svm *svm, u64 suberror)
> +{
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_MALFORMED_INPUT, suberror);
> +}
> +
> +static inline void svm_vmgexit_success(struct vcpu_svm *svm, u64 data)

As mentioned in patch 1, please keep svm_vmgexit_success() even if
GHCB_HV_RESP_SUCCESS is renamed to GHCB_HV_RESP_NO_ACTION.

> +{
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_SUCCESS, data);
> +}
> +
>  /* svm.c */
>  #define MSR_INVALID				0xffffffffU
>  
> -- 
> 2.34.1
>