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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2020 11:01, Julien Grall wrote:
> Hi Jan,
> 
> On 06/04/2020 12:01, Jan Beulich wrote:
>> On 04.04.2020 15:06, 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>
>>
>> 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.

Well, to me the "__maybe_unused" is telling enough.

>> 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.

Well, I did say "each header" exactly because of this.

> 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".

Ah yes, that would seem better (with s/handler/handle/ of course).

Jan