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

Re: [Xen-devel] [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest

On 02/09/2018 03:42 PM, Paul Durrant wrote:
-----Original Message-----
From: Ross Lagerwall [mailto:ross.lagerwall@xxxxxxxxxx]
Sent: 09 February 2018 15:34
To: xen-devel@xxxxxxxxxxxxx
Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>; Jan Beulich
<jbeulich@xxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul
Durrant <Paul.Durrant@xxxxxxxxxx>
Subject: [PATCH] x86/hvm/dmop: Only copy what is needed to/from the

dm_op() fails with -EFAULT if the struct xen_dm_op given by the guest is
smaller than Xen's struct xen_dm_op. This is a problem because DMOP is
meant to be a stable ABI but it breaks whenever the size of struct
xen_dm_op changes.

To fix this, change how the copying to and from the guest is done. When
copying from the guest, first copy the header and inspect the op. Then,
only copy the correct amount needed for that op. When copying to the
guest, don't copy the header. Rather, copy only the correct amount
needed for that particular op.

So now the dm_op() will fail if the guest does not supply enough bytes
for the specific op. It will not fail if the guest supplies too many
bytes for the specific op, but Xen will not copy the extra bytes.

Remove some now unused macros and helper functions.

Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>

I'm sure that was how it was supposed to work originally but it got lost 
somewhere along the way.

Originally it zeroed struct xen_dm_op and then copied whatever the guest provided (up to the size of struct xen_dm_op) which is similar to this but not quite the same.

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>, with one suggestion...

  xen/arch/x86/hvm/dm.c | 78 +++++++++++++++++++++++++++--------------
  1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 8083ded..6c7276c 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -54,42 +54,10 @@ static bool _raw_copy_from_guest_buf_offset(void
                                     offset_bytes, dst_bytes);

@@ -372,6 +340,28 @@ static int dm_op(const struct dmop_args *op_args)
      struct xen_dm_op op;
      bool const_op = true;
      long rc;
+    size_t offset;
+    static const uint8_t op_size[] = {

Given the correspondence between the op code name and the struct name, it might 
be worth using a macro to help with this array declaration.

Probably... there are a couple of exceptions to the correspondence though.

Ross Lagerwall

Xen-devel mailing list



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