Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 31/03/2020 08:26, Jan Beulich wrote:
> On 30.03.2020 21:21, 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.
> But there are other implications you break:
>
> > --- a/xen/include/asm-arm/guest_access.h
> > +++ b/xen/include/asm-arm/guest_access.h
> > @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t 
> > ipa, void *buf,
> >   
> >   #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> >       const typeof(*(ptr)) *_s = (ptr);                   \
> > -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> > +    typeof(*((hnd).p)) *_d = (hnd).p;                   \
> >       ((void)((hnd).p == (ptr)));                         \
> >       __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> Until this change, it is "ptr" which all sizes get derived from,
> i.e. it is the internally used type rather than the handle type
> which controls this. I'm sure we use this in a few places, to
> copy to e.g. a handle derived from "void". Compatibility of types
> (disallowing other than void) is checked by the comparison on the
> line immediately after the line you change. Yes "_d+(off)" right
> above here then changes its result. I consider it pretty likely
> you'd notice this issue once you go beyond just build testing.
I missed that part. To be honest, it feels wrong to me to have "off" != 
0 and use a void type for the handle. Would it make sense to forbid it?
As a side node, I have updated __copy_to_guest_offset() but forgot to 
update copy_to_guest_offset(). I will look to apply the modifications we 
agree on both side.
> To address this, I guess we need to find an expression along the
> lines of that comparison, which does not cause any code to be
> generated, but which verifies the properties we care about. The
> line you change should be left alone, from all I can tell right
> now.
I am not aware of any way before C11 to check if a variable is const or 
not. If we wanted to keep allow void type the handle then a possible 
approach would be:
#define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
    const typeof(*(ptr)) *_s = (ptr);                           \
    typeof(*((hnd).p)) *_d = (hnd).p;                           \
    size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
    ((void)((hnd).p == (ptr)));                                 \
    raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
})

I don't particularly like it but I could not come up with better so far.

Cheers,

--
Julien Grall