diff mbox

[v5,for-4.9,3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

Message ID 1491593753-1181-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper April 7, 2017, 7:35 p.m. UTC
copy_{to,from}_guest_buf() are now implemented using an offset of 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Paul Durrant April 10, 2017, 9:11 a.m. UTC | #1
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 07 April 2017 20:36
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
> 
> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 3d8ae89..d584aba 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -37,9 +37,9 @@ struct dmop_bufs {
>  #undef MAX_NR_BUFS
>  };
> 
> -static bool _raw_copy_from_guest_buf(
> +static bool _raw_copy_from_guest_buf_offset(
>      const struct dmop_bufs *bufs, unsigned int idx,
> -    void *dst, size_t dst_bytes)
> +    size_t offset_bytes, void *dst, size_t dst_bytes)
>  {
>      size_t buf_bytes;
> 
> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
> 
>      buf_bytes = bufs->buf[idx].size;
> 
> -    if ( dst_bytes > buf_bytes )
> +    if ( offset_bytes >= dst_bytes ||
> +         (offset_bytes + dst_bytes) < offset_bytes ||
> +         (offset_bytes + dst_bytes) > dst_bytes )
>          return false;
> 
>      memset(dst, 0, dst_bytes);
> 
> -    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> +    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> +                                   offset_bytes, dst_bytes);

Not sure what's going on here. 'buf_bytes' is being assigned but no longer seems to be used (since it's dropped from the if statement). Also, I'm not entirely sure what range check that if statement is trying to perform. Can we at least have a comment it it's actually correct (which I'm not at all convinced of).

  Paul

>  }
> 
> -static bool _raw_copy_to_guest_buf(
> +static bool _raw_copy_to_guest_buf_offset(
>      struct dmop_bufs *bufs, unsigned int idx,
> -    const void *src, size_t src_bytes)
> +    size_t offset_bytes, const void *src, size_t src_bytes)
>  {
>      size_t buf_bytes;
> 
> @@ -67,17 +70,28 @@ static bool _raw_copy_to_guest_buf(
> 
>      buf_bytes = bufs->buf[idx].size;
> 
> -    if ( src_bytes > buf_bytes )
> +    if ( offset_bytes >= src_bytes ||
> +         (offset_bytes + src_bytes) < offset_bytes ||
> +         (offset_bytes + src_bytes) > src_bytes )
>          return false;
> 
> -    return !copy_to_guest(bufs->buf[idx].h, src, src_bytes);
> +    return !copy_to_guest_offset(bufs->buf[idx].h, offset_bytes,
> +                                 src, src_bytes);
>  }
> 
> +#define copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, dst) \
> +    _raw_copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, \
> +                                    dst, 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(bufs, buf_idx, dst) \
> -    _raw_copy_from_guest_buf(bufs, buf_idx, dst, sizeof(*(dst)))
> +    copy_from_guest_buf_offset(bufs, buf_idx, 0, dst)
> 
>  #define copy_to_guest_buf(bufs, buf_idx, src) \
> -    _raw_copy_to_guest_buf(bufs, buf_idx, src, sizeof(*(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, struct xen_dm_op_buf *buf)
> --
> 2.1.4
Andrew Cooper April 10, 2017, 9:35 a.m. UTC | #2
On 10/04/17 10:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 07 April 2017 20:36
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien Grall
>> <julien.grall@arm.com>
>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>> copy_{to,from}_guest_buf_offset() helpers
>>
>> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
>>  1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index 3d8ae89..d584aba 100644
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -37,9 +37,9 @@ struct dmop_bufs {
>>  #undef MAX_NR_BUFS
>>  };
>>
>> -static bool _raw_copy_from_guest_buf(
>> +static bool _raw_copy_from_guest_buf_offset(
>>      const struct dmop_bufs *bufs, unsigned int idx,
>> -    void *dst, size_t dst_bytes)
>> +    size_t offset_bytes, void *dst, size_t dst_bytes)
>>  {
>>      size_t buf_bytes;
>>
>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
>>
>>      buf_bytes = bufs->buf[idx].size;
>>
>> -    if ( dst_bytes > buf_bytes )
>> +    if ( offset_bytes >= dst_bytes ||
>> +         (offset_bytes + dst_bytes) < offset_bytes ||
>> +         (offset_bytes + dst_bytes) > dst_bytes )
>>          return false;
>>
>>      memset(dst, 0, dst_bytes);
>>
>> -    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
>> +    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
>> +                                   offset_bytes, dst_bytes);
> Not sure what's going on here. 'buf_bytes' is being assigned but no longer seems to be used (since it's dropped from the if statement). Also, I'm not entirely sure what range check that if statement is trying to perform. Can we at least have a comment it it's actually correct (which I'm not at all convinced of).

That is actually because the if statement isn't correct.  The final
comparison should be against buf_bytes, not dst_bytes.

The problem is that copy_from_guest_offset() takes offset in units of
sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
the type of buf.h changes), while nr is strictly in bytes.  My
conclusion after Friday's hacking is that this is also a recipe
security-relevant mistakes, and is fiendishly complicated to reason about.

~Andrew
Paul Durrant April 10, 2017, 9:52 a.m. UTC | #3
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:36
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
> 
> On 10/04/17 10:11, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: 07 April 2017 20:36
> >> To: Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
> Grall
> >> <julien.grall@arm.com>
> >> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >> copy_{to,from}_guest_buf_offset() helpers
> >>
> >> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >>  xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
> >>  1 file changed, 24 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >> index 3d8ae89..d584aba 100644
> >> --- a/xen/arch/x86/hvm/dm.c
> >> +++ b/xen/arch/x86/hvm/dm.c
> >> @@ -37,9 +37,9 @@ struct dmop_bufs {
> >>  #undef MAX_NR_BUFS
> >>  };
> >>
> >> -static bool _raw_copy_from_guest_buf(
> >> +static bool _raw_copy_from_guest_buf_offset(
> >>      const struct dmop_bufs *bufs, unsigned int idx,
> >> -    void *dst, size_t dst_bytes)
> >> +    size_t offset_bytes, void *dst, size_t dst_bytes)
> >>  {
> >>      size_t buf_bytes;
> >>
> >> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
> >>
> >>      buf_bytes = bufs->buf[idx].size;
> >>
> >> -    if ( dst_bytes > buf_bytes )
> >> +    if ( offset_bytes >= dst_bytes ||
> >> +         (offset_bytes + dst_bytes) < offset_bytes ||
> >> +         (offset_bytes + dst_bytes) > dst_bytes )
> >>          return false;
> >>
> >>      memset(dst, 0, dst_bytes);
> >>
> >> -    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> >> +    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> >> +                                   offset_bytes, dst_bytes);
> > Not sure what's going on here. 'buf_bytes' is being assigned but no longer
> seems to be used (since it's dropped from the if statement). Also, I'm not
> entirely sure what range check that if statement is trying to perform. Can we
> at least have a comment it it's actually correct (which I'm not at all convinced
> of).
> 
> That is actually because the if statement isn't correct.  The final
> comparison should be against buf_bytes, not dst_bytes.

Ok, that makes more sense... so the first clause is checking for size_t overflow, right?     

> 
> The problem is that copy_from_guest_offset() takes offset in units of
> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
> the type of buf.h changes), while nr is strictly in bytes.  My
> conclusion after Friday's hacking is that this is also a recipe
> security-relevant mistakes, and is fiendishly complicated to reason about.

Anything using typeof() is a PITA to reason about IMO, so using the offset variant is definitely going to be an improvement.

  Paul

> 
> ~Andrew
Andrew Cooper April 10, 2017, 9:57 a.m. UTC | #4
On 10/04/17 10:52, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 10 April 2017 10:36
>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
>> devel@lists.xen.org>
>> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>> copy_{to,from}_guest_buf_offset() helpers
>>
>> On 10/04/17 10:11, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: 07 April 2017 20:36
>>>> To: Xen-devel <xen-devel@lists.xen.org>
>>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>>>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
>> Grall
>>>> <julien.grall@arm.com>
>>>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>>>> copy_{to,from}_guest_buf_offset() helpers
>>>>
>>>> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
>>>>  1 file changed, 24 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>> index 3d8ae89..d584aba 100644
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -37,9 +37,9 @@ struct dmop_bufs {
>>>>  #undef MAX_NR_BUFS
>>>>  };
>>>>
>>>> -static bool _raw_copy_from_guest_buf(
>>>> +static bool _raw_copy_from_guest_buf_offset(
>>>>      const struct dmop_bufs *bufs, unsigned int idx,
>>>> -    void *dst, size_t dst_bytes)
>>>> +    size_t offset_bytes, void *dst, size_t dst_bytes)
>>>>  {
>>>>      size_t buf_bytes;
>>>>
>>>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
>>>>
>>>>      buf_bytes = bufs->buf[idx].size;
>>>>
>>>> -    if ( dst_bytes > buf_bytes )
>>>> +    if ( offset_bytes >= dst_bytes ||
>>>> +         (offset_bytes + dst_bytes) < offset_bytes ||
>>>> +         (offset_bytes + dst_bytes) > dst_bytes )
>>>>          return false;
>>>>
>>>>      memset(dst, 0, dst_bytes);
>>>>
>>>> -    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
>>>> +    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
>>>> +                                   offset_bytes, dst_bytes);
>>> Not sure what's going on here. 'buf_bytes' is being assigned but no longer
>> seems to be used (since it's dropped from the if statement). Also, I'm not
>> entirely sure what range check that if statement is trying to perform. Can we
>> at least have a comment it it's actually correct (which I'm not at all convinced
>> of).
>>
>> That is actually because the if statement isn't correct.  The final
>> comparison should be against buf_bytes, not dst_bytes.
> Ok, that makes more sense... so the first clause is checking for size_t overflow, right?

The first is a straight "is the offset off the end of the buffer", the
middle is a size_t overflow check (this is defined behaviour as size_t
is unsigned.  It would be UB if size_t was signed), and the final was
supposed to be "(offset_bytes + dst_bytes) > buf_bytes" to check whether
the entire region wanting copying resides inside the buffer.

>
>> The problem is that copy_from_guest_offset() takes offset in units of
>> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
>> the type of buf.h changes), while nr is strictly in bytes.  My
>> conclusion after Friday's hacking is that this is also a recipe
>> security-relevant mistakes, and is fiendishly complicated to reason about.
> Anything using typeof() is a PITA to reason about IMO, so using the offset variant is definitely going to be an improvement.

