On 31.03.2020 21:13, Julien Grall wrote:
> On 31/03/2020 08:26, Jan Beulich wrote:
>> On 30.03.2020 21:21, Julien Grall wrote:
>>> From: Julien Grall <jgr...@amazon.com>
>>>
>>> At the moment, copy_to_guest_offset() will allow the hypervisor to copy
>>> data to guest handle marked const.
>>>
>>> Thankfully, no users of the helper will do that. Rather than hoping this
>>> can be caught during review, harden copy_to_guest_offset() so the build
>>> will fail if such users are introduced.
>>
>> But there are other implications you break:
>>
>>> --- a/xen/include/asm-arm/guest_access.h
>>> +++ b/xen/include/asm-arm/guest_access.h
>>> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d,
>>> paddr_t ipa, void *buf,
>>> #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
>>> const typeof(*(ptr)) *_s = (ptr); \
>>> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
>>> + typeof(*((hnd).p)) *_d = (hnd).p; \
>>> ((void)((hnd).p == (ptr))); \
>>> __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
>>
>> Until this change, it is "ptr" which all sizes get derived from,
>> i.e. it is the internally used type rather than the handle type
>> which controls this. I'm sure we use this in a few places, to
>> copy to e.g. a handle derived from "void". Compatibility of types
>> (disallowing other than void) is checked by the comparison on the
>> line immediately after the line you change. Yes "_d+(off)" right
>> above here then changes its result. I consider it pretty likely
>> you'd notice this issue once you go beyond just build testing.
>
> I missed that part. To be honest, it feels wrong to me to have
> "off" != 0 and use a void type for the handle. Would it make
> sense to forbid it?
I don't think so - the idea (aiui) has always been for the Xen
internal object's type to control what gets copied, and hence a
void handle is to be fine for both copy-in and copy-out.
> As a side node, I have updated __copy_to_guest_offset() but
> forgot to update copy_to_guest_offset(). I will look to apply
> the modifications we agree on both side.
Ah, sure.
>> To address this, I guess we need to find an expression along the
>> lines of that comparison, which does not cause any code to be
>> generated, but which verifies the properties we care about. The
>> line you change should be left alone, from all I can tell right
>> now.
>
> I am not aware of any way before C11 to check if a variable is
> const or not. If we wanted to keep allow void type the handle
> then a possible approach would be:
>
> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> const typeof(*(ptr)) *_s = (ptr); \
> typeof(*((hnd).p)) *_d = (hnd).p; \
> size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s); \
> ((void)((hnd).p == (ptr))); \
> raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr)); \
> })
>
> I don't particularly like it but I could not come up with better so far.
Not very nice indeed, and the conditional expression - at the
first glance being the wrong way round - seems outright
confusing to me. I'll try to find time to experiment some with
this as well, since unless we can find a reasonably neat
solution here, I'm inclined to suggest to leave this as it is
now.
Jan
