* [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest
@ 2018-02-09 15:34 Ross Lagerwall
2018-02-09 15:42 ` Paul Durrant
0 siblings, 1 reply; 5+ messages in thread
From: Ross Lagerwall @ 2018-02-09 15:34 UTC (permalink / raw)
To: xen-devel; +Cc: Ross Lagerwall, Paul Durrant, Jan Beulich, Andrew Cooper
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@citrix.com>
---
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[] = {
+ [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) ) {
+ 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest
2018-02-09 15:34 [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest Ross Lagerwall
@ 2018-02-09 15:42 ` Paul Durrant
2018-02-09 15:44 ` Andrew Cooper
2018-02-09 15:48 ` Ross Lagerwall
0 siblings, 2 replies; 5+ messages in thread
From: Paul Durrant @ 2018-02-09 15:42 UTC (permalink / raw)
To: xen-devel; +Cc: Ross Lagerwall, Jan Beulich, Andrew Cooper
> -----Original Message-----
> From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com]
> Sent: 09 February 2018 15:34
> To: xen-devel@lists.xen.org
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>
> 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@citrix.com>
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@citrix.com>, 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);
> }
>
> -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.
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) ) {
> + 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest
2018-02-09 15:42 ` Paul Durrant
@ 2018-02-09 15:44 ` Andrew Cooper
2018-02-09 15:48 ` Ross Lagerwall
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2018-02-09 15:44 UTC (permalink / raw)
To: Paul Durrant, Ross Lagerwall, xen-devel; +Cc: Jan Beulich
On 09/02/18 15:42, Paul Durrant wrote:
>> -----Original Message-----
>> From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com]
>> Sent: 09 February 2018 15:34
>> To: xen-devel@lists.xen.org
>> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; Jan Beulich
>> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul
>> Durrant <Paul.Durrant@citrix.com>
>> 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@citrix.com>
> 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@citrix.com>, 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@citrix.com>
>> + 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest
2018-02-09 15:42 ` Paul Durrant
2018-02-09 15:44 ` Andrew Cooper
@ 2018-02-09 15:48 ` Ross Lagerwall
2018-02-09 15:49 ` Paul Durrant
1 sibling, 1 reply; 5+ messages in thread
From: Ross Lagerwall @ 2018-02-09 15:48 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Andrew Cooper, Jan Beulich
On 02/09/2018 03:42 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com]
>> Sent: 09 February 2018 15:34
>> To: xen-devel@lists.xen.org
>> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; Jan Beulich
>> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul
>> Durrant <Paul.Durrant@citrix.com>
>> 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@citrix.com>
>
> 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@citrix.com>, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest
2018-02-09 15:48 ` Ross Lagerwall
@ 2018-02-09 15:49 ` Paul Durrant
0 siblings, 0 replies; 5+ messages in thread
From: Paul Durrant @ 2018-02-09 15:49 UTC (permalink / raw)
To: Ross Lagerwall, xen-devel; +Cc: Andrew Cooper, Jan Beulich
> -----Original Message-----
> From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com]
> Sent: 09 February 2018 15:49
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org
> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>
> 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@citrix.com]
> >> Sent: 09 February 2018 15:34
> >> To: xen-devel@lists.xen.org
> >> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; Jan Beulich
> >> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Paul
> >> Durrant <Paul.Durrant@citrix.com>
> >> 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@citrix.com>
> >
> > 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@citrix.com>, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-09 15:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-09 15:34 [PATCH] x86/hvm/dmop: Only copy what is needed to/from the guest Ross Lagerwall
2018-02-09 15:42 ` Paul Durrant
2018-02-09 15:44 ` Andrew Cooper
2018-02-09 15:48 ` Ross Lagerwall
2018-02-09 15:49 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.