[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 09:16, Jan Beulich wrote:
On 23.04.2020 10:10, Julien Grall wrote:

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.

No, this isn't the part I was referring to - there are no unusual
circumstances here. Quite a bit further up you'll in particular

"In a case where a maintainer themselves submits a patch, the
  Signed-off-by meets the approval requirement (#1); so a Review
  from anyone in the community suffices for requirement #2."T

This is the general rule that we haven't followed on Arm so far. In any case this is followed by:

"Before a maintainer checks in their own patch with another community
member's R-b but no co-maintainer Ack, it is especially important to
give their co-maintainer opportunity to give feedback, perhaps
declaring their intention to check it in without their co-maintainers
ack a day before doing so."

So let's give a couple of days for Stefano to object.


Julien Grall



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