[v8,for-4.9,3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

Commit Message

Jennifer Herbert April 21, 2017, 2:05 p.m. UTC

From: Andrew Cooper <[e-mail]>

This also allows the usual cases to be simplified, by omitting an unnecessary
buf parameters, and because the macros can appropriately size the object.

Signed-off-by: Andrew Cooper <[e-mail]>
Signed-off-by: Jennifer Herbert <[e-mail]>
--
CC: Paul Durrant <[e-mail]>
CC: Andrew Cooper <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Julien Grall <[e-mail]>
---
Rebased.
---
 xen/arch/x86/hvm/dm.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

Comments

Paul Durrant April 21, 2017, 2:11 p.m. UTC | #1

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> Sent: 21 April 2017 15:06
> To: Xen-devel <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>; Jennifer Herbert
> <[e-mail]>; Paul Durrant <[e-mail]>; Jan
> Beulich <[e-mail]>; Julien Grall <[e-mail]>
> Subject: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to,
> from}_guest_buf() in terms of raw accessors
> 
> From: Andrew Cooper <[e-mail]>
> 
> This also allows the usual cases to be simplified, by omitting an unnecessary
> buf parameters, and because the macros can appropriately size the object.
> 
> Signed-off-by: Andrew Cooper <[e-mail]>
> Signed-off-by: Jennifer Herbert <[e-mail]>

Reviewed-by: Paul Durrant <[e-mail]>

> --
> CC: Paul Durrant <[e-mail]>
> CC: Andrew Cooper <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Julien Grall <[e-mail]>
> ---
> Rebased.
> ---
>  xen/arch/x86/hvm/dm.c | 43 ++++++++++++++++++++++++++---------------
> --
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 89186d2..b31c252 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,38 +32,47 @@ struct dmop_args {
>      struct xen_dm_op_buf buf[2];
>  };
> 
> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> -                                unsigned int nr_bufs, void *dst,
> -                                unsigned int idx, size_t dst_size)
> +static bool _raw_copy_from_guest_buf(void *dst,
> +                                     const struct dmop_args *args,
> +                                     unsigned int buf_idx,
> +                                     size_t dst_bytes)
>  {
>      size_t buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    buf_bytes = bufs[idx].size;
> -    if ( dst_size > buf_bytes )
> +    buf_bytes =  args->buf[buf_idx].size;
> +
> +    if ( dst_bytes > buf_bytes )
>          return false;
> 
> -    return !copy_from_guest(dst, bufs[idx].h, dst_size);
> +    return !copy_from_guest(dst, args->buf[buf_idx].h, dst_bytes);
>  }
> 
> -static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
> -                              unsigned int nr_bufs, unsigned int idx,
> -                              const void *src, size_t src_size)
> +static bool _raw_copy_to_guest_buf(const struct dmop_args *args,
> +                                   unsigned int buf_idx,
> +                                   const void *src, size_t src_bytes)
>  {
>      size_t buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    buf_bytes = bufs[idx].size;
> -    if ( src_size > buf_bytes )
> +    buf_bytes = args->buf[buf_idx].size;
> +
> +    if ( src_bytes > buf_bytes )
>          return false;
> 
> -    return !copy_to_guest(bufs[idx].h, src, src_size);
> +    return !copy_to_guest(args->buf[buf_idx].h, src, src_bytes);
>  }
> 
> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
> +
> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
> +
>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>                              unsigned int nr, const struct xen_dm_op_buf *buf)
>  {
> @@ -314,7 +323,7 @@ static int dm_op(const struct dmop_args *op_args)
>      if ( rc )
>          goto out;
> 
> -    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0,
> sizeof(op)) )
> +    if ( !COPY_FROM_GUEST_BUF(op, op_args, 0) );
>      {
>          rc = -EFAULT;
>          goto out;
> @@ -570,8 +579,8 @@ static int dm_op(const struct dmop_args *op_args)
>      }
> 
>      if ( (!rc || rc == -ERESTART) &&
> -         !const_op &&
> -         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op,
> sizeof(op)) )
> +         !const_op && !COPY_TO_GUEST_BUF(op_args, 0, op) )
> +
>          rc = -EFAULT;
> 
>   out:
> --
> 2.1.4

