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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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
> > guest
> >
> > 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
> > *dst,
> >                                      offset_bytes, dst_bytes);
> >   }
snip
> > @@ -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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel