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

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