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

[Sean Christopherson <seanjc@google.com>]

On Tue, Aug 15, 2023, Michal Luczaj wrote:
> On 8/15/23 02:48, Sean Christopherson wrote:
> > ...
> > Argh, apparently I didn't run these on AMD.  The exception injection test hangs
> > because the vCPU hits triple fault shutdown, and because the VMCB is technically
> > undefined on shutdown, KVM synthesizes INIT.  That starts the vCPU at the reset
> > vector and it happily fetches zeroes util being killed.
> 
> Thank you for getting this. I should have mentioned, due to lack of access to
> AMD hardware, I've only tested on Intel.
> 
> > @@ -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.

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).