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

[Julien Grall <julien@xxxxxxx>]

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