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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2020 10:10, Julien Grall wrote:
> Hi,
> 
> On 23/04/2020 08:38, Jan Beulich wrote:
>> 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?
> 
> "2. In unusual circumstances, a more general maintainer's Ack can stand
> in for or even overrule a specific maintainer's Ack.  Unusual
> circumstances might include:
> 
>  - The more specific maintainer has not responded either to the
>  original patch, nor to "pings", within a reasonable amount of time.
> "
> 
> So it depends on your definition of "reasonable amount of time".
> A week or two seems reasonable to me for non-pressing issues.

No, this isn't the part I was referring to - there are no unusual
circumstances here. Quite a bit further up you'll in particular
find

"In a case where a maintainer themselves submits a patch, the
 Signed-off-by meets the approval requirement (#1); so a Review
 from anyone in the community suffices for requirement #2."

Jan