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

[Julien Grall <julien@xxxxxxx>]

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.
> In which case it might be
> a good idea to Cc him (now done here), also to given him a more
> direct means to object. Same would go for the x86 reviewers ...
> All of them were Cc-ed on v2. In light of this I can't sensibly
> ask that you please commit this patch soon, so that I can put
> mine on top, I guess (this was the original intention when
> starting to write this reply).
Ah that's why he didn't responded... I thought I CCed him but it seems I 
didn't call scripts/add_maintainers.pl before sending the patch.
Thank you for adding the correct CC!

Cheers,

--
Julien Grall