Message ID | 1492783552-29472-3-git-send-email-jennifer.herbert@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> -----Original Message----- > From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com] > Sent: 21 April 2017 15:06 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jennifer Herbert > <jennifer.herbert@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan > Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com> > Subject: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, > from}_guest_buf() in terms of raw accessors > > From: Andrew Cooper <andrew.cooper3@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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com> Reviewed-by: Paul Durrant <paul.durrant@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> > --- > Rebased. > --- > xen/arch/x86/hvm/dm.c | 43 ++++++++++++++++++++++++++--------------- > -- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index 89186d2..b31c252 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -32,38 +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 buf_bytes; > > - if ( idx >= nr_bufs ) > + if ( buf_idx >= args->nr_bufs ) > return false; > > - buf_bytes = bufs[idx].size; > - if ( dst_size > buf_bytes ) > + buf_bytes = args->buf[buf_idx].size; > + > + if ( dst_bytes > buf_bytes ) > return false; > > - return !copy_from_guest(dst, bufs[idx].h, dst_size); > + return !copy_from_guest(dst, args->buf[buf_idx].h, dst_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(const struct dmop_args *args, > + unsigned int buf_idx, > + const void *src, size_t src_bytes) > { > size_t buf_bytes; > > - if ( idx >= nr_bufs ) > + if ( buf_idx >= args->nr_bufs ) > return false; > > - buf_bytes = bufs[idx].size; > - if ( src_size > buf_bytes ) > + buf_bytes = args->buf[buf_idx].size; > + > + if ( src_bytes > buf_bytes ) > return false; > > - return !copy_to_guest(bufs[idx].h, src, src_size); > + return !copy_to_guest(args->buf[buf_idx].h, src, src_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, const struct xen_dm_op_buf *buf) > { > @@ -314,7 +323,7 @@ static int dm_op(const 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; > @@ -570,8 +579,8 @@ static int dm_op(const 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 16:05, <jennifer.herbert@citrix.com> wrote: > +#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)) Why all caps all of the sudden? With this undone Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 21/04/17 16:45, Jan Beulich wrote: >>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote: >> +#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)) > Why all caps all of the sudden? This is the start of some code improvements, given the fallout from XSA-212. This macro is not a C function and doesn't behave like one (specifically, capturing src by name rather than value). Therefore, it gets ALL_CAPS() (which is actually traditional for any macro in C) to make it more obvious to people reading the code that it *is not* a C function and doesn't behave like one. It is getting embarrassing how many security vulnerability we are seeing because macros look like they are doing one thing, yet actually do something else, and improving the quality of the code is the only way this is going to get better. Therefore, I am going to insist that they stay like this. ~Andrew
>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote: > On 21/04/17 16:45, Jan Beulich wrote: >>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote: >>> +#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)) (Side note: src also isn't properly parenthesized, and the title went out of sync with the implementation.) >> Why all caps all of the sudden? > > This is the start of some code improvements, given the fallout from XSA-212. I don't think making the names shout is an improvement in any way. The #define-s above may still look fine, but the code using them is now looking plain ugly (even more so with the yet longer names introduced in patch 4). > This macro is not a C function and doesn't behave like one > (specifically, capturing src by name rather than value). Therefore, it > gets ALL_CAPS() (which is actually traditional for any macro in C) to I disagree - manifest constants may indeed be traditionally all caps, but not macros. Just look in the Library section of the C standard. In various cases it's not even enforced whether a given identifier is a macro or a variable/function. > make it more obvious to people reading the code that it *is not* a C > function and doesn't behave like one. > > It is getting embarrassing how many security vulnerability we are seeing > because macros look like they are doing one thing, yet actually do > something else, and improving the quality of the code is the only way > this is going to get better. Considering the "how many" you use, mind giving three examples where using all caps macro names would have made a difference? I sincerely doubt that the case used in identifiers would make a whole lot of a difference. > Therefore, I am going to insist that they stay like this. Well, how about I insist on names being made consistent again with what we have elsewhere, as they were in earlier versions? My conditional R-b stands in any event (plus the correction of the parenthesization issue mentioned above, also for patch 4). As a possible alternative, was it considered to pass pointers here as before, using __builtin_object_size() on them instead of the sizeof() above, and making the macros inline functions? Jan
On 24/04/17 09:19, Jan Beulich wrote: >>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote: >> On 21/04/17 16:45, Jan Beulich wrote: >>>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote: >>>> +#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)) > (Side note: src also isn't properly parenthesized, and the title went > out of sync with the implementation.) > >>> Why all caps all of the sudden? >> This is the start of some code improvements, given the fallout from XSA-212. > I don't think making the names shout is an improvement in any way. > The #define-s above may still look fine, but the code using them is > now looking plain ugly (even more so with the yet longer names > introduced in patch 4). That is a matter of opinion which I don't share, but ok. As an alternative, how else do you suggest making it obvious to the reader of the code that this thing which looks like a function doesn't have function semantics? (This is the purpose I am trying to get across.) >> make it more obvious to people reading the code that it *is not* a C >> function and doesn't behave like one. >> >> It is getting embarrassing how many security vulnerability we are seeing >> because macros look like they are doing one thing, yet actually do >> something else, and improving the quality of the code is the only way >> this is going to get better. > Considering the "how many" you use, mind giving three examples > where using all caps macro names would have made a difference? > I sincerely doubt that the case used in identifiers would make a > whole lot of a difference. You have missed my point then. We have many security vulnerabilities because we have deceptive code, and fix for that is to prevent the code being deceptive. This is going to positive code quality effort on our behalf, because the status quo is currently terrible. Most notably, XSA-212 just gone, where the root of the vulnerability is that "guest_handle_okay(base_ptr, array_element)" doesn't consider its second parameter, and degrades to checking just base_ptr. I accept that, in this case, capitalising the macro wouldn't help, but that is because its deceptive nature is in its naming, not because it behaves in a way contrary to a C function. > As a possible alternative, was it considered to pass pointers > here as before, using __builtin_object_size() on them instead of > the sizeof() above, and making the macros inline functions? I have never tried using it in anger, but looking into it, it degrades to (size_t)-1 in the case the compiler can't statically work out the correct value. As a result, you'd end up with a function which has gets() semantics (in the case that the compiler can't work out what's going on). I don't recommend we use any constructs like this. ~Andrew
>>> On 25.04.17 at 22:03, <andrew.cooper3@citrix.com> wrote: > On 24/04/17 09:19, Jan Beulich wrote: >>>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote: >>> On 21/04/17 16:45, Jan Beulich wrote: >>>>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote: >>>>> +#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)) >> (Side note: src also isn't properly parenthesized, and the title went >> out of sync with the implementation.) >> >>>> Why all caps all of the sudden? >>> This is the start of some code improvements, given the fallout from XSA-212. >> I don't think making the names shout is an improvement in any way. >> The #define-s above may still look fine, but the code using them is >> now looking plain ugly (even more so with the yet longer names >> introduced in patch 4). > > That is a matter of opinion which I don't share, but ok. > > As an alternative, how else do you suggest making it obvious to the > reader of the code that this thing which looks like a function doesn't > have function semantics? (This is the purpose I am trying to get across.) I do understand the intention, but I don't think capitalization of the names helps this in any way. This is not the least because there are compiler builtins which don't really have function semantics either (e.g. __builtin_constant_p(), __builtin_expect(), __builtin_types_compatible_p(), or __builtin_unreachable()). You won't be able to do anything about these, other than perhaps encapsulating them wrapper macros, but that still won't make the compiler mandated name all caps. Or take offsetof(), which following your argumentation then should be - contrary to the C standard - OFFSETOF(), while what we have as ASSERT() should then be - in line with the C standard, albeit our macro has slightly different behavior - assert(). >>> make it more obvious to people reading the code that it *is not* a C >>> function and doesn't behave like one. >>> >>> It is getting embarrassing how many security vulnerability we are seeing >>> because macros look like they are doing one thing, yet actually do >>> something else, and improving the quality of the code is the only way >>> this is going to get better. >> Considering the "how many" you use, mind giving three examples >> where using all caps macro names would have made a difference? >> I sincerely doubt that the case used in identifiers would make a >> whole lot of a difference. > > You have missed my point then. We have many security vulnerabilities > because we have deceptive code, and fix for that is to prevent the code > being deceptive. This is going to positive code quality effort on our > behalf, because the status quo is currently terrible. > > Most notably, XSA-212 just gone, where the root of the vulnerability is > that "guest_handle_okay(base_ptr, array_element)" doesn't consider its > second parameter, and degrades to checking just base_ptr. > > I accept that, in this case, capitalising the macro wouldn't help, but > that is because its deceptive nature is in its naming, not because it > behaves in a way contrary to a C function. A function could easily (and - by default - without warning) ignore one or more of the arguments passed to it, too. And as you say - XSA-212 is not an suitable example for your argumentation here. Once again - I don't think name capitalization helps in any way in what you want to ensure. And no, I don't think I've missed your point here, I'm merely questioning the measure you want to take to address your concerns. Therefore I'm still lacking any examples of XSAs supporting your position regarding identifier naming. >> As a possible alternative, was it considered to pass pointers >> here as before, using __builtin_object_size() on them instead of >> the sizeof() above, and making the macros inline functions? > > I have never tried using it in anger, but looking into it, it degrades > to (size_t)-1 in the case the compiler can't statically work out the > correct value. As a result, you'd end up with a function which has > gets() semantics (in the case that the compiler can't work out what's > going on). I don't recommend we use any constructs like this. Well, I wouldn't rule it out without a little bit of experimenting. While it can't be used in BUILD_BUG_ON(), it can very well be used in ways similar to e.g. {read,write}_atomic(), adding a reference to an always undefined symbol in the case the builtin produces 0 or -1. On the grounds of wanting to not delay this series any further, I'm about to commit it (albeit I note patch 2 is lacking a proper maintainer ack, and I'm not going to add mine), but I'll make my disagreement with the choice of macro names explicit, so these names can't later be used as a reference to support naming other macros in similarly ugly ways. Jan
>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote: > @@ -314,7 +323,7 @@ static int dm_op(const 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) ); Btw, I also had to remove the bogus semicolon here, causing a build failure, and ... > @@ -570,8 +579,8 @@ static int dm_op(const 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; ... I've taken the liberty to remove this stray blank line. Both are likely a sign of this having been put together in too much of a hurry, and the former suggests this version hasn't been tested at all. Jan
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index 89186d2..b31c252 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -32,38 +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 buf_bytes; - if ( idx >= nr_bufs ) + if ( buf_idx >= args->nr_bufs ) return false; - buf_bytes = bufs[idx].size; - if ( dst_size > buf_bytes ) + buf_bytes = args->buf[buf_idx].size; + + if ( dst_bytes > buf_bytes ) return false; - return !copy_from_guest(dst, bufs[idx].h, dst_size); + return !copy_from_guest(dst, args->buf[buf_idx].h, dst_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(const struct dmop_args *args, + unsigned int buf_idx, + const void *src, size_t src_bytes) { size_t buf_bytes; - if ( idx >= nr_bufs ) + if ( buf_idx >= args->nr_bufs ) return false; - buf_bytes = bufs[idx].size; - if ( src_size > buf_bytes ) + buf_bytes = args->buf[buf_idx].size; + + if ( src_bytes > buf_bytes ) return false; - return !copy_to_guest(bufs[idx].h, src, src_size); + return !copy_to_guest(args->buf[buf_idx].h, src, src_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, const struct xen_dm_op_buf *buf) { @@ -314,7 +323,7 @@ static int dm_op(const 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; @@ -570,8 +579,8 @@ static int dm_op(const 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: