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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ross Lagerwall [mailto:ross.lagerwall@xxxxxxxxxx]
> Sent: 09 February 2018 15:49
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>
> Subject: Re: [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
> >> 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.

Yeah, I know. Just thought it might look neater overall, but doesn't matter.

  Paul
 
> --
> Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel