[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


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!


Julien Grall



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