[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.