Re: [PATCH V3 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/28/2015 06:25 PM, Jan Beulich wrote:
>>>> On 28.09.15 at 12:16, <rcojocaru@bitdefender.com> wrote:
>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> +    v->arch.user_regs.eax = rsp->data.regs.x86.rax;
>> +    v->arch.user_regs.ebx = rsp->data.regs.x86.rbx;
>> +    v->arch.user_regs.ecx = rsp->data.regs.x86.rcx;
>> +    v->arch.user_regs.edx = rsp->data.regs.x86.rdx;
>> +    v->arch.user_regs.esp = rsp->data.regs.x86.rsp;
>> +    v->arch.user_regs.ebp = rsp->data.regs.x86.rbp;
>> +    v->arch.user_regs.esi = rsp->data.regs.x86.rsi;
>> +    v->arch.user_regs.edi = rsp->data.regs.x86.rdi;
>> +
>> +    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
>> +    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
>> +    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
>> +    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
>> +    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
>> +    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
>> +    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
>> +    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
>> +
>> +    v->arch.user_regs.eflags = rsp->data.regs.x86.rflags;
> 
> Shouldn't you sanitize the value? I can't immediately see anything
> putting Xen at risk (but it also doesn't seem impossible that I'm
> overlooking something), but surely putting insane values here
> can lead to hard to debug guest crashes.
> 
>> +    v->arch.user_regs.eip = rsp->data.regs.x86.rip;
> 
> Similarly here I wonder whether this shouldn't be at least
> range checked.

I'm assuming that the userspace vm_event client application will use the
interface wisely. A typical scenario would be:

- vm_event request received;
- reply.registers = request.registers;
- if this event requires setting registers, set only relevant ones;
- send the reply to the hypervisor (with the set registers flag).

So with this usage it shouldn't be possible to accidentally send garbage
back.

>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -417,6 +417,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>>  
>>          if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>>          {
>> +            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>> +                vm_event_set_registers(v, &rsp);
>> +
>>              if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>>                  vm_event_toggle_singlestep(d, v);
>  
> What meaning has setting both flags and EFLAGS.TF in the new
> register values?

That's a good question. I'm not sure how that would affect an attached
debugger type scenario.

I'm also unsure of what a good solution to this issue would be. I could
make the flags exclusive, but that would prevent, for example, setting
EAX and singlestepping, which should not be a problem. I could also
remove the eflags assignment from vm_event_set_registers() (maybe
replace it with a comment).

Tamas, do you need eflags set? What do you think? Again, I'm happy with
just setting EIP, what scenarios are you interested in?


Thanks,
Razvan