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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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.

Cheers,

--
Julien Grall