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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 4 Nov 2015, Julien Grall wrote:
> On 04/11/15 11:27, Ian Campbell wrote:
> > On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
> >>>
> >>> we could:
> >>> #ifdef(__XEN__)
> >>> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> >>> #elif !defined(XEN_BUILD_BUG_ON)
> >>> #define XEN_BUILD_BUG_ON(x)
> >>> #endif
> >>> and using XEN_BUILD_BUG_ON in the macro
> >>>
> >>> So, __XEN__ builds get the check and users can opt in by providing
> >>> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the
> >>> check.
> >>
> >> I wouldn't let the user the choice to disable build-time check.
> > 
> > Up to you. Note that "the user" here would potentially include
> > xen.git/tools, and I would expect them to want to define it.
> > 
> > Also...
> > 
> >>  There is
> >> no harm to open-code them as I did today and avoid possible issue in the
> >> code later.
> > 
> > ... there is always a downside to open coding. If you don't want to expose
> > the ability to define a BUG_ON to the application then just drop that #elif
> > from the chain.
> 
> Good point. I will give a look.
> 
> > 
> >>> Or maybe we could just omit this from the public API by one or both of
> >>> a)
> >>> adding an explicit 8 byte type to the union purely to force the size
> >>> and/or
> >>
> >> This is already done in the current version:
> >>
> >>     typedef union { type *p; uint64_aligned_t q; }              \
> >>         __guest_handle_64_ ## name;
> >>
> >> However I don't see how this ensure that the caller of
> >> set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
> >> placeholder.
> > 
> > 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.
> 
> [...]
> 
> >>> 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.

Sorry to be pedantic, but why is this safe given that previously you
wrote:

Finally, based on the defect report #283 [2], the behavior of writing
from one member and reading from another is not clear.

Looks like it might be better to remove the union completely?

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