Re: [PATCH 0/2] sync_regs() TOCTOU issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/15/23 17:40, Sean Christopherson wrote:
> On Tue, Aug 15, 2023, Michal Luczaj wrote:
>>> @@ -115,6 +116,7 @@ static void *race_events_exc(void *arg)
>>>  	for (;;) {
>>>  		WRITE_ONCE(run->kvm_dirty_regs, KVM_SYNC_X86_EVENTS);
>>>  		WRITE_ONCE(events->flags, 0);
>>> +		WRITE_ONCE(events->exception.nr, GP_VECTOR);
>>>  		WRITE_ONCE(events->exception.pending, 1);
>>>  		WRITE_ONCE(events->exception.nr, 255);
>>
>> Here you're setting events->exception.nr twice. Is it deliberate?
> 
> Heh, yes and no.  It's partly leftover from a brief attempt to gracefully eat the
> fault in the guest.
> 
> However, unless there's magic I'm missing, race_events_exc() needs to set a "good"
> vector in every iteration, otherwise only the first iteration will be able to hit
> the "check good, consume bad" scenario.

I think I understand what you mean. I see things slightly different: because

	if (events->flags & KVM_VCPUEVENT_VALID_PAYLOAD) {
		...
	} else {
		events->exception.pending = 0;
		events->exception_has_payload = 0;
	}

zeroes exception.pending on every iteration, even though exception.nr may
already be > 31, KVM does not necessary return -EINVAL at

	if ((events->exception.injected || events->exception.pending) &&
	    (events->exception.nr > 31 || events->exception.nr == NMI_VECTOR))
		return -EINVAL;

It would if the racer set exception.pending before this check, but if it does it
after the check, then KVM goes

	vcpu->arch.exception.pending = events->exception.pending;
	vcpu->arch.exception.vector = events->exception.nr;

which later triggers the WARN. That said, if I you think setting and re-setting
exception.nr is more efficient (as in: racy), I'm all for it.

> For race_events_inj_pen(), it should be sufficient to set the vector just once,
> outside of the loop.  I do think it should be explicitly set, as subtly relying
> on '0' being a valid exception is a bit mean (though it does work).

Sure, I get it.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux