Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-11-04 at 11:40 +0000, Julien Grall wrote:
> > Not sure I follow. If hnd isn't a suitable struct xen guest handle then
> > other things will fail. If a user is passing a non struct
> > xen_guest_handle
> > to this which happens to contain the same fields then more fool them,
> > and
> > if it happens to be 8 bytes anyway your check won't catch that.
> 
> With the 2 checks in set_xen_raw_guest_handle we catch most of the
> problem. They both ensure that the handle is 8-byte and the pointer is
> valid. However we don't check that the padding is at the beginning of
> the structure.
> 
> It's better than what we have today as we don't even check that the
> handle is 8-byte.

I'm not sure I'm all that worried about callers constructing their own
guest handle structs and getting it wrong.

> [...]
> 
> > > > This looses out on the arm32 hypervisor sanity checking that the
> > > > padding
> > > > bytes are 0 (as required by the ABI) but TBH I haven't checked that
> > > > the
> > > > current version has that property either.
> > > 
> > > It's done during the assignation by the compiler:
> > > 
> > > (hnd).q = (uint64_t)(uintptr_t)(val);
> > 
> > I meant on the reading side.
> 
> It's the responsibility of the caller to zero the padding. There is
> nothing to do on the reading side, the hypervisor will use "p" which
> will be the size of the natural pointer.

For a 32-bit Xen the check would be that a guest was not inadvertently
violating this rule, such a guest would crash if it was run on a 64-bit
hypervisor (which would see the non-zero padding as part of the pointer),
by rejecting such cases on 32-bit Xen we avoid such guests becoming
established and therefore presenting a case for us to relax this rule in
one way or another.

This is the same reason as check_multicall_32bit_clean() exists. multicall
is special only in that it was pretty easy to know where to add that check.

Ian.