Re: [[PATCH v3]] xen/guest_access: Harden *copy_to_guest_offset() to prevent const dest operand

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2020 19:16, Julien Grall wrote:
> On 16/04/2020 13:19, Jan Beulich wrote:
>> On 16.04.2020 13:24, 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.
>>>
>>> There is no easy way to check whether a const is NULL in C99. The
>>> approach used is to introduce an unused variable that is non-const and
>>> assign the handle. If the handle were const, this would fail at build
>>> because without an explicit cast, it is not possible to assign a const
>>> variable to a non-const variable.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> with one further remark:
>>
>>> --- a/xen/include/asm-x86/guest_access.h
>>> +++ b/xen/include/asm-x86/guest_access.h
>>> @@ -87,6 +87,8 @@
>>>   #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
>>>       const typeof(*(ptr)) *_s = (ptr);                   \
>>>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>>> +    /* Check if the handle is not const */              \
>>> +    void *__maybe_unused _t = (hnd).p;                  \
>>
>> Not being a native speaker, to me "if" doesn't look appropriate
>> here. I'd use "that" instead, but you may want to confirm this.
>> Overall then maybe "Check that the handle is not for a const type"?
> 
> I am happy with the new suggestion. I will fixup while comitting it.
> 
> 
> I would also need a review from Stefano here also.

Would you, even under the new rules? In which case it might be
a good idea to Cc him (now done here), also to given him a more
direct means to object. Same would go for the x86 reviewers ...
All of them were Cc-ed on v2. In light of this I can't sensibly
ask that you please commit this patch soon, so that I can put
mine on top, I guess (this was the original intention when
starting to write this reply).

Jan