linux-kernel - Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

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: <0b5dcab333906f166fcdbc296373cc5e08bec79f.camel@infradead.org>
Date:   Tue, 09 Aug 2022 15:16:59 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Coleman Dietsch <dietschc@....edu>, kvm@...r.kernel.org,
        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>, linux-kernel@...r.kernel.org,
        skhan@...uxfoundation.org, Pavel Skripkin <paskripkin@...il.com>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        stable@...r.kernel.org,
        syzbot+e54f930ed78eb0f85281@...kaller.appspotmail.com,
        metikaya <metikaya@...zon.co.uk>
Subject: Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

On Tue, 2022-08-09 at 14:07 +0000, Sean Christopherson wrote:
> On Tue, Aug 09, 2022, David Woodhouse wrote:
> > On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
> > > On 8/9/22 11:22, David Woodhouse wrote:
> > > > On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
> > > > > Stop Xen timer (if it's running) prior to changing the IRQ
> > > > > vector and
> > > > > potentially (re)starting the timer. Changing the IRQ vector
> > > > > while the
> > > > > timer is still running can result in KVM injecting a garbage
> > > > > event, e.g.
> > > > > vm_xen_inject_timer_irqs() could see a non-zero
> > > > > xen.timer_pending from
> > > > > a previous timer but inject the new xen.timer_virq.
> > > > 
> > > > Hm, wasn't that already addressed in the first patch I saw,
> > > > which just
> > > > called kvm_xen_stop_timer() unconditionally before (possibly)
> > > > setting
> > > > it up again?
> > > 
> > > Which patch is that?
> > 
> > The one I acked in
> > https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@infradead.org/
> > 
> 
> It's effectively the same patch.  I had asked Coleman to split it into two separate
> patches: (1) fix the re-initialization of an active timer bug and (2) stop the active
> timer before changing the vector (this patch).
> 

But both bugs just require that the timer is stopped first. I preferred
the original which was less intrusive, which did just that:

	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
		/* Stop current timer if it is enabled */
		if (kvm_xen_timer_enabled(vcpu)) {
			kvm_xen_stop_timer(vcpu);
			vcpu->arch.xen.timer_virq = 0;
		}
		if (data->u.timer.port) {
			if (data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
				r = -EINVAL;
				break;
			}
			vcpu->arch.xen.timer_virq = data->u.timer.port;
			kvm_xen_init_timer(vcpu);

			/* Restart the timer if it's set */
			if (data->u.timer.expires_ns)
				kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
						    data->u.timer.expires_ns -
						    get_kvmclock_ns(vcpu->kvm));
		}
		r = 0;
		break;


I find the new version a bit harder to follow, with its init-then-stop-
then-start logic:

	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
		if (data->u.timer.port &&
		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
			r = -EINVAL;
			break;
                }

		if (!vcpu->arch.xen.timer.function)
			kvm_xen_init_timer(vcpu);

		/* Stop the timer (if it's running) before changing the vector */
		kvm_xen_stop_timer(vcpu);
		vcpu->arch.xen.timer_virq = data->u.timer.port;

		/* Start the timer if the new value has a valid vector+expiry. */
		if (data->u.timer.port && data->u.timer.expires_ns)
			kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
					    data->u.timer.expires_ns -
					    get_kvmclock_ns(vcpu->kvm));
                r = 0;
		break;

But I won't fight you for it. 


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