I'm mulling over rewriting it all, because it is starting to get
embarrassing how many security issues we have in the existing
infrastructure.

~Andrew
Paul Durrant April 10, 2017, 10:04 a.m. UTC | #5
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:57
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
> 
> On 10/04/17 10:52, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 10 April 2017 10:36
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> >> devel@lists.xen.org>
> >> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >> copy_{to,from}_guest_buf_offset() helpers
> >>
> >> On 10/04/17 10:11, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>>> Sent: 07 April 2017 20:36
> >>>> To: Xen-devel <xen-devel@lists.xen.org>
> >>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >>>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
> >> Grall
> >>>> <julien.grall@arm.com>
> >>>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >>>> copy_{to,from}_guest_buf_offset() helpers
> >>>>
> >>>> copy_{to,from}_guest_buf() are now implemented using an offset of
> 0.
> >>>>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> ---
> >>>> CC: Jan Beulich <JBeulich@suse.com>
> >>>> CC: Paul Durrant <paul.durrant@citrix.com>
> >>>> CC: Julien Grall <julien.grall@arm.com>
> >>>> ---
> >>>>  xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
> >>>>  1 file changed, 24 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >>>> index 3d8ae89..d584aba 100644
> >>>> --- a/xen/arch/x86/hvm/dm.c
> >>>> +++ b/xen/arch/x86/hvm/dm.c
> >>>> @@ -37,9 +37,9 @@ struct dmop_bufs {
> >>>>  #undef MAX_NR_BUFS
> >>>>  };
> >>>>
> >>>> -static bool _raw_copy_from_guest_buf(
> >>>> +static bool _raw_copy_from_guest_buf_offset(
> >>>>      const struct dmop_bufs *bufs, unsigned int idx,
> >>>> -    void *dst, size_t dst_bytes)
> >>>> +    size_t offset_bytes, void *dst, size_t dst_bytes)
> >>>>  {
> >>>>      size_t buf_bytes;
> >>>>
> >>>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
> >>>>
> >>>>      buf_bytes = bufs->buf[idx].size;
> >>>>
> >>>> -    if ( dst_bytes > buf_bytes )
> >>>> +    if ( offset_bytes >= dst_bytes ||
> >>>> +         (offset_bytes + dst_bytes) < offset_bytes ||
> >>>> +         (offset_bytes + dst_bytes) > dst_bytes )
> >>>>          return false;
> >>>>
> >>>>      memset(dst, 0, dst_bytes);
> >>>>
> >>>> -    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> >>>> +    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> >>>> +                                   offset_bytes, dst_bytes);
> >>> Not sure what's going on here. 'buf_bytes' is being assigned but no
> longer
> >> seems to be used (since it's dropped from the if statement). Also, I'm not
> >> entirely sure what range check that if statement is trying to perform. Can
> we
> >> at least have a comment it it's actually correct (which I'm not at all
> convinced
> >> of).
> >>
> >> That is actually because the if statement isn't correct.  The final
> >> comparison should be against buf_bytes, not dst_bytes.
> > Ok, that makes more sense... so the first clause is checking for size_t
> overflow, right?
> 
> The first is a straight "is the offset off the end of the buffer", the
> middle is a size_t overflow check (this is defined behaviour as size_t
> is unsigned.  It would be UB if size_t was signed), and the final was
> supposed to be "(offset_bytes + dst_bytes) > buf_bytes" to check whether
> the entire region wanting copying resides inside the buffer.
> 

Sorry, I meant the middle clause, so with the fix that all makes sense now.

> >
> >> The problem is that copy_from_guest_offset() takes offset in units of
> >> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
> >> the type of buf.h changes), while nr is strictly in bytes.  My
> >> conclusion after Friday's hacking is that this is also a recipe
> >> security-relevant mistakes, and is fiendishly complicated to reason about.
> > Anything using typeof() is a PITA to reason about IMO, so using the offset
> variant is definitely going to be an improvement.
> 
> I'm mulling over rewriting it all, because it is starting to get
> embarrassing how many security issues we have in the existing
> infrastructure.
> 

Certainly removing some of the macrotization would make it easier to follow and, if copy_from_guest() is going to use typeof (for convenience of copying an op struct in the common case I guess) then it really should be single instance for safety, or preferably take an explicit type.

  Paul

> ~Andrew
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 3d8ae89..d584aba 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -37,9 +37,9 @@  struct dmop_bufs {
 #undef MAX_NR_BUFS
 };
 
-static bool _raw_copy_from_guest_buf(
+static bool _raw_copy_from_guest_buf_offset(
     const struct dmop_bufs *bufs, unsigned int idx,
-    void *dst, size_t dst_bytes)
+    size_t offset_bytes, void *dst, size_t dst_bytes)
 {
     size_t buf_bytes;
 
@@ -48,17 +48,20 @@  static bool _raw_copy_from_guest_buf(
 
     buf_bytes = bufs->buf[idx].size;
 
-    if ( dst_bytes > buf_bytes )
+    if ( offset_bytes >= dst_bytes ||
+         (offset_bytes + dst_bytes) < offset_bytes ||
+         (offset_bytes + dst_bytes) > dst_bytes )
         return false;
 
     memset(dst, 0, dst_bytes);
 
-    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
+    return !copy_from_guest_offset(dst, bufs->buf[idx].h,
+                                   offset_bytes, dst_bytes);
 }
 
-static bool _raw_copy_to_guest_buf(
+static bool _raw_copy_to_guest_buf_offset(
     struct dmop_bufs *bufs, unsigned int idx,
-    const void *src, size_t src_bytes)
+    size_t offset_bytes, const void *src, size_t src_bytes)
 {
     size_t buf_bytes;
 
@@ -67,17 +70,28 @@  static bool _raw_copy_to_guest_buf(
 
     buf_bytes = bufs->buf[idx].size;
 
-    if ( src_bytes > buf_bytes )
+    if ( offset_bytes >= src_bytes ||
+         (offset_bytes + src_bytes) < offset_bytes ||
+         (offset_bytes + src_bytes) > src_bytes )
         return false;
 
-    return !copy_to_guest(bufs->buf[idx].h, src, src_bytes);
+    return !copy_to_guest_offset(bufs->buf[idx].h, offset_bytes,
+                                 src, src_bytes);
 }
 
+#define copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, dst) \
+    _raw_copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, \
+                                    dst, 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(bufs, buf_idx, dst) \
-    _raw_copy_from_guest_buf(bufs, buf_idx, dst, sizeof(*(dst)))
+    copy_from_guest_buf_offset(bufs, buf_idx, 0, dst)
 
 #define copy_to_guest_buf(bufs, buf_idx, src) \
-    _raw_copy_to_guest_buf(bufs, buf_idx, src, sizeof(*(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, struct xen_dm_op_buf *buf)