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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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