Jan Beulich April 21, 2017, 3:45 p.m. UTC | #2

>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
> +
> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))

Why all caps all of the sudden? With this undone
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Andrew Cooper April 21, 2017, 4:10 p.m. UTC | #3

On 21/04/17 16:45, Jan Beulich wrote:
>>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>> +
>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
> Why all caps all of the sudden?

This is the start of some code improvements, given the fallout from XSA-212.

This macro is not a C function and doesn't behave like one
(specifically, capturing src by name rather than value).  Therefore, it
gets ALL_CAPS() (which is actually traditional for any macro in C) to
make it more obvious to people reading the code that it *is not* a C
function and doesn't behave like one.

It is getting embarrassing how many security vulnerability we are seeing
because macros look like they are doing one thing, yet actually do
something else, and improving the quality of the code is the only way
this is going to get better.

Therefore, I am going to insist that they stay like this.

~Andrew

Jan Beulich April 24, 2017, 8:19 a.m. UTC | #4

>>> On 21.04.17 at 18:10, <[e-mail]> wrote:
> On 21/04/17 16:45, Jan Beulich wrote:
>>>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
>>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>>> +
>>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))

(Side note: src also isn't properly parenthesized, and the title went
out of sync with the implementation.)

>> Why all caps all of the sudden?
> 
> This is the start of some code improvements, given the fallout from XSA-212.

I don't think making the names shout is an improvement in any way.
The #define-s above may still look fine, but the code using them is
now looking plain ugly (even more so with the yet longer names
introduced in patch 4).

> This macro is not a C function and doesn't behave like one
> (specifically, capturing src by name rather than value).  Therefore, it
> gets ALL_CAPS() (which is actually traditional for any macro in C) to

I disagree - manifest constants may indeed be traditionally all
caps, but not macros. Just look in the Library section of the C
standard. In various cases it's not even enforced whether a
given identifier is a macro or a variable/function.

> make it more obvious to people reading the code that it *is not* a C
> function and doesn't behave like one.
> 
> It is getting embarrassing how many security vulnerability we are seeing
> because macros look like they are doing one thing, yet actually do
> something else, and improving the quality of the code is the only way
> this is going to get better.

Considering the "how many" you use, mind giving three examples
where using all caps macro names would have made a difference?
I sincerely doubt that the case used in identifiers would make a
whole lot of a difference.

> Therefore, I am going to insist that they stay like this.

Well, how about I insist on names being made consistent again
with what we have elsewhere, as they were in earlier versions?
My conditional R-b stands in any event (plus the correction of
the parenthesization issue mentioned above, also for patch 4).

As a possible alternative, was it considered to pass pointers
here as before, using __builtin_object_size() on them instead of
the sizeof() above, and making the macros inline functions?

Jan

Andrew Cooper April 25, 2017, 8:03 p.m. UTC | #5

On 24/04/17 09:19, Jan Beulich wrote:
>>>> On 21.04.17 at 18:10, <[e-mail]> wrote:
>> On 21/04/17 16:45, Jan Beulich wrote:
>>>>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
>>>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>>>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>>>> +
>>>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>>>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
> (Side note: src also isn't properly parenthesized, and the title went
> out of sync with the implementation.)
>
>>> Why all caps all of the sudden?
>> This is the start of some code improvements, given the fallout from XSA-212.
> I don't think making the names shout is an improvement in any way.
> The #define-s above may still look fine, but the code using them is
> now looking plain ugly (even more so with the yet longer names
> introduced in patch 4).

That is a matter of opinion which I don't share, but ok.

As an alternative, how else do you suggest making it obvious to the
reader of the code that this thing which looks like a function doesn't
have function semantics?  (This is the purpose I am trying to get across.)

>> make it more obvious to people reading the code that it *is not* a C
>> function and doesn't behave like one.
>>
>> It is getting embarrassing how many security vulnerability we are seeing
>> because macros look like they are doing one thing, yet actually do
>> something else, and improving the quality of the code is the only way
>> this is going to get better.
> Considering the "how many" you use, mind giving three examples
> where using all caps macro names would have made a difference?
> I sincerely doubt that the case used in identifiers would make a
> whole lot of a difference.

