On Wed, Jul 25, 2018 at 07:20:57PM +0200, Andrew Jones wrote: > 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... I agree, though there are few direct users of this API, and I couldn't come up with a scenario where anyone in their right mind would access the core regs struct with access sizes <= 16 bits, and I've seen no evidence so far of the API being used in this way. So it would be nice to close this hole before it springs a leak. I'll keep if for now, but flag it up for attention in the repost. I'm happy to drop it if people care strongly enough. > > > > + > > > > + 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. Hint taken... I'll add a comment. No doubt I'd eventually forget why the BUILD_BUG_ON() was there too. > > 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. It's a bit tricky to get right, because it all depends on the combination of types being used in the expression. I might have a think about how to do this, but for now I don't want to introduce more churn. Cheers ---Dave _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
