linux-kernel - Re: [PATCH v7 11/11] KVM: xen: allow vcpu

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: <d89f022f2813a48b7a7751d5b81c391844e94041.camel@infradead.org>
Date:   Wed, 08 Nov 2023 21:56:17 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paul Durrant <paul@....org>, Jack Allister <jalliste@...zon.co.uk>,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paul Durrant <pdurrant@...zon.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org
Subject: Re: [PATCH v7 11/11] KVM: xen: allow vcpu_info content to be
 'safely' copied

On Wed, 2023-11-08 at 12:55 -0800, Sean Christopherson wrote:
> 
> Any objection to splitting out the vcpu_info chunk to a separate function?  That'd
> eliminate the subtle gpc+lock switch, and provide a convenient and noticeable
> location to explain why it's ok for setting the vcpu_info to fail.

No objection, that looks entirely sane to me.

> E.g.
> 
> ---
>  arch/x86/kvm/xen.c | 102 ++++++++++++++++++++++++++-------------------
>  1 file changed, 58 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index d65e89e062e4..cd2021ba0ae3 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -1621,6 +1621,57 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port)
>         }
>  }
>  
> +static int kvm_xen_needs_a_name(struct kvm_vcpu *vcpu, int port_word_bit)
> 

kvm_xen_deliver_vcpu_evtchn_pending_sel()  ?

...

> - out_rcu:
> + out_unlock:
>         read_unlock_irqrestore(&gpc->lock, flags);
> +
> +       /* Paul to add comment explaining why it's ok kvm_xen_needs_a_name() fails */

Basically the point here is that it *doesn't* fail. Either it delivers
directly to the vcpu_info at the appropriate GPA via the GPC, or it
will set the corresponding bit in the 'shadow'
vcpu->arch.xen.evtchn_pending_sel to be delivered later by
kvm_xen_inject_pending_events(). Either way, it's 'success'.

I do want to vet the code path where userspace forgets to reinstate the
vcpu_info before running the vCPU again. Looking at
kvm_xen_inject_pending_events(), I think the kvm_gpc_check() for the
vcpu_info will fail, so it'll just return, and KVM will enter the vCPU
with vcpu->arch.xen.evtchn_pending_sel still unchanged? The bits won't
get delivered until the next time the vCPU is entered after the
vcpu_info is set.

Which I *think* is fine. Userspace has to stop running the vCPU in
order to actually set the vcpu_info again, when it finally decides to
do so. And if it never does, that works out as well as can be
expected. 

Probably does want adding to the selftest.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)