linux-kernel - Re: [PATCH 05/10] KVM: x86: Don't bleed PVCLOCK_GUEST

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: <30fb80cb-7f4b-4abe-8095-c9b029013923@xen.org>
Date: Tue, 21 Jan 2025 16:54:09 +0000
From: Paul Durrant <xadimgnik@...il.com>
To: Sean Christopherson <seanjc@...gle.com>,
 Paolo Bonzini <pbonzini@...hat.com>, David Woodhouse <dwmw2@...radead.org>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 syzbot+352e553a86e0d75f5120@...kaller.appspotmail.com,
 Paul Durrant <pdurrant@...zon.com>, David Woodhouse <dwmw@...zon.co.uk>,
 Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 05/10] KVM: x86: Don't bleed PVCLOCK_GUEST_STOPPED across
 PV clocks

On 18/01/2025 00:55, Sean Christopherson wrote:
> When updating a specific PV clock, make a full copy of KVM's reference
> copy/cache so that PVCLOCK_GUEST_STOPPED doesn't bleed across clocks.
> E.g. in the unlikely scenario the guest has enabled both kvmclock and Xen
> PV clock, a dangling GUEST_STOPPED in kvmclock would bleed into Xen PV
> clock.

... but the line I queried in the previous patch squashes the flag 
before the Xen PV clock is set up, so no bleed?

> 
> Using a local copy of the pvclock structure also sets the stage for
> eliminating the per-vCPU copy/cache (only the TSC frequency information
> actually "needs" to be cached/persisted).
> 
> Fixes: aa096aa0a05f ("KVM: x86/xen: setup pvclock updates")
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>   arch/x86/kvm/x86.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3c4d210e8a9e..5f3ad13a8ac7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3123,8 +3123,11 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
>   {
>   	struct kvm_vcpu_arch *vcpu = &v->arch;
>   	struct pvclock_vcpu_time_info *guest_hv_clock;
> +	struct pvclock_vcpu_time_info hv_clock;
>   	unsigned long flags;
>   
> +	memcpy(&hv_clock, &vcpu->hv_clock, sizeof(hv_clock));
> +
>   	read_lock_irqsave(&gpc->lock, flags);
>   	while (!kvm_gpc_check(gpc, offset + sizeof(*guest_hv_clock))) {
>   		read_unlock_irqrestore(&gpc->lock, flags);
> @@ -3144,25 +3147,25 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
>   	 * it is consistent.
>   	 */
>   
> -	guest_hv_clock->version = vcpu->hv_clock.version = (guest_hv_clock->version + 1) | 1;
> +	guest_hv_clock->version = hv_clock.version = (guest_hv_clock->version + 1) | 1;
>   	smp_wmb();
>   
>   	/* retain PVCLOCK_GUEST_STOPPED if set in guest copy */
> -	vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED);
> +	hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED);
>   
> -	memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock));
> +	memcpy(guest_hv_clock, &hv_clock, sizeof(*guest_hv_clock));
>   
>   	if (force_tsc_unstable)
>   		guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT;
>   
>   	smp_wmb();
>   
> -	guest_hv_clock->version = ++vcpu->hv_clock.version;
> +	guest_hv_clock->version = ++hv_clock.version;
>   
>   	kvm_gpc_mark_dirty_in_slot(gpc);
>   	read_unlock_irqrestore(&gpc->lock, flags);
>   
> -	trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
> +	trace_kvm_pvclock_update(v->vcpu_id, &hv_clock);
>   }
>   
>   static int kvm_guest_time_update(struct kvm_vcpu *v)