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

[Julien Grall <julien.grall@citrix.com>]

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.

Regards,

-- 
Julien Grall