On Wed, Jul 25, 2018 at 03:06:21PM +0100, Dave Martin wrote: > On Thu, Jul 19, 2018 at 03:04:33PM +0200, Andrew Jones wrote: > > On Thu, Jun 21, 2018 at 03:57:38PM +0100, Dave Martin wrote: > > > + > > > + if (usize % sizeof(u32)) > > > + return -EINVAL; > > > > Currently we don't enforce the register size to be a multiple of 32 bits, > but I'm trying to establish a stronger position. Passing different > register sizes feels like an abuse of the API and there is no evidence > that qemu or kvmtool is relying on this so far. The ability to pass > a misaligned register ID and/or slurp multiple vcpu registers (or parts > of registers) is once call really seems like it works by accident today > and seems not to be intentional design. Rather, it exposes kernel > implementation details, which is best avoided. > > It would be better to make this a global check for usize % 32 == 0 > though, rather than burying it in fpsimd_vreg_bounds(). > > Opinions? There's only one reason to not start enforcing it globally on arm/arm64, and that's that it's not documented that way. Changing it would be an API change, rather than just an API fix. It's probably a safe change, but... > > > > + > > > + usize /= sizeof(u32); > > > + > > > + if ((uoffset <= start && usize <= start - uoffset) || > > > + uoffset >= limit) > > > + return -ENOENT; /* not a vreg */ > > > + > > > + BUILD_BUG_ON(uoffset > limit); > > > > Hmm, a build bug on uoffset can't be right, it's not a constant. > > > > > + if (uoffset < start || usize > limit - uoffset) > > > + return -EINVAL; /* overlaps vregs[] bounds */ > > uoffset is not compile-time constant, but (uoffset > limit) is compile- > time constant, because the previous if() returns from the function > otherwise. > > gcc seems to do the right thing here: the code compiles as-is, but > if the prior if() is commented out then the BUILD_BUG_ON() fires > because (uoffset > limit) is no longer compile-time constant. Oh, interesting. > > > This is a defensively-coded bounds check, where > > if (A + B > C) > > is transformed to > > if (C >= B && A > C - B) > > The former is susceptible to overflow in (A + B), whereas the latter is > not. We might be able to hide the risk with type casts, but that trades > one kind of fragility for another IMHO. > > In this patch, the C >= B part is subsumed into the previous if(), but > because this is non-obvious I dropped the BUILD_BUG_ON() in as a hint > to maintainers that we really do depend on a property of the previous > check, so although it may look like the checks could be swapped over > with no ill effects, really that is not safe. I'm glad our maintainers can pick up on hints like that :-) Maybe you can add a comment for mortals like me though. > > > Maybe the BUILD_BUG_ON() is superfluous, but I would prefer at least > to keep a comment here. > > What do you think. > Comment plus build-bug or just comment works for me. > > OTOH, if we can show conclusively that we can avoid overflow here > then the code can be simplified. But I would want to be confident > that this is really safe not just now but also under future maintenance. > I agree with thoroughly checking user input. Maybe we can create/use some helper functions to do it. Those helpers can then get reused elsewhere, helping to keep ourselves sane the next time we need to do similar sanity checks. Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
