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: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.

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

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