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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:57
> 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: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.
> 

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

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