| Message ID | 1492711189-2857-2-git-send-email-jennifer.herbert@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: > From: Jennifer Herbert <Jennifer.Herbert@citrix.com> Is this correct, considering that iirc the patch was new in v5 and ... > This also allows the usual cases to be simplified, by omitting an > unnecessary > buf parameters, and because the macros can appropriately size the object. > > This makes copying to or from a buf that isn't big enough an error. > If the buffer isnt big enough, trying to carry on regardless > can only cause trouble later on. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> ... this sequence of S-o-b-s? > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -32,36 +32,47 @@ struct dmop_args { > struct xen_dm_op_buf buf[2]; > }; > > -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], > - unsigned int nr_bufs, void *dst, > - unsigned int idx, size_t dst_size) > +static bool _raw_copy_from_guest_buf(void *dst, > + const struct dmop_args *args, > + unsigned int buf_idx, > + size_t dst_bytes) > { > - size_t size; > + size_t buf_bytes; > > - if ( idx >= nr_bufs ) > + if ( buf_idx >= args->nr_bufs ) > return false; > > - memset(dst, 0, dst_size); > + buf_bytes = args->buf[buf_idx].size; > > - size = min_t(size_t, dst_size, bufs[idx].size); > + if ( dst_bytes > buf_bytes ) > + return false; While this behavioral change is now being mentioned in the description, I'm not sure I buy the argument of basically being guaranteed to cause trouble down the road. Did you consider the forward compatibility aspect here, allowing us to extend interface structures by adding fields to their ends without breaking old callers? Paul, what are your thoughts here? Jan
> -----Original Message----- > From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com] > Sent: 20 April 2017 19:00 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; > Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com> > Subject: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in > terms of raw accessors > > From: Jennifer Herbert <Jennifer.Herbert@citrix.com> > > This also allows the usual cases to be simplified, by omitting an unnecessary > buf parameters, and because the macros can appropriately size the object. > > This makes copying to or from a buf that isn't big enough an error. > If the buffer isnt big enough, trying to carry on regardless > can only cause trouble later on. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> > -- > CC: Paul Durrant <paul.durrant@citrix.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > --- > xen/arch/x86/hvm/dm.c | 47 +++++++++++++++++++++++++++++---------- > -------- > 1 file changed, 29 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index fb4bcec..3607ddb 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -32,36 +32,47 @@ struct dmop_args { > struct xen_dm_op_buf buf[2]; > }; > > -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], > - unsigned int nr_bufs, void *dst, > - unsigned int idx, size_t dst_size) > +static bool _raw_copy_from_guest_buf(void *dst, > + const struct dmop_args *args, > + unsigned int buf_idx, > + size_t dst_bytes) > { > - size_t size; > + size_t buf_bytes; > > - if ( idx >= nr_bufs ) > + if ( buf_idx >= args->nr_bufs ) > return false; > > - memset(dst, 0, dst_size); > + buf_bytes = args->buf[buf_idx].size; > > - size = min_t(size_t, dst_size, bufs[idx].size); > + if ( dst_bytes > buf_bytes ) > + return false; > > - return !copy_from_guest(dst, bufs[idx].h, size); > + return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes); > } > > -static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[], > - unsigned int nr_bufs, unsigned int idx, > - const void *src, size_t src_size) > +static bool _raw_copy_to_guest_buf(struct dmop_args *args, I think this should be const, same as in the copy-from case above. > + unsigned int buf_idx, > + const void *src, size_t src_bytes) > { > - size_t size; > + size_t buf_bytes; > > - if ( idx >= nr_bufs ) > + if ( buf_idx >= args->nr_bufs ) > return false; > > - size = min_t(size_t, bufs[idx].size, src_size); > + buf_bytes = args->buf[buf_idx].size; > + > + if ( src_bytes > buf_bytes ) > + return false; > > - return !copy_to_guest(bufs[idx].h, src, size); > + return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes); > } > > +#define copy_from_guest_buf(dst, args, buf_idx) \ > + _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst))) > + > +#define copy_to_guest_buf(args, buf_idx, src) \ > + _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src))) > + Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to use these macros and pass a pointer to allocated memory rather than &<thing-on-stack> then they would not have the desired effect. Clearly such use would be very naïve but I wonder whether having something like: #define copy_to_guest_buf(args, buf_idx, src) \ _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src)) would be safer. Paul > static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn, > unsigned int nr, struct xen_dm_op_buf *buf) > { > @@ -312,7 +323,7 @@ static int dm_op(struct dmop_args *op_args) > if ( rc ) > goto out; > > - if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, > sizeof(op)) ) > + if ( !copy_from_guest_buf(&op, op_args, 0) ); > { > rc = -EFAULT; > goto out; > @@ -568,8 +579,8 @@ static int dm_op(struct dmop_args *op_args) > } > > if ( (!rc || rc == -ERESTART) && > - !const_op && > - !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, > sizeof(op)) ) > + !const_op && !copy_to_guest_buf(op_args, 0, &op) ) > + > rc = -EFAULT; > > out: > -- > 2.1.4
>>> On 21.04.17 at 10:02, <Paul.Durrant@citrix.com> wrote: >> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com] >> Sent: 20 April 2017 19:00 >> +#define copy_from_guest_buf(dst, args, buf_idx) \ >> + _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst))) >> + >> +#define copy_to_guest_buf(args, buf_idx, src) \ >> + _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src))) >> + > > Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to use > these macros and pass a pointer to allocated memory rather than &<thing-on-stack> > then they would not have the desired effect. Clearly such use would be very > naïve but I wonder whether having something like: > > #define copy_to_guest_buf(args, buf_idx, src) \ > _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src)) > > would be safer. You mean in the case the allocated memory was an array? If it's a pointer to a singular object, I don't think there would be anything wrong. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 21 April 2017 09:48 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Jennifer Herbert > <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org> > Subject: RE: [PATCH 2/4] hvm/dmop: Implement copy_{to, > from}_guest_buf() in terms of raw accessors > > >>> On 21.04.17 at 10:02, <Paul.Durrant@citrix.com> wrote: > >> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com] > >> Sent: 20 April 2017 19:00 > >> +#define copy_from_guest_buf(dst, args, buf_idx) \ > >> + _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst))) > >> + > >> +#define copy_to_guest_buf(args, buf_idx, src) \ > >> + _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src))) > >> + > > > > Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to > use > > these macros and pass a pointer to allocated memory rather than &<thing- > on-stack> > > then they would not have the desired effect. Clearly such use would be > very > > naïve but I wonder whether having something like: > > > > #define copy_to_guest_buf(args, buf_idx, src) \ > > _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src)) > > > > would be safer. > > You mean in the case the allocated memory was an array? If it's a > pointer to a singular object, I don't think there would be anything > wrong. Yes, or someone erroneously passing a void *. As I said, such code would be very naïve, but changing the macro as I suggest would rule it out. Paul > > Jan
On 21/04/2017 08:27, Jan Beulich wrote: >>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> > Is this correct, considering that iirc the patch was new in v5 and ... > >> This also allows the usual cases to be simplified, by omitting an >> unnecessary >> buf parameters, and because the macros can appropriately size the object. >> >> This makes copying to or from a buf that isn't big enough an error. >> If the buffer isnt big enough, trying to carry on regardless >> can only cause trouble later on. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> > ... this sequence of S-o-b-s? > >> --- a/xen/arch/x86/hvm/dm.c >> +++ b/xen/arch/x86/hvm/dm.c >> @@ -32,36 +32,47 @@ struct dmop_args { >> struct xen_dm_op_buf buf[2]; >> }; >> >> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >> - unsigned int nr_bufs, void *dst, >> - unsigned int idx, size_t dst_size) >> +static bool _raw_copy_from_guest_buf(void *dst, >> + const struct dmop_args *args, >> + unsigned int buf_idx, >> + size_t dst_bytes) >> { >> - size_t size; >> + size_t buf_bytes; >> >> - if ( idx >= nr_bufs ) >> + if ( buf_idx >= args->nr_bufs ) >> return false; >> >> - memset(dst, 0, dst_size); >> + buf_bytes = args->buf[buf_idx].size; >> >> - size = min_t(size_t, dst_size, bufs[idx].size); >> + if ( dst_bytes > buf_bytes ) >> + return false; > While this behavioral change is now being mentioned in the > description, I'm not sure I buy the argument of basically being > guaranteed to cause trouble down the road. Did you consider the > forward compatibility aspect here, allowing us to extend interface > structures by adding fields to their ends without breaking old > callers? Paul, what are your thoughts here? DMOP is a stable ABI. There is no legal extending of any objects. The previous semantics are guaranteed to break the ABI with future subops, which is why I removed it. In the case that the guest originally passed an overly long buffer, and someone tried be "clever" here passing the same object here expecting _raw_copy_from_guest_buf() to DTRT, the function will end up copying too much data from the guest, and you will end up with something the guest wasn't intending to be part of the structure replacing the zero extension. New subops which take a longer object should use a brand new object. $MAGIC compatibility logic like you want has no business living in the copy helper. Had I spotted the intention during the original dmop series, I would have rejected it during review. ~Andrew
>>> On 21.04.17 at 10:54, <andrew.cooper3@citrix.com> wrote: > On 21/04/2017 08:27, Jan Beulich wrote: >>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >> Is this correct, considering that iirc the patch was new in v5 and ... >> >>> This also allows the usual cases to be simplified, by omitting an >>> unnecessary >>> buf parameters, and because the macros can appropriately size the object. >>> >>> This makes copying to or from a buf that isn't big enough an error. >>> If the buffer isnt big enough, trying to carry on regardless >>> can only cause trouble later on. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >> ... this sequence of S-o-b-s? >> >>> --- a/xen/arch/x86/hvm/dm.c >>> +++ b/xen/arch/x86/hvm/dm.c >>> @@ -32,36 +32,47 @@ struct dmop_args { >>> struct xen_dm_op_buf buf[2]; >>> }; >>> >>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>> - unsigned int nr_bufs, void *dst, >>> - unsigned int idx, size_t dst_size) >>> +static bool _raw_copy_from_guest_buf(void *dst, >>> + const struct dmop_args *args, >>> + unsigned int buf_idx, >>> + size_t dst_bytes) >>> { >>> - size_t size; >>> + size_t buf_bytes; >>> >>> - if ( idx >= nr_bufs ) >>> + if ( buf_idx >= args->nr_bufs ) >>> return false; >>> >>> - memset(dst, 0, dst_size); >>> + buf_bytes = args->buf[buf_idx].size; >>> >>> - size = min_t(size_t, dst_size, bufs[idx].size); >>> + if ( dst_bytes > buf_bytes ) >>> + return false; >> While this behavioral change is now being mentioned in the >> description, I'm not sure I buy the argument of basically being >> guaranteed to cause trouble down the road. Did you consider the >> forward compatibility aspect here, allowing us to extend interface >> structures by adding fields to their ends without breaking old >> callers? Paul, what are your thoughts here? > > DMOP is a stable ABI. There is no legal extending of any objects. I disagree: We have various pad fields which we check to be zero. Putting flags there indicating presence of extended fields would be quite fine, without breaking the ABI in any way. > The previous semantics are guaranteed to break the ABI with future > subops, which is why I removed it. In the case that the guest > originally passed an overly long buffer, and someone tried be "clever" > here passing the same object here expecting _raw_copy_from_guest_buf() > to DTRT, the function will end up copying too much data from the guest, > and you will end up with something the guest wasn't intending to be part > of the structure replacing the zero extension. Good point, but taken care of with the flags semantics assignable to padding fields. > New subops which take a longer object should use a brand new object. > > $MAGIC compatibility logic like you want has no business living in the > copy helper. Had I spotted the intention during the original dmop > series, I would have rejected it during review. I'm surprised you didn't notice, because iirc the behavior had even been changed back and forth between revisions. Jan
On 21/04/2017 09:54, Andrew Cooper wrote: > On 21/04/2017 08:27, Jan Beulich wrote: >>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >> Is this correct, considering that iirc the patch was new in v5 and ... >> >>> This also allows the usual cases to be simplified, by omitting an >>> unnecessary >>> buf parameters, and because the macros can appropriately size the object. >>> >>> This makes copying to or from a buf that isn't big enough an error. >>> If the buffer isnt big enough, trying to carry on regardless >>> can only cause trouble later on. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >> ... this sequence of S-o-b-s? >> >>> --- a/xen/arch/x86/hvm/dm.c >>> +++ b/xen/arch/x86/hvm/dm.c >>> @@ -32,36 +32,47 @@ struct dmop_args { >>> struct xen_dm_op_buf buf[2]; >>> }; >>> >>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>> - unsigned int nr_bufs, void *dst, >>> - unsigned int idx, size_t dst_size) >>> +static bool _raw_copy_from_guest_buf(void *dst, >>> + const struct dmop_args *args, >>> + unsigned int buf_idx, >>> + size_t dst_bytes) >>> { >>> - size_t size; >>> + size_t buf_bytes; >>> >>> - if ( idx >= nr_bufs ) >>> + if ( buf_idx >= args->nr_bufs ) >>> return false; >>> >>> - memset(dst, 0, dst_size); >>> + buf_bytes = args->buf[buf_idx].size; >>> >>> - size = min_t(size_t, dst_size, bufs[idx].size); >>> + if ( dst_bytes > buf_bytes ) >>> + return false; >> While this behavioral change is now being mentioned in the >> description, I'm not sure I buy the argument of basically being >> guaranteed to cause trouble down the road. Did you consider the >> forward compatibility aspect here, allowing us to extend interface >> structures by adding fields to their ends without breaking old >> callers? Paul, what are your thoughts here? > DMOP is a stable ABI. There is no legal extending of any objects. > > The previous semantics are guaranteed to break the ABI with future > subops, which is why I removed it. In the case that the guest > originally passed an overly long buffer, and someone tried be "clever" > here passing the same object here expecting _raw_copy_from_guest_buf() > to DTRT, the function will end up copying too much data from the guest, > and you will end up with something the guest wasn't intending to be part > of the structure replacing the zero extension. > > New subops which take a longer object should use a brand new object. > > $MAGIC compatibility logic like you want has no business living in the > copy helper. Had I spotted the intention during the original dmop > series, I would have rejected it during review. Actually, the existing behaviour is already broken. If a guest passes an overly short buf[0], the dmop logic won't get a failure, and instead get a truncated structure which has been zero extended. This is very definitely the wrong thing to do, because such a truncated structure might actually contain some legitimate operations. I reiterate my statement that $MAGIC like this has no place living in the copy helpers. All it does is introduce bugs. ~Andrew
>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: > On 21/04/2017 09:54, Andrew Cooper wrote: >> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>> Is this correct, considering that iirc the patch was new in v5 and ... >>> >>>> This also allows the usual cases to be simplified, by omitting an >>>> unnecessary >>>> buf parameters, and because the macros can appropriately size the object. >>>> >>>> This makes copying to or from a buf that isn't big enough an error. >>>> If the buffer isnt big enough, trying to carry on regardless >>>> can only cause trouble later on. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>> ... this sequence of S-o-b-s? >>> >>>> --- a/xen/arch/x86/hvm/dm.c >>>> +++ b/xen/arch/x86/hvm/dm.c >>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>> struct xen_dm_op_buf buf[2]; >>>> }; >>>> >>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>> - unsigned int nr_bufs, void *dst, >>>> - unsigned int idx, size_t dst_size) >>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>> + const struct dmop_args *args, >>>> + unsigned int buf_idx, >>>> + size_t dst_bytes) >>>> { >>>> - size_t size; >>>> + size_t buf_bytes; >>>> >>>> - if ( idx >= nr_bufs ) >>>> + if ( buf_idx >= args->nr_bufs ) >>>> return false; >>>> >>>> - memset(dst, 0, dst_size); >>>> + buf_bytes = args->buf[buf_idx].size; >>>> >>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>> + if ( dst_bytes > buf_bytes ) >>>> + return false; >>> While this behavioral change is now being mentioned in the >>> description, I'm not sure I buy the argument of basically being >>> guaranteed to cause trouble down the road. Did you consider the >>> forward compatibility aspect here, allowing us to extend interface >>> structures by adding fields to their ends without breaking old >>> callers? Paul, what are your thoughts here? >> DMOP is a stable ABI. There is no legal extending of any objects. >> >> The previous semantics are guaranteed to break the ABI with future >> subops, which is why I removed it. In the case that the guest >> originally passed an overly long buffer, and someone tried be "clever" >> here passing the same object here expecting _raw_copy_from_guest_buf() >> to DTRT, the function will end up copying too much data from the guest, >> and you will end up with something the guest wasn't intending to be part >> of the structure replacing the zero extension. >> >> New subops which take a longer object should use a brand new object. >> >> $MAGIC compatibility logic like you want has no business living in the >> copy helper. Had I spotted the intention during the original dmop >> series, I would have rejected it during review. > > Actually, the existing behaviour is already broken. If a guest passes > an overly short buf[0], the dmop logic won't get a failure, and instead > get a truncated structure which has been zero extended. > > This is very definitely the wrong thing to do, because such a truncated > structure might actually contain some legitimate operations. So what, if that's what the caller meant to happen? Considering this is a controversial change, I think it is a bad idea to merge this into the otherwise uncontroversial change here. Jan
On 21/04/17 10:46, Jan Beulich wrote: >>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: >> On 21/04/2017 09:54, Andrew Cooper wrote: >>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>> >>>>> This also allows the usual cases to be simplified, by omitting an >>>>> unnecessary >>>>> buf parameters, and because the macros can appropriately size the object. >>>>> >>>>> This makes copying to or from a buf that isn't big enough an error. >>>>> If the buffer isnt big enough, trying to carry on regardless >>>>> can only cause trouble later on. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>> ... this sequence of S-o-b-s? >>>> >>>>> --- a/xen/arch/x86/hvm/dm.c >>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>> struct xen_dm_op_buf buf[2]; >>>>> }; >>>>> >>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>> - unsigned int nr_bufs, void *dst, >>>>> - unsigned int idx, size_t dst_size) >>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>> + const struct dmop_args *args, >>>>> + unsigned int buf_idx, >>>>> + size_t dst_bytes) >>>>> { >>>>> - size_t size; >>>>> + size_t buf_bytes; >>>>> >>>>> - if ( idx >= nr_bufs ) >>>>> + if ( buf_idx >= args->nr_bufs ) >>>>> return false; >>>>> >>>>> - memset(dst, 0, dst_size); >>>>> + buf_bytes = args->buf[buf_idx].size; >>>>> >>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>> + if ( dst_bytes > buf_bytes ) >>>>> + return false; >>>> While this behavioral change is now being mentioned in the >>>> description, I'm not sure I buy the argument of basically being >>>> guaranteed to cause trouble down the road. Did you consider the >>>> forward compatibility aspect here, allowing us to extend interface >>>> structures by adding fields to their ends without breaking old >>>> callers? Paul, what are your thoughts here? >>> DMOP is a stable ABI. There is no legal extending of any objects. >>> >>> The previous semantics are guaranteed to break the ABI with future >>> subops, which is why I removed it. In the case that the guest >>> originally passed an overly long buffer, and someone tried be "clever" >>> here passing the same object here expecting _raw_copy_from_guest_buf() >>> to DTRT, the function will end up copying too much data from the guest, >>> and you will end up with something the guest wasn't intending to be part >>> of the structure replacing the zero extension. >>> >>> New subops which take a longer object should use a brand new object. >>> >>> $MAGIC compatibility logic like you want has no business living in the >>> copy helper. Had I spotted the intention during the original dmop >>> series, I would have rejected it during review. >> Actually, the existing behaviour is already broken. If a guest passes >> an overly short buf[0], the dmop logic won't get a failure, and instead >> get a truncated structure which has been zero extended. >> >> This is very definitely the wrong thing to do, because such a truncated >> structure might actually contain some legitimate operations. > So what, if that's what the caller meant to happen? > > Considering this is a controversial change, I think it is a bad idea > to merge this into the otherwise uncontroversial change here. How about we move the min(..) and memset out of the helper function, and into dm_op?
>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote: > On 21/04/17 10:46, Jan Beulich wrote: >>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: >>> On 21/04/2017 09:54, Andrew Cooper wrote: >>>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>>> >>>>>> This also allows the usual cases to be simplified, by omitting an >>>>>> unnecessary >>>>>> buf parameters, and because the macros can appropriately size the object. >>>>>> >>>>>> This makes copying to or from a buf that isn't big enough an error. >>>>>> If the buffer isnt big enough, trying to carry on regardless >>>>>> can only cause trouble later on. >>>>>> >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>> ... this sequence of S-o-b-s? >>>>> >>>>>> --- a/xen/arch/x86/hvm/dm.c >>>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>>> struct xen_dm_op_buf buf[2]; >>>>>> }; >>>>>> >>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>>> - unsigned int nr_bufs, void *dst, >>>>>> - unsigned int idx, size_t dst_size) >>>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>>> + const struct dmop_args *args, >>>>>> + unsigned int buf_idx, >>>>>> + size_t dst_bytes) >>>>>> { >>>>>> - size_t size; >>>>>> + size_t buf_bytes; >>>>>> >>>>>> - if ( idx >= nr_bufs ) >>>>>> + if ( buf_idx >= args->nr_bufs ) >>>>>> return false; >>>>>> >>>>>> - memset(dst, 0, dst_size); >>>>>> + buf_bytes = args->buf[buf_idx].size; >>>>>> >>>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>>> + if ( dst_bytes > buf_bytes ) >>>>>> + return false; >>>>> While this behavioral change is now being mentioned in the >>>>> description, I'm not sure I buy the argument of basically being >>>>> guaranteed to cause trouble down the road. Did you consider the >>>>> forward compatibility aspect here, allowing us to extend interface >>>>> structures by adding fields to their ends without breaking old >>>>> callers? Paul, what are your thoughts here? >>>> DMOP is a stable ABI. There is no legal extending of any objects. >>>> >>>> The previous semantics are guaranteed to break the ABI with future >>>> subops, which is why I removed it. In the case that the guest >>>> originally passed an overly long buffer, and someone tried be "clever" >>>> here passing the same object here expecting _raw_copy_from_guest_buf() >>>> to DTRT, the function will end up copying too much data from the guest, >>>> and you will end up with something the guest wasn't intending to be part >>>> of the structure replacing the zero extension. >>>> >>>> New subops which take a longer object should use a brand new object. >>>> >>>> $MAGIC compatibility logic like you want has no business living in the >>>> copy helper. Had I spotted the intention during the original dmop >>>> series, I would have rejected it during review. >>> Actually, the existing behaviour is already broken. If a guest passes >>> an overly short buf[0], the dmop logic won't get a failure, and instead >>> get a truncated structure which has been zero extended. >>> >>> This is very definitely the wrong thing to do, because such a truncated >>> structure might actually contain some legitimate operations. >> So what, if that's what the caller meant to happen? >> >> Considering this is a controversial change, I think it is a bad idea >> to merge this into the otherwise uncontroversial change here. > > How about we move the min(..) and memset out of the helper function, and > into dm_op? Wouldn't that result in different buffers being treated differently, which I'd rather not see happening? Jan
On 21/04/17 10:46, Jan Beulich wrote: >>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: >> On 21/04/2017 09:54, Andrew Cooper wrote: >>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>> >>>>> This also allows the usual cases to be simplified, by omitting an >>>>> unnecessary >>>>> buf parameters, and because the macros can appropriately size the object. >>>>> >>>>> This makes copying to or from a buf that isn't big enough an error. >>>>> If the buffer isnt big enough, trying to carry on regardless >>>>> can only cause trouble later on. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>> ... this sequence of S-o-b-s? >>>> >>>>> --- a/xen/arch/x86/hvm/dm.c >>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>> struct xen_dm_op_buf buf[2]; >>>>> }; >>>>> >>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>> - unsigned int nr_bufs, void *dst, >>>>> - unsigned int idx, size_t dst_size) >>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>> + const struct dmop_args *args, >>>>> + unsigned int buf_idx, >>>>> + size_t dst_bytes) >>>>> { >>>>> - size_t size; >>>>> + size_t buf_bytes; >>>>> >>>>> - if ( idx >= nr_bufs ) >>>>> + if ( buf_idx >= args->nr_bufs ) >>>>> return false; >>>>> >>>>> - memset(dst, 0, dst_size); >>>>> + buf_bytes = args->buf[buf_idx].size; >>>>> >>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>> + if ( dst_bytes > buf_bytes ) >>>>> + return false; >>>> While this behavioral change is now being mentioned in the >>>> description, I'm not sure I buy the argument of basically being >>>> guaranteed to cause trouble down the road. Did you consider the >>>> forward compatibility aspect here, allowing us to extend interface >>>> structures by adding fields to their ends without breaking old >>>> callers? Paul, what are your thoughts here? >>> DMOP is a stable ABI. There is no legal extending of any objects. >>> >>> The previous semantics are guaranteed to break the ABI with future >>> subops, which is why I removed it. In the case that the guest >>> originally passed an overly long buffer, and someone tried be "clever" >>> here passing the same object here expecting _raw_copy_from_guest_buf() >>> to DTRT, the function will end up copying too much data from the guest, >>> and you will end up with something the guest wasn't intending to be part >>> of the structure replacing the zero extension. >>> >>> New subops which take a longer object should use a brand new object. >>> >>> $MAGIC compatibility logic like you want has no business living in the >>> copy helper. Had I spotted the intention during the original dmop >>> series, I would have rejected it during review. >> Actually, the existing behaviour is already broken. If a guest passes >> an overly short buf[0], the dmop logic won't get a failure, and instead >> get a truncated structure which has been zero extended. >> >> This is very definitely the wrong thing to do, because such a truncated >> structure might actually contain some legitimate operations. > So what, if that's what the caller meant to happen? If the caller doesn't provide all the information the hypervisor half of the interface expects, it should get a hard error, not a truncated attempt to fulful the request. The existing behaviour is simply broken. ~Andrew
On 21/04/17 11:38, Jan Beulich wrote: >>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote: >> On 21/04/17 10:46, Jan Beulich wrote: >>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: >>>> On 21/04/2017 09:54, Andrew Cooper wrote: >>>>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>>>> >>>>>>> This also allows the usual cases to be simplified, by omitting an >>>>>>> unnecessary >>>>>>> buf parameters, and because the macros can appropriately size the object. >>>>>>> >>>>>>> This makes copying to or from a buf that isn't big enough an error. >>>>>>> If the buffer isnt big enough, trying to carry on regardless >>>>>>> can only cause trouble later on. >>>>>>> >>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>>> ... this sequence of S-o-b-s? >>>>>> >>>>>>> --- a/xen/arch/x86/hvm/dm.c >>>>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>>>> struct xen_dm_op_buf buf[2]; >>>>>>> }; >>>>>>> >>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>>>> - unsigned int nr_bufs, void *dst, >>>>>>> - unsigned int idx, size_t dst_size) >>>>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>>>> + const struct dmop_args *args, >>>>>>> + unsigned int buf_idx, >>>>>>> + size_t dst_bytes) >>>>>>> { >>>>>>> - size_t size; >>>>>>> + size_t buf_bytes; >>>>>>> >>>>>>> - if ( idx >= nr_bufs ) >>>>>>> + if ( buf_idx >= args->nr_bufs ) >>>>>>> return false; >>>>>>> >>>>>>> - memset(dst, 0, dst_size); >>>>>>> + buf_bytes = args->buf[buf_idx].size; >>>>>>> >>>>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>>>> + if ( dst_bytes > buf_bytes ) >>>>>>> + return false; >>>>>> While this behavioral change is now being mentioned in the >>>>>> description, I'm not sure I buy the argument of basically being >>>>>> guaranteed to cause trouble down the road. Did you consider the >>>>>> forward compatibility aspect here, allowing us to extend interface >>>>>> structures by adding fields to their ends without breaking old >>>>>> callers? Paul, what are your thoughts here? >>>>> DMOP is a stable ABI. There is no legal extending of any objects. >>>>> >>>>> The previous semantics are guaranteed to break the ABI with future >>>>> subops, which is why I removed it. In the case that the guest >>>>> originally passed an overly long buffer, and someone tried be "clever" >>>>> here passing the same object here expecting _raw_copy_from_guest_buf() >>>>> to DTRT, the function will end up copying too much data from the guest, >>>>> and you will end up with something the guest wasn't intending to be part >>>>> of the structure replacing the zero extension. >>>>> >>>>> New subops which take a longer object should use a brand new object. >>>>> >>>>> $MAGIC compatibility logic like you want has no business living in the >>>>> copy helper. Had I spotted the intention during the original dmop >>>>> series, I would have rejected it during review. >>>> Actually, the existing behaviour is already broken. If a guest passes >>>> an overly short buf[0], the dmop logic won't get a failure, and instead >>>> get a truncated structure which has been zero extended. >>>> >>>> This is very definitely the wrong thing to do, because such a truncated >>>> structure might actually contain some legitimate operations. >>> So what, if that's what the caller meant to happen? >>> >>> Considering this is a controversial change, I think it is a bad idea >>> to merge this into the otherwise uncontroversial change here. >> How about we move the min(..) and memset out of the helper function, and >> into dm_op? > Wouldn't that result in different buffers being treated differently, > which I'd rather not see happening? I think in the general case, its wrong to assume that its ok to truncate a buffer. In specific cases, such as buf[0], we don't really know how big the buf needs to be until we start parsing it. In this case, we want to say 'give me what you have, upto this maximum'. This is the special case, not the general one, an as such does not belong in the general case accesser helper. Certainly when we get to updating the accesser helper to deal with offsets, its get more confused. If we give it an offset greater then the size of the buffer, do we just memset? That would seem wrong. With an offset of 0, we want it to do the memset thing, to deal with buf[0]. But what about if we have an offset. I'd think the calling function would really want to know if its got nonsense. Another solution would be to have two variants of the helper function. A 'normal' one, and the best effort one. -jenny
>>> On 21.04.17 at 13:03, <Jennifer.Herbert@citrix.com> wrote: > > On 21/04/17 11:38, Jan Beulich wrote: >>>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote: >>> On 21/04/17 10:46, Jan Beulich wrote: >>>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: >>>>> On 21/04/2017 09:54, Andrew Cooper wrote: >>>>>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: >>>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>>>>> >>>>>>>> This also allows the usual cases to be simplified, by omitting an >>>>>>>> unnecessary >>>>>>>> buf parameters, and because the macros can appropriately size the object. >>>>>>>> >>>>>>>> This makes copying to or from a buf that isn't big enough an error. >>>>>>>> If the buffer isnt big enough, trying to carry on regardless >>>>>>>> can only cause trouble later on. >>>>>>>> >>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> >>>>>>> ... this sequence of S-o-b-s? >>>>>>> >>>>>>>> --- a/xen/arch/x86/hvm/dm.c >>>>>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>>>>> struct xen_dm_op_buf buf[2]; >>>>>>>> }; >>>>>>>> >>>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>>>>> - unsigned int nr_bufs, void *dst, >>>>>>>> - unsigned int idx, size_t dst_size) >>>>>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>>>>> + const struct dmop_args *args, >>>>>>>> + unsigned int buf_idx, >>>>>>>> + size_t dst_bytes) >>>>>>>> { >>>>>>>> - size_t size; >>>>>>>> + size_t buf_bytes; >>>>>>>> >>>>>>>> - if ( idx >= nr_bufs ) >>>>>>>> + if ( buf_idx >= args->nr_bufs ) >>>>>>>> return false; >>>>>>>> >>>>>>>> - memset(dst, 0, dst_size); >>>>>>>> + buf_bytes = args->buf[buf_idx].size; >>>>>>>> >>>>>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>>>>> + if ( dst_bytes > buf_bytes ) >>>>>>>> + return false; >>>>>>> While this behavioral change is now being mentioned in the >>>>>>> description, I'm not sure I buy the argument of basically being >>>>>>> guaranteed to cause trouble down the road. Did you consider the >>>>>>> forward compatibility aspect here, allowing us to extend interface >>>>>>> structures by adding fields to their ends without breaking old >>>>>>> callers? Paul, what are your thoughts here? >>>>>> DMOP is a stable ABI. There is no legal extending of any objects. >>>>>> >>>>>> The previous semantics are guaranteed to break the ABI with future >>>>>> subops, which is why I removed it. In the case that the guest >>>>>> originally passed an overly long buffer, and someone tried be "clever" >>>>>> here passing the same object here expecting _raw_copy_from_guest_buf() >>>>>> to DTRT, the function will end up copying too much data from the guest, >>>>>> and you will end up with something the guest wasn't intending to be part >>>>>> of the structure replacing the zero extension. >>>>>> >>>>>> New subops which take a longer object should use a brand new object. >>>>>> >>>>>> $MAGIC compatibility logic like you want has no business living in the >>>>>> copy helper. Had I spotted the intention during the original dmop >>>>>> series, I would have rejected it during review. >>>>> Actually, the existing behaviour is already broken. If a guest passes >>>>> an overly short buf[0], the dmop logic won't get a failure, and instead >>>>> get a truncated structure which has been zero extended. >>>>> >>>>> This is very definitely the wrong thing to do, because such a truncated >>>>> structure might actually contain some legitimate operations. >>>> So what, if that's what the caller meant to happen? >>>> >>>> Considering this is a controversial change, I think it is a bad idea >>>> to merge this into the otherwise uncontroversial change here. >>> How about we move the min(..) and memset out of the helper function, and >>> into dm_op? >> Wouldn't that result in different buffers being treated differently, >> which I'd rather not see happening? > > I think in the general case, its wrong to assume that its ok to truncate > a buffer. > In specific cases, such as buf[0], we don't really know how big the buf > needs to be until we start parsing it. How that? Just like with domctl and sysctl the caller is supposed to be handing a full struct xen_dm_op, so I'd rather view this as the specific case where zero-extension is of no use. > Certainly when we get to updating the accesser helper to deal with > offsets, its get more confused. If we give it an offset greater then > the size of the buffer, do we just memset? That would seem wrong. With > an offset of 0, we want it to do the memset thing, to deal with buf[0]. Why? It's the natural extension of the zero-extending model. > But what about if we have an offset. I'd think the calling function > would really want to know if its got nonsense. Zero-extended input is not nonsense to me. > Another solution would be to have two variants of the helper function. A > 'normal' one, and the best effort one. I don't think we want that. Andrew being wholeheartedly opposed to this zero-extension approach, he wouldn't agree to that either afaict. Considering that I don't expect him to change his mind, I guess I'll give up opposition to the change done, provided it is being done as a separate patch. Of course I'd be interested to know Paul's position here, as back then he appeared to agree with using this model (or maybe it also was just giving in)... Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 21 April 2017 13:05 > To: Jennifer Herbert <jennifer.herbert@citrix.com> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; > Xen-devel <xen-devel@lists.xen.org> > Subject: Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to, > from}_guest_buf() in terms of raw accessors > > >>> On 21.04.17 at 13:03, <Jennifer.Herbert@citrix.com> wrote: > > > > > On 21/04/17 11:38, Jan Beulich wrote: > >>>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote: > >>> On 21/04/17 10:46, Jan Beulich wrote: > >>>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote: > >>>>> On 21/04/2017 09:54, Andrew Cooper wrote: > >>>>>> On 21/04/2017 08:27, Jan Beulich wrote: > >>>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote: > >>>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com> > >>>>>>> Is this correct, considering that iirc the patch was new in v5 and ... > >>>>>>> > >>>>>>>> This also allows the usual cases to be simplified, by omitting an > >>>>>>>> unnecessary > >>>>>>>> buf parameters, and because the macros can appropriately size > the object. > >>>>>>>> > >>>>>>>> This makes copying to or from a buf that isn't big enough an error. > >>>>>>>> If the buffer isnt big enough, trying to carry on regardless > >>>>>>>> can only cause trouble later on. > >>>>>>>> > >>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >>>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> > >>>>>>> ... this sequence of S-o-b-s? > >>>>>>> > >>>>>>>> --- a/xen/arch/x86/hvm/dm.c > >>>>>>>> +++ b/xen/arch/x86/hvm/dm.c > >>>>>>>> @@ -32,36 +32,47 @@ struct dmop_args { > >>>>>>>> struct xen_dm_op_buf buf[2]; > >>>>>>>> }; > >>>>>>>> > >>>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t > bufs[], > >>>>>>>> - unsigned int nr_bufs, void *dst, > >>>>>>>> - unsigned int idx, size_t dst_size) > >>>>>>>> +static bool _raw_copy_from_guest_buf(void *dst, > >>>>>>>> + const struct dmop_args *args, > >>>>>>>> + unsigned int buf_idx, > >>>>>>>> + size_t dst_bytes) > >>>>>>>> { > >>>>>>>> - size_t size; > >>>>>>>> + size_t buf_bytes; > >>>>>>>> > >>>>>>>> - if ( idx >= nr_bufs ) > >>>>>>>> + if ( buf_idx >= args->nr_bufs ) > >>>>>>>> return false; > >>>>>>>> > >>>>>>>> - memset(dst, 0, dst_size); > >>>>>>>> + buf_bytes = args->buf[buf_idx].size; > >>>>>>>> > >>>>>>>> - size = min_t(size_t, dst_size, bufs[idx].size); > >>>>>>>> + if ( dst_bytes > buf_bytes ) > >>>>>>>> + return false; > >>>>>>> While this behavioral change is now being mentioned in the > >>>>>>> description, I'm not sure I buy the argument of basically being > >>>>>>> guaranteed to cause trouble down the road. Did you consider the > >>>>>>> forward compatibility aspect here, allowing us to extend interface > >>>>>>> structures by adding fields to their ends without breaking old > >>>>>>> callers? Paul, what are your thoughts here? > >>>>>> DMOP is a stable ABI. There is no legal extending of any objects. > >>>>>> > >>>>>> The previous semantics are guaranteed to break the ABI with future > >>>>>> subops, which is why I removed it. In the case that the guest > >>>>>> originally passed an overly long buffer, and someone tried be > "clever" > >>>>>> here passing the same object here expecting > _raw_copy_from_guest_buf() > >>>>>> to DTRT, the function will end up copying too much data from the > guest, > >>>>>> and you will end up with something the guest wasn't intending to be > part > >>>>>> of the structure replacing the zero extension. > >>>>>> > >>>>>> New subops which take a longer object should use a brand new > object. > >>>>>> > >>>>>> $MAGIC compatibility logic like you want has no business living in the > >>>>>> copy helper. Had I spotted the intention during the original dmop > >>>>>> series, I would have rejected it during review. > >>>>> Actually, the existing behaviour is already broken. If a guest passes > >>>>> an overly short buf[0], the dmop logic won't get a failure, and instead > >>>>> get a truncated structure which has been zero extended. > >>>>> > >>>>> This is very definitely the wrong thing to do, because such a truncated > >>>>> structure might actually contain some legitimate operations. > >>>> So what, if that's what the caller meant to happen? > >>>> > >>>> Considering this is a controversial change, I think it is a bad idea > >>>> to merge this into the otherwise uncontroversial change here. > >>> How about we move the min(..) and memset out of the helper function, > and > >>> into dm_op? > >> Wouldn't that result in different buffers being treated differently, > >> which I'd rather not see happening? > > > > I think in the general case, its wrong to assume that its ok to truncate > > a buffer. > > In specific cases, such as buf[0], we don't really know how big the buf > > needs to be until we start parsing it. > > How that? Just like with domctl and sysctl the caller is supposed to > be handing a full struct xen_dm_op, so I'd rather view this as the > specific case where zero-extension is of no use. > > > Certainly when we get to updating the accesser helper to deal with > > offsets, its get more confused. If we give it an offset greater then > > the size of the buffer, do we just memset? That would seem wrong. With > > an offset of 0, we want it to do the memset thing, to deal with buf[0]. > > Why? It's the natural extension of the zero-extending model. > > > But what about if we have an offset. I'd think the calling function > > would really want to know if its got nonsense. > > Zero-extended input is not nonsense to me. > > > Another solution would be to have two variants of the helper function. A > > 'normal' one, and the best effort one. > > I don't think we want that. Andrew being wholeheartedly opposed > to this zero-extension approach, he wouldn't agree to that either > afaict. Considering that I don't expect him to change his mind, I > guess I'll give up opposition to the change done, provided it is > being done as a separate patch. Of course I'd be interested to > know Paul's position here, as back then he appeared to agree > with using this model (or maybe it also was just giving in)... > I've lost track of all the various arguments but my position is that zero extending makes no sense; the caller must always supply sufficient data. The length check in the helper function should not be an identity check though since not all the data from a particular buffer may be required in one go. Thus the patch, with the macro changes, should be ok. Paul > Jan
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index fb4bcec..3607ddb 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -32,36 +32,47 @@ struct dmop_args { struct xen_dm_op_buf buf[2]; }; -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], - unsigned int nr_bufs, void *dst, - unsigned int idx, size_t dst_size) +static bool _raw_copy_from_guest_buf(void *dst, + const struct dmop_args *args, + unsigned int buf_idx, + size_t dst_bytes) { - size_t size; + size_t buf_bytes; - if ( idx >= nr_bufs ) + if ( buf_idx >= args->nr_bufs ) return false; - memset(dst, 0, dst_size); + buf_bytes = args->buf[buf_idx].size; - size = min_t(size_t, dst_size, bufs[idx].size); + if ( dst_bytes > buf_bytes ) + return false; - return !copy_from_guest(dst, bufs[idx].h, size); + return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes); } -static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[], - unsigned int nr_bufs, unsigned int idx, - const void *src, size_t src_size) +static bool _raw_copy_to_guest_buf(struct dmop_args *args, + unsigned int buf_idx, + const void *src, size_t src_bytes) { - size_t size; + size_t buf_bytes; - if ( idx >= nr_bufs ) + if ( buf_idx >= args->nr_bufs ) return false; - size = min_t(size_t, bufs[idx].size, src_size); + buf_bytes = args->buf[buf_idx].size; + + if ( src_bytes > buf_bytes ) + return false; - return !copy_to_guest(bufs[idx].h, src, size); + return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes); } +#define copy_from_guest_buf(dst, args, buf_idx) \ + _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst))) + +#define copy_to_guest_buf(args, buf_idx, src) \ + _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src))) + static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn, unsigned int nr, struct xen_dm_op_buf *buf) { @@ -312,7 +323,7 @@ static int dm_op(struct dmop_args *op_args) if ( rc ) goto out; - if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) ) + if ( !copy_from_guest_buf(&op, op_args, 0) ); { rc = -EFAULT; goto out; @@ -568,8 +579,8 @@ static int dm_op(struct dmop_args *op_args) } if ( (!rc || rc == -ERESTART) && - !const_op && - !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) ) + !const_op && !copy_to_guest_buf(op_args, 0, &op) ) + rc = -EFAULT; out: