Re: [PATCH 1/2] KVM: x86: Fix KVM_CAP_SYNC_REGS's sync_regs() TOCTOU issues

[Sean Christopherson <seanjc@google.com>]

On Tue, Aug 01, 2023, Michal Luczaj wrote:
> On 8/1/23 01:49, Sean Christopherson wrote:
> >> A note: when servicing kvm_run->kvm_dirty_regs, changes made by
> >> __set_sregs()/kvm_vcpu_ioctl_x86_set_vcpu_events() to on-stack copies of
> >> vcpu->run.s.regs will not be reflected back in vcpu->run.s.regs. Is this
> >> ok?
> > 
> > I would be amazed if anyone cares.  Given the justification and the author,
> > 
> >     This reduces ioctl overhead which is particularly important when userspace
> >     is making synchronous guest state modifications (e.g. when emulating and/or
> >     intercepting instructions).
> >     
> >     Signed-off-by: Ken Hofsass <hofsass@google.com>
> > 
> > I am pretty sure this was added to optimize a now-abandoned Google effort to do
> > emulation in uesrspace.  I bring that up because I was going to suggest that we
> > might be able to get away with a straight revert, as QEMU doesn't use the flag
> > and AFAICT neither does our VMM, but there are a non-zero number of hits in e.g.
> > github, so sadly I think we're stuck with the feature :-(
> 
> All right, so assuming the revert is not happening and the API is not misused
> (i.e. unless vcpu->run->kvm_valid_regs is set, no one is expecting up to date
> values in vcpu->run->s.regs), is assignment copying
> 
> 	struct kvm_vcpu_events events = vcpu->run->s.regs.events;
> 
> the right approach or should it be a memcpy(), like in ioctl handlers?

Both approaches are fine, though I am gaining a preference for the copy-by-value
method.  With gcc-12 and probably most compilers, the code generation is identical
for both as the compiler generates a call to memcpy() to handle the the struct
assignment.

The advantage of copy-by-value for structs, and why I think I now prefer it, is
that it provides type safety.  E.g. this compiles without complaint

	memcpy(&events, &vcpu->run->s.regs.sregs, sizeof(events));

whereas this

	struct kvm_vcpu_events events = vcpu->run->s.regs.sregs;

yields

  arch/x86/kvm/x86.c: In function ‘sync_regs’:
  arch/x86/kvm/x86.c:11793:49: error: invalid initializer
  11793 |                 struct kvm_vcpu_events events = vcpu->run->s.regs.sregs;
        |                                                 ^~~~

The downside is that it's less obvious when reading the code that there is a
large-ish memcpy happening, but IMO it's worth gaining the type safety.