[1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand
diff mbox series

Message ID 20200330192157.1335-2-julien@xen.org
State New
Headers show
Series
  • Fix build with using OCaml 4.06.1 and -safe-string
Related show

Commit Message

Julien Grall March 30, 2020, 7:21 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

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.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/guest_access.h | 2 +-
 xen/include/asm-x86/guest_access.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich March 31, 2020, 7:26 a.m. UTC | #1
On 30.03.2020 21:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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
Julien Grall March 31, 2020, 7:13 p.m. UTC | #2
Hi Jan,

On 31/03/2020 08:26, Jan Beulich wrote:
> On 30.03.2020 21:21, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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,
Jan Beulich April 1, 2020, 6:48 a.m. UTC | #3
On 31.03.2020 21:13, Julien Grall wrote:
> On 31/03/2020 08:26, Jan Beulich wrote:
>> On 30.03.2020 21:21, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> 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?

I don't think so - the idea (aiui) has always been for the Xen
internal object's type to control what gets copied, and hence a
void handle is to be fine for both copy-in and copy-out.

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

Ah, sure.

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

Not very nice indeed, and the conditional expression - at the
first glance being the wrong way round - seems outright
confusing to me. I'll try to find time to experiment some with
this as well, since unless we can find a reasonably neat
solution here, I'm inclined to suggest to leave this as it is
now.

Jan
Julien Grall April 1, 2020, 8:53 a.m. UTC | #4
Hi Jan,

On 01/04/2020 07:48, Jan Beulich wrote:
> On 31.03.2020 21:13, Julien Grall wrote:
>> On 31/03/2020 08:26, Jan Beulich wrote:
>>> On 30.03.2020 21:21, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> 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?
> 
> I don't think so - the idea (aiui) has always been for the Xen
> internal object's type to control what gets copied, and hence a
> void handle is to be fine for both copy-in and copy-out.
> 
>> 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.
> 
> Ah, sure.
> 
>>> 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.
> 
> Not very nice indeed, and the conditional expression - at the
> first glance being the wrong way round - seems outright
> confusing to me.

It is the correct way. If the handle is a void (sizeof(*(hnd).p) == 1), 
then we need to know the size of the Xen buffer so we can compute the 
offset correctly. Otherwise, as we have the correct type, we can apply 
the offset directly and let the compiler do the math for us.

> I'll try to find time to experiment some with
> this as well, since unless we can find a reasonably neat
> solution here, I'm inclined to suggest to leave this as it is
> now.

It is difficult to catch the hypervisor who errorneously write to 
"const" handler. So I would not like a statu-quo.

A slightly uglier version (with more comments) is going to be better 
than what we currently have.

Cheers,
Jan Beulich April 1, 2020, 9:25 a.m. UTC | #5
On 31.03.2020 21:13, Julien Grall wrote:
> 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.

Having looked at how in particular copy_field_to_guest() (which
doesn't have this issue afaict) works, here's an imo much better
alternative:

@@ -87,6 +87,7 @@
 #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
     ((void)((hnd).p == (ptr)));                         \
     raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
 })
@@ -143,6 +144,7 @@ static inline void put_guest_handle(void
 #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
     ((void)((hnd).p == (ptr)));                         \
     __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
 })

Jan
Julien Grall April 1, 2020, 9:10 p.m. UTC | #6
Hi Jan,

On 01/04/2020 10:25, Jan Beulich wrote:
> On 31.03.2020 21:13, Julien Grall wrote:
>> 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.
> 
> Having looked at how in particular copy_field_to_guest() (which
> doesn't have this issue afaict) works, here's an imo much better
> alternative:
> 
> @@ -87,6 +87,7 @@
>   #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
>       const typeof(*(ptr)) *_s = (ptr);                   \
>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    void *__maybe_unused _t = (hnd).p;                  \
>       ((void)((hnd).p == (ptr)));                         \
>       raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
>   })
> @@ -143,6 +144,7 @@ static inline void put_guest_handle(void
>   #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>       const typeof(*(ptr)) *_s = (ptr);                   \
>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    void *__maybe_unused _t = (hnd).p;                  \
>       ((void)((hnd).p == (ptr)));                         \
>       __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
>   })

I actually thought about this one but discarded it because it was using 
unused variable. But I am happy with it, I will have a look to respin 
the patch.

Cheers,

Patch
diff mbox series

diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 8997a1cbfe..ff2eec237d 100644
--- 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));\
 })
diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index ca700c959a..2693c6540b 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -86,7 +86,7 @@ 
  */
 #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));  \
 })