linux-kernel - Re: [PATCH 1/4] KVM: x86: always initialize system

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: <87bkwta1p4.wl-maz@kernel.org>
Date:   Fri, 22 Apr 2022 10:48:23 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org, will@...nel.org,
        apatel@...tanamicro.com, atishp@...osinc.com, seanjc@...gle.com,
        pgonda@...gle.com
Subject: Re: [PATCH 1/4] KVM: x86: always initialize system_event.ndata

On Thu, 21 Apr 2022 19:04:40 +0100,
Paolo Bonzini <pbonzini@...hat.com> wrote:
> 
> The KVM_SYSTEM_EVENT_NDATA_VALID mechanism that was introduced
> contextually with KVM_SYSTEM_EVENT_SEV_TERM is not a good match
> for ARM and RISC-V, which want to communicate information even
> for existing KVM_SYSTEM_EVENT_* constants.  Userspace is not ready
> to filter out bit 31 of type, and fails to process the
> KVM_EXIT_SYSTEM_EVENT exit.
> 
> Therefore, tie the availability of ndata to a system capability
> (which will be added once all architectures are on board).
> Userspace written for released versions of Linux has no reason to
> check flags, since it was never written, so it is okay to replace
> it with ndata and data[0] (on 32-bit kernels) or with data[0]
> (on 64-bit kernels).

How is it going to work for new userspace on old kernels, for which
the ndata field is left uninitialised?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/svm/sev.c   | 3 +--
>  arch/x86/kvm/x86.c       | 2 ++
>  include/uapi/linux/kvm.h | 1 -
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a93f0d01bb90..51b963ec122b 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2739,8 +2739,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  			reason_set, reason_code);
>  
>  		vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> -		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM |
> -					       KVM_SYSTEM_EVENT_NDATA_VALID;
> +		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM;
>  		vcpu->run->system_event.ndata = 1;
>  		vcpu->run->system_event.data[1] = control->ghcb_gpa;

Isn't this really odd? ndata = 1, and yet you populate data[1]?

>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4e7f3a8da16a..517c0228881c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10056,12 +10056,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_HV_CRASH, vcpu)) {
>  			vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  			vcpu->run->system_event.type = KVM_SYSTEM_EVENT_CRASH;
> +			vcpu->run->system_event.ndata = 0;
>  			r = 0;
>  			goto out;
>  		}
>  		if (kvm_check_request(KVM_REQ_HV_RESET, vcpu)) {
>  			vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  			vcpu->run->system_event.type = KVM_SYSTEM_EVENT_RESET;
> +			vcpu->run->system_event.ndata = 0;
>  			r = 0;
>  			goto out;
>  		}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index dd1d8167e71f..5a57f74b4903 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -445,7 +445,6 @@ struct kvm_run {
>  #define KVM_SYSTEM_EVENT_RESET          2
>  #define KVM_SYSTEM_EVENT_CRASH          3
>  #define KVM_SYSTEM_EVENT_SEV_TERM       4
> -#define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
>  			__u32 type;
>  			__u32 ndata;
>  			__u64 data[16];

Cat we please get a #define that aliases data[0] to flags? At the next
merge of the KVM headers into their respective trees, all the existing
VMM are going to break if they have a reference to this field (CrosVM
definitely does today -- yes, we're ahead of time).

Also, getting a bisectable series would be good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.