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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 06/04/2020 12:01, Jan Beulich wrote:
> On 04.04.2020 15:06, Julien Grall wrote:
>> From: Julien Grall <jgrall@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.
>>
>> 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@suse.com>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> I'm not convinced it is a good idea to add (recurring) comments
> like you do - there are more aspects of these macros that would
> be worth commenting on, and commenting on some but not all may
> give the wrong impression of all subtleties being covered (also
> for others).

I thought you would say that, but I don't think I am the best person to 
describe all the other subtetly of the code. Yet I didn't want to not 
comment the oddity of using a maybe_unused variable.

> 
> In any event I'd like to ask that each header gain such a
> comment only once, with the other being a tiny reference to the
> one "complete" instance.

I am not entirely sure how this would look like. We would need to rely 
on _t having the same meaning across all the headers. This is quite easy 
to miss during review, so my preference still sticks to multiple comments.

Although I can reduce the size of the comment to one on top of the 
definition of _t. Something like: "Check if the handler is not const".

Cheers,

-- 
Julien Grall