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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

Jan