Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 22 Aug 2023 10:35:30 -0700 | Subject | Re: [PATCH next v3 0/5] minmax: Relax type checks in min() and max(). |
| |
On Mon, 21 Aug 2023 at 11:24, Kees Cook <keescook@chromium.org> wrote: > > It seems like the foot-gun problems are when a value gets clamped by the > imposed type. Can't we just warn about those cases?
I think that the problem with min_t() is that it is used as a random "when min() warns about different types", and that it basically just adds a cast without any semantic meaning.
End result: we currently have 4500+ of those cases (and another 1300 uses of 'max_t') and I bet that several of them are the narrowing kind. And some are probably valid.
And if we tighten up "min_t()" type rules, we'll just hit the *next* problem in the series, namely "what are people going to do now?"
We don't want to just keep pushing the problem down.
So I actually mostly liked all the *other* patches in David's series: using 'min_unsigned()' and friends adds a nice *semantic* layer, not just a cast. And relaxing the checking of min/max doesn't cause the same the "push problems down" issue, as long as the relaxing is reasonable.
(Side note: I'm not convinced 'min_unsigned()' is the right syntax. While I like the concept, I think 'min()' is often used as a part of other expressions, and 'min_unsigned()' ends up making for a very illegible long and complex thing. I think we might as well use 'umin()/umax()', matching our type system).
It's just that I very much don't think it's reasonable to relax "20u" (or - more commonly - sizeof) to be any random constant signed integer, and it should *not* compare well with signed integers and not silently become a signed compare.
(But I do think that it's very ok to go the other way: compare a _unsigned_ value with a "signed" constant integer like 20. The two cases are not symmetrical: '20' can be a perfectly fine unsigned value, but '20u' cannot be treated signed).
And while I don't like David's patch to silently turn unsigned constant signed, I do acknowledge that very often the *source* of the unsignedness is a 'sizeof()' expression, and then you have an integer that gets compared to a size, and you end up using 'min_t()'. But I do *NOT* want to fix those cases by ignoring the signedness.
Just a quick grep of
git grep 'min_t(size_t' | wc
shows that quite a lot of the 'min_t()' cases are this exact issue. But I absolutely do *not* think the solution is to relax 'min()'.
I suspect the fix to those cases is to much more eagerly use 'clamp()'. Almost every single time you do a "compare to a size", it really is "make sure the integer is within the size range". So while
int val ... x = min(val,sizeof(xyz));
is horrendously wrong and *should* warn, I think doing
x = clamp(val, 0, sizeof(xyz));
is a *much* nicer model, and should not warn even if "val" and the upper bound do not agree. In the above kind of situation, suddenly it *is* ok to treat the 'sizeof()' as a signed integer, but only because we have that explicit lower bound too.
In other words: we should not "try to fix the types". That was what caused the problem in the first place, with "min_t()" just trying to fix the type mismatch with a cast. Casts are wrong, and we should have known that. The end result is horrendous, and I do agree with David on that too.
I think we should strive to fix it with "semantic" fixes instead. Like the above "use clamp() instead of min(), and the fundamental signedness problem simply goes away because it has enough semantic meaning to be well-defined".
Hmm?
Linus
| |