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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/02/18 15:42, 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.
>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>, with one suggestion...

This needs backporting to 4.9(?) and later.

>
>> ---
>>  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);
>>  }
>>
>> -static bool _raw_copy_to_guest_buf_offset(const struct dmop_args *args,
>> -                                          unsigned int buf_idx,
>> -                                          size_t offset_bytes,
>> -                                          const void *src,
>> -                                          size_t src_bytes)
>> -{
>> -    size_t buf_bytes;
>> -
>> -    if ( buf_idx >= args->nr_bufs )
>> -        return false;
>> -
>> -    buf_bytes = args->buf[buf_idx].size;
>> -
>> -
>> -    if ( (offset_bytes + src_bytes) < offset_bytes ||
>> -         (offset_bytes + src_bytes) > buf_bytes )
>> -        return false;
>> -
>> -    return !copy_to_guest_offset(args->buf[buf_idx].h, offset_bytes,
>> -                                 src, src_bytes);
>> -}
>> -
>>  #define COPY_FROM_GUEST_BUF_OFFSET(dst, bufs, buf_idx,
>> offset_bytes) \
>>      _raw_copy_from_guest_buf_offset(&(dst), bufs, buf_idx, offset_bytes, \
>>                                      sizeof(dst))
>>
>> -#define COPY_TO_GUEST_BUF_OFFSET(bufs, buf_idx, offset_bytes, src) \
>> -    _raw_copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, \
>> -                                  &(src), sizeof(src))
>> -
>> -#define COPY_FROM_GUEST_BUF(dst, bufs, buf_idx) \
>> -    COPY_FROM_GUEST_BUF_OFFSET(dst, bufs, buf_idx, 0)
>> -
>> -#define COPY_TO_GUEST_BUF(bufs, buf_idx, src) \
>> -    COPY_TO_GUEST_BUF_OFFSET(bufs, buf_idx, 0, src)
>> -
>>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>>                              unsigned int nr, const struct xen_dm_op_buf 
>> *buf)
>>  {
>> @@ -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.

They aren't as 1:1 as you'd expect, and mixing&matching macros is going
to be far more obscure to read than this.

>
>   Paul
>
>> +        [XEN_DMOP_create_ioreq_server]              = sizeof(struct
>> xen_dm_op_create_ioreq_server),
>> +        [XEN_DMOP_get_ioreq_server_info]            = sizeof(struct
>> xen_dm_op_get_ioreq_server_info),
>> +        [XEN_DMOP_map_io_range_to_ioreq_server]     = sizeof(struct
>> xen_dm_op_ioreq_server_range),
>> +        [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct
>> xen_dm_op_ioreq_server_range),
>> +        [XEN_DMOP_set_ioreq_server_state]           = sizeof(struct
>> xen_dm_op_set_ioreq_server_state),
>> +        [XEN_DMOP_destroy_ioreq_server]             = sizeof(struct
>> xen_dm_op_destroy_ioreq_server),
>> +        [XEN_DMOP_track_dirty_vram]                 = sizeof(struct
>> xen_dm_op_track_dirty_vram),
>> +        [XEN_DMOP_set_pci_intx_level]               = sizeof(struct
>> xen_dm_op_set_pci_intx_level),
>> +        [XEN_DMOP_set_isa_irq_level]                = sizeof(struct
>> xen_dm_op_set_isa_irq_level),
>> +        [XEN_DMOP_set_pci_link_route]               = sizeof(struct
>> xen_dm_op_set_pci_link_route),
>> +        [XEN_DMOP_modified_memory]                  = sizeof(struct
>> xen_dm_op_modified_memory),
>> +        [XEN_DMOP_set_mem_type]                     = sizeof(struct
>> xen_dm_op_set_mem_type),
>> +        [XEN_DMOP_inject_event]                     = sizeof(struct
>> xen_dm_op_inject_event),
>> +        [XEN_DMOP_inject_msi]                       = sizeof(struct
>> xen_dm_op_inject_msi),
>> +        [XEN_DMOP_map_mem_type_to_ioreq_server]     = sizeof(struct
>> xen_dm_op_map_mem_type_to_ioreq_server),
>> +        [XEN_DMOP_remote_shutdown]                  = sizeof(struct
>> xen_dm_op_remote_shutdown),
>> +        [XEN_DMOP_relocate_memory]                  = sizeof(struct
>> xen_dm_op_relocate_memory),
>> +        [XEN_DMOP_pin_memory_cacheattr]             = sizeof(struct
>> xen_dm_op_pin_memory_cacheattr),
>> +    };
>>
>>      rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
>>      if ( rc )
>> @@ -384,12 +374,27 @@ static int dm_op(const struct dmop_args *op_args)
>>      if ( rc )
>>          goto out;
>>
>> -    if ( !COPY_FROM_GUEST_BUF(op, op_args, 0) )
>> -    {
>> -        rc = -EFAULT;
>> +    offset = offsetof(struct xen_dm_op, u);
>> +
>> +    rc = -EFAULT;
>> +    if ( op_args->buf[0].size < offset )
>> +        goto out;
>> +
>> +    if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, offset) )
>> +        goto out;
>> +
>> +    if ( op.op >= ARRAY_SIZE(op_size) ) {

Style, which can be fixed on commit.  Otherwise, Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

>> +        rc = -EOPNOTSUPP;
>>          goto out;
>>      }
>>
>> +    if ( op_args->buf[0].size < offset + op_size[op.op] )
>> +        goto out;
>> +
>> +    if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset,
>> +                                op_size[op.op]) )
>> +        goto out;
>> +
>>      rc = -EINVAL;
>>      if ( op.pad )
>>          goto out;
>> @@ -694,7 +699,8 @@ static int dm_op(const struct dmop_args *op_args)
>>      }
>>
>>      if ( (!rc || rc == -ERESTART) &&
>> -         !const_op && !COPY_TO_GUEST_BUF(op_args, 0, op) )
>> +         !const_op && copy_to_guest_offset(op_args->buf[0].h, offset,
>> +                                           (void *)&op.u, op_size[op.op]) )
>>          rc = -EFAULT;
>>
>>   out:
>> --
>> 2.9.5


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