Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jgrall@xxxxxxxxxx>
>>>
>>> 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