Re: [Xen-devel] [PATCH 4/9] x86/pv: Implement pv_hypercall() in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.16 at 13:57, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/08/16 14:12, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +        mov   %rsp, %rdi
>>> +        call  pv_hypercall
>>>          movl  %eax,UREGS_rax(%rsp)       # save the return value
>> To follow the HVM model, this should also move into C.
> 
> Having tried this, I can't.
> 
> Using regs->eax = -ENOSYS; in C results in the upper 32bits of UREGS_rax
> set on the stack, and nothing else re-clobbers this.

I don't understand - why would this need "re-clobbering"? Hypercalls
are assumed to return longs, i.e. full 64 bits of data. Even in case
you say this to just the compat variant, my original comment was of
course meant for both, and in the compat case I don't see why the
upper half of RAX would be of any interest, considering the guest
can't look at it anyway.

> It highlights a second bug present in the hvm side, and propagated to
> the pv side.
> 
> Currently, eax gets truncated when reading out of the registers, before
> it is bounds-checked against NR_hypercalls.  For the HVM side, all this
> does is risk aliasing if upper bits are set, but for the PV side, it
> will cause a failure for a compat guest issuing a hypercall after
> previously receiving an error.

And again I don't understand - when we look at only the low 32 bits,
how would a prior error matter?

> I am proposing the following change to the HVM side to compensate, and
> to leave the asm adjustment of UREGS_rax in place.
> 
> ~Andrew
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e2bb58a..69315d1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4132,11 +4132,11 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>      struct domain *currd = curr->domain;
>      struct segment_register sreg;
>      int mode = hvm_guest_x86_mode(curr);
> -    uint32_t eax = regs->eax;
> +    unsigned long eax = regs->eax;

That would have the potential of breaking 32-bit callers, as we
mustn't rely on the upper halves of registers when coming out of
compat mode. IOW this would need to be made mode dependent,
and even then I'm afraid this has a (however small) potential of
breaking existing callers (but I agree that from a pure bug fix pov
this change should be made for the 64-bit path, as the PV code
looks at the full 64 bits too).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel