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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-11-04 at 15:19 +0000, Stefano Stabellini wrote:
> 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.

The writer via one is the guest and reader via the other is the hypervisor,
so no matter what they are certainly different compilation units, even in
the face of whole program optimisations.

The concerning issue is that if the compiler can observe you writing to
both halves of the union then it can either omit the first write or dive
off into deep undefined behaviour territory.

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

If that were possible I think someone would have suggested a scheme which
achieves it within the other constraints.

Ian.

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