Message ID | 20210121212718.2441-5-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Implement support for external IPT monitoring | expand |
On 21.01.2021 22:27, Andrew Cooper wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d, > case XENMEM_resource_grant_table: > return gnttab_resource_max_frames(d, id); > > + case XENMEM_resource_vmtrace_buf: > + return d->vmtrace_frames; > + > default: > return arch_resource_max_frames(d, type, id); > } > } > > +static int acquire_vmtrace_buf( > + struct domain *d, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]) > +{ > + const struct vcpu *v = domain_vcpu(d, id); > + unsigned int i; > + mfn_t mfn; > + > + if ( !v || !v->vmtrace.buf || > + nr_frames > d->vmtrace_frames || > + (frame + nr_frames) > d->vmtrace_frames ) > + return -EINVAL; I think that for this to guard against overflow, the first nr_frames needs to be replaced by frame (as having the wider type), or else a very large value of frame coming in will not yield the intended -EINVAL. If you agree, with this changed, Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 25.01.2021 17:31, Jan Beulich wrote: > On 21.01.2021 22:27, Andrew Cooper wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d, >> case XENMEM_resource_grant_table: >> return gnttab_resource_max_frames(d, id); >> >> + case XENMEM_resource_vmtrace_buf: >> + return d->vmtrace_frames; >> + >> default: >> return arch_resource_max_frames(d, type, id); >> } >> } >> >> +static int acquire_vmtrace_buf( >> + struct domain *d, unsigned int id, unsigned long frame, >> + unsigned int nr_frames, xen_pfn_t mfn_list[]) >> +{ >> + const struct vcpu *v = domain_vcpu(d, id); >> + unsigned int i; >> + mfn_t mfn; >> + >> + if ( !v || !v->vmtrace.buf || >> + nr_frames > d->vmtrace_frames || >> + (frame + nr_frames) > d->vmtrace_frames ) >> + return -EINVAL; > > > I think that for this to guard against overflow, the first nr_frames > needs to be replaced by frame (as having the wider type), or else a > very large value of frame coming in will not yield the intended > -EINVAL. Actually, besides this then wanting to be >= instead of >, this wouldn't take care of the 32-bit case (or more generally the sizeof(long) == sizeof(int) one). So I think you want if ( !v || !v->vmtrace.buf || (frame + nr_frames) < frame || (frame + nr_frames) > d->vmtrace_frames ) return -EINVAL; > If you agree, with this changed, > Reviewed-by: Jan Beulich <jbeulich@suse.com> This holds. Jan
On 26/01/2021 07:37, Jan Beulich wrote: > On 25.01.2021 17:31, Jan Beulich wrote: >> On 21.01.2021 22:27, Andrew Cooper wrote: >>> --- a/xen/common/memory.c >>> +++ b/xen/common/memory.c >>> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d, >>> case XENMEM_resource_grant_table: >>> return gnttab_resource_max_frames(d, id); >>> >>> + case XENMEM_resource_vmtrace_buf: >>> + return d->vmtrace_frames; >>> + >>> default: >>> return arch_resource_max_frames(d, type, id); >>> } >>> } >>> >>> +static int acquire_vmtrace_buf( >>> + struct domain *d, unsigned int id, unsigned long frame, >>> + unsigned int nr_frames, xen_pfn_t mfn_list[]) >>> +{ >>> + const struct vcpu *v = domain_vcpu(d, id); >>> + unsigned int i; >>> + mfn_t mfn; >>> + >>> + if ( !v || !v->vmtrace.buf || >>> + nr_frames > d->vmtrace_frames || >>> + (frame + nr_frames) > d->vmtrace_frames ) >>> + return -EINVAL; >> >> I think that for this to guard against overflow, the first nr_frames >> needs to be replaced by frame (as having the wider type), or else a >> very large value of frame coming in will not yield the intended >> -EINVAL. > Actually, besides this then wanting to be >= instead of >, this > wouldn't take care of the 32-bit case (or more generally the > sizeof(long) == sizeof(int) one). So I think you want > > if ( !v || !v->vmtrace.buf || > (frame + nr_frames) < frame || > (frame + nr_frames) > d->vmtrace_frames ) > return -EINVAL; > >> If you agree, with this changed, >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > This holds. I slipped this buggy version in to prove a point. You're now 3 or 4 attempts into "simplifying" my original version, and have on at least 2 attempts made your R-b conditional on a buggy version. This form is clearly too complicated to reason about correctly, and it is definitely more complicated than I am happy taking. I am either going to go with my original version, which is trivially and obviously correct, or I'm considering reducing frame to 32 bits at the top level to fix this width nonsense throughout Xen. ~Andrew
On 26.01.2021 10:58, Andrew Cooper wrote: > On 26/01/2021 07:37, Jan Beulich wrote: >> On 25.01.2021 17:31, Jan Beulich wrote: >>> On 21.01.2021 22:27, Andrew Cooper wrote: >>>> +static int acquire_vmtrace_buf( >>>> + struct domain *d, unsigned int id, unsigned long frame, >>>> + unsigned int nr_frames, xen_pfn_t mfn_list[]) >>>> +{ >>>> + const struct vcpu *v = domain_vcpu(d, id); >>>> + unsigned int i; >>>> + mfn_t mfn; >>>> + >>>> + if ( !v || !v->vmtrace.buf || >>>> + nr_frames > d->vmtrace_frames || >>>> + (frame + nr_frames) > d->vmtrace_frames ) >>>> + return -EINVAL; >>> >>> I think that for this to guard against overflow, the first nr_frames >>> needs to be replaced by frame (as having the wider type), or else a >>> very large value of frame coming in will not yield the intended >>> -EINVAL. >> Actually, besides this then wanting to be >= instead of >, this >> wouldn't take care of the 32-bit case (or more generally the >> sizeof(long) == sizeof(int) one). So I think you want >> >> if ( !v || !v->vmtrace.buf || >> (frame + nr_frames) < frame || >> (frame + nr_frames) > d->vmtrace_frames ) >> return -EINVAL; >> >>> If you agree, with this changed, >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> This holds. > > I slipped this buggy version in to prove a point. IOW you've been intentionally submitting buggy code. Very interesting. > You're now 3 or 4 attempts into "simplifying" my original version, and > have on at least 2 attempts made your R-b conditional on a buggy version. In which way is the last proposed version buggy, and in which way was the intermediate proposal problematic beyond the aspects I did recognize myself? (I also see no problem with taking a number of iterations to arrive at the correct result, and I also wouldn't view this happening as an indication that an initial comment was wrong then, unless the final result of this iterative process matches what there was originally.) > This form is clearly too complicated to reason about correctly, and it > is definitely more complicated than I am happy taking. > > > I am either going to go with my original version, which is trivially and > obviously correct, I've just tried to locate your "original version" in my mailbox. I don't have an earlier patch there with this same title. Without being able to locate the prior suggestion of mine, I'm afraid I won't be able to verify if indeed I did suggest the variant above before; I wouldn't consider it very likely though. In any event I think it would have helped more if you had proven to me where I'm wrong; I can be convinced, but calling something "trivially and obviously correct" is not a technical statement in such a situation. It instead feels more like a killer phrase. By implication, you saying "trivially and obviously correct" can really mean only one of two things if indeed I had found a need to comment on this same piece of code (under a different title) earlier on: I'm trivially and obviously stupid (and would better go away), or you're wrong with the statement (at least in assuming what's trivial and obvious to you also necessarily is to everyone else). I'm sorry to say it this bluntly, but your reply above feels pretty blunt as well. > or I'm considering reducing frame to 32 bits at the > top level to fix this width nonsense throughout Xen. I wouldn't mind this (and I've been wondering about the "unsigned long" a number of times), but I'm afraid I don't see how your construct above would be correctly rejecting all overflowing cases then. Jan
diff --git a/xen/common/memory.c b/xen/common/memory.c index c89923d909..ec6a55172a 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d, case XENMEM_resource_grant_table: return gnttab_resource_max_frames(d, id); + case XENMEM_resource_vmtrace_buf: + return d->vmtrace_frames; + default: return arch_resource_max_frames(d, type, id); } } +static int acquire_vmtrace_buf( + struct domain *d, unsigned int id, unsigned long frame, + unsigned int nr_frames, xen_pfn_t mfn_list[]) +{ + const struct vcpu *v = domain_vcpu(d, id); + unsigned int i; + mfn_t mfn; + + if ( !v || !v->vmtrace.buf || + nr_frames > d->vmtrace_frames || + (frame + nr_frames) > d->vmtrace_frames ) + return -EINVAL; + + mfn = page_to_mfn(v->vmtrace.buf); + + for ( i = 0; i < nr_frames; i++ ) + mfn_list[i] = mfn_x(mfn) + frame + i; + + return nr_frames; +} + /* * Returns -errno on error, or positive in the range [1, nr_frames] on * success. Returning less than nr_frames contitutes a request for a @@ -1087,6 +1111,9 @@ static int _acquire_resource( case XENMEM_resource_grant_table: return gnttab_acquire_resource(d, id, frame, nr_frames, mfn_list); + case XENMEM_resource_vmtrace_buf: + return acquire_vmtrace_buf(d, id, frame, nr_frames, mfn_list); + default: return arch_acquire_resource(d, type, id, frame, nr_frames, mfn_list); } diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index c4c47a0b38..b0378bb14b 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -625,6 +625,7 @@ struct xen_mem_acquire_resource { #define XENMEM_resource_ioreq_server 0 #define XENMEM_resource_grant_table 1 +#define XENMEM_resource_vmtrace_buf 2 /* * IN - a type-specific resource identifier, which must be zero