You have missed my point then.  We have many security vulnerabilities
because we have deceptive code, and fix for that is to prevent the code
being deceptive.  This is going to positive code quality effort on our
behalf, because the status quo is currently terrible.

Most notably, XSA-212 just gone, where the root of the vulnerability is
that "guest_handle_okay(base_ptr, array_element)" doesn't consider its
second parameter, and degrades to checking just base_ptr.

I accept that, in this case, capitalising the macro wouldn't help, but
that is because its deceptive nature is in its naming, not because it
behaves in a way contrary to a C function.

> As a possible alternative, was it considered to pass pointers
> here as before, using __builtin_object_size() on them instead of
> the sizeof() above, and making the macros inline functions?

I have never tried using it in anger, but looking into it, it degrades
to (size_t)-1 in the case the compiler can't statically work out the
correct value.  As a result, you'd end up with a function which has
gets() semantics (in the case that the compiler can't work out what's
going on).  I don't recommend we use any constructs like this.

~Andrew

Jan Beulich April 26, 2017, 7:37 a.m. UTC | #6

>>> On 25.04.17 at 22:03, <[e-mail]> wrote:
> On 24/04/17 09:19, Jan Beulich wrote:
>>>>> On 21.04.17 at 18:10, <[e-mail]> wrote:
>>> On 21/04/17 16:45, Jan Beulich wrote:
>>>>>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
>>>>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>>>>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>>>>> +
>>>>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>>>>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
>> (Side note: src also isn't properly parenthesized, and the title went
>> out of sync with the implementation.)
>>
>>>> Why all caps all of the sudden?
>>> This is the start of some code improvements, given the fallout from XSA-212.
>> I don't think making the names shout is an improvement in any way.
>> The #define-s above may still look fine, but the code using them is
>> now looking plain ugly (even more so with the yet longer names
>> introduced in patch 4).
> 
> That is a matter of opinion which I don't share, but ok.
> 
> As an alternative, how else do you suggest making it obvious to the
> reader of the code that this thing which looks like a function doesn't
> have function semantics?  (This is the purpose I am trying to get across.)

I do understand the intention, but I don't think capitalization of the
names helps this in any way. This is not the least because there are
compiler builtins which don't really have function semantics either
(e.g. __builtin_constant_p(), __builtin_expect(),
__builtin_types_compatible_p(), or __builtin_unreachable()). You
won't be able to do anything about these, other than perhaps
encapsulating them wrapper macros, but that still won't make the
compiler mandated name all caps.

Or take offsetof(), which following your argumentation then should
be - contrary to the C standard - OFFSETOF(), while what we have
as ASSERT() should then be - in line with the C standard, albeit our
macro has slightly different behavior - assert().

>>> make it more obvious to people reading the code that it *is not* a C
>>> function and doesn't behave like one.
>>>
>>> It is getting embarrassing how many security vulnerability we are seeing
>>> because macros look like they are doing one thing, yet actually do
>>> something else, and improving the quality of the code is the only way
>>> this is going to get better.
>> Considering the "how many" you use, mind giving three examples
>> where using all caps macro names would have made a difference?
>> I sincerely doubt that the case used in identifiers would make a
>> whole lot of a difference.
> 
> You have missed my point then.  We have many security vulnerabilities
> because we have deceptive code, and fix for that is to prevent the code
> being deceptive.  This is going to positive code quality effort on our
> behalf, because the status quo is currently terrible.
> 
> Most notably, XSA-212 just gone, where the root of the vulnerability is
> that "guest_handle_okay(base_ptr, array_element)" doesn't consider its
> second parameter, and degrades to checking just base_ptr.
> 
> I accept that, in this case, capitalising the macro wouldn't help, but
> that is because its deceptive nature is in its naming, not because it
> behaves in a way contrary to a C function.

A function could easily (and - by default - without warning) ignore
one or more of the arguments passed to it, too.

And as you say - XSA-212 is not an suitable example for your
argumentation here. Once again - I don't think name capitalization
helps in any way in what you want to ensure. And no, I don't think
I've missed your point here, I'm merely questioning the measure
you want to take to address your concerns. Therefore I'm still
lacking any examples of XSAs supporting your position regarding
identifier naming.

>> As a possible alternative, was it considered to pass pointers
>> here as before, using __builtin_object_size() on them instead of
>> the sizeof() above, and making the macros inline functions?
> 
> I have never tried using it in anger, but looking into it, it degrades
> to (size_t)-1 in the case the compiler can't statically work out the
> correct value.  As a result, you'd end up with a function which has
> gets() semantics (in the case that the compiler can't work out what's
> going on).  I don't recommend we use any constructs like this.

Well, I wouldn't rule it out without a little bit of experimenting.
While it can't be used in BUILD_BUG_ON(), it can very well be used
in ways similar to e.g. {read,write}_atomic(), adding a reference
to an always undefined symbol in the case the builtin produces 0
or -1.

On the grounds of wanting to not delay this series any further,
I'm about to commit it (albeit I note patch 2 is lacking a proper
maintainer ack, and I'm not going to add mine), but I'll make my
disagreement with the choice of macro names explicit, so these
names can't later be used as a reference to support naming
other macros in similarly ugly ways.

Jan

Jan Beulich April 26, 2017, 7:46 a.m. UTC | #7

>>> On 21.04.17 at 16:05, <[e-mail]> wrote:
> @@ -314,7 +323,7 @@ static int dm_op(const struct dmop_args *op_args)
>      if ( rc )
>          goto out;
>  
> -    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) )
> +    if ( !COPY_FROM_GUEST_BUF(op, op_args, 0) );

Btw, I also had to remove the bogus semicolon here, causing a build
failure, and ...

> @@ -570,8 +579,8 @@ static int dm_op(const struct dmop_args *op_args)
>      }
>  
>      if ( (!rc || rc == -ERESTART) &&
> -         !const_op &&
> -         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) )
> +         !const_op && !COPY_TO_GUEST_BUF(op_args, 0, op) )
> +
>          rc = -EFAULT;

... I've taken the liberty to remove this stray blank line.

Both are likely a sign of this having been put together in too much
of a hurry, and the former suggests this version hasn't been tested
at all.

Jan

Patch

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 89186d2..b31c252 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -32,38 +32,47 @@  struct dmop_args {
     struct xen_dm_op_buf buf[2];
 };
 
-static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
-                                unsigned int nr_bufs, void *dst,
-                                unsigned int idx, size_t dst_size)
+static bool _raw_copy_from_guest_buf(void *dst,
+                                     const struct dmop_args *args,
+                                     unsigned int buf_idx,
+                                     size_t dst_bytes)
 {
     size_t buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    buf_bytes = bufs[idx].size;
-    if ( dst_size > buf_bytes )
+    buf_bytes =  args->buf[buf_idx].size;
+
+    if ( dst_bytes > buf_bytes )
         return false;
 
-    return !copy_from_guest(dst, bufs[idx].h, dst_size);
+    return !copy_from_guest(dst, args->buf[buf_idx].h, dst_bytes);
 }
 
-static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
-                              unsigned int nr_bufs, unsigned int idx,
-                              const void *src, size_t src_size)
+static bool _raw_copy_to_guest_buf(const struct dmop_args *args,
+                                   unsigned int buf_idx,
+                                   const void *src, size_t src_bytes)
 {
     size_t buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    buf_bytes = bufs[idx].size;
-    if ( src_size > buf_bytes )
+    buf_bytes = args->buf[buf_idx].size;
+
+    if ( src_bytes > buf_bytes )
         return false;
 
-    return !copy_to_guest(bufs[idx].h, src, src_size);
+    return !copy_to_guest(args->buf[buf_idx].h, src, src_bytes);
 }
 
+#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
+    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
+
+#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
+    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
+
 static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
                             unsigned int nr, const struct xen_dm_op_buf *buf)
 {
@@ -314,7 +323,7 @@  static int dm_op(const struct dmop_args *op_args)
     if ( rc )
         goto out;
 
-    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) )
+    if ( !COPY_FROM_GUEST_BUF(op, op_args, 0) );
     {
         rc = -EFAULT;
         goto out;
@@ -570,8 +579,8 @@  static int dm_op(const struct dmop_args *op_args)
     }
 
     if ( (!rc || rc == -ERESTART) &&
-         !const_op &&
-         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) )
+         !const_op && !COPY_TO_GUEST_BUF(op_args, 0, op) )
+
         rc = -EFAULT;
 
  out: