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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>; Xen-devel <xen-
>> devel@xxxxxxxxxxxxx>
>> Cc: Jan Beulich <JBeulich@xxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
>> 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@xxxxxxxxxx]
>>>> Sent: 07 April 2017 20:36
>>>> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
>>>> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
>>>> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Julien
>> Grall
>>>> <julien.grall@xxxxxxx>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>>> CC: Julien Grall <julien.grall@xxxxxxx>
>>>> ---
>>>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel