| Message ID | 1521143266-31350-2-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook <keescook@chromium.org> wrote: > > To gain the ability to compare differing types, the arguments are > explicitly cast to size_t. Ugh, I really hate this. It silently does insane things if you do const_max(-1,6) and there is nothing in the name that implies that you can't use negative constants. Linus
On Thu, Mar 15, 2018 at 2:42 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook <keescook@chromium.org> wrote: >> >> To gain the ability to compare differing types, the arguments are >> explicitly cast to size_t. > > Ugh, I really hate this. > > It silently does insane things if you do > > const_max(-1,6) > > and there is nothing in the name that implies that you can't use > negative constants. Yeah, I didn't like that effect either. I was seeing this: ./include/linux/kernel.h:836:14: warning: comparison between ‘enum <anonymous>’ and ‘enum <anonymous>’ [-Wenum-compare] (x) > (y) ? \ ^ ./include/linux/kernel.h:838:7: note: in definition of macro ‘const_max’ (y), \ ^ net/ipv6/proc.c:34:11: note: in expansion of macro ‘const_max’ const_max(IPSTATS_MIB_MAX, ICMP_MIB_MAX)) ^~~~~~~~~ But it turns out that just doing a typeof() fixes this, and there's no need for the hard cast to size_t: size_t __error_not_const_arg(void) \ __compiletime_error("const_max() used with non-compile-time constant arg"); #define const_max(x, y) \ __builtin_choose_expr(__builtin_constant_p(x) && \ __builtin_constant_p(y), \ (typeof(x))(x) > (typeof(y))(y) ? \ (x) : (y), \ __error_not_const_arg()) Is typeof() forcing enums to int? Regardless, I'll put this through larger testing. How does that look? -Kees
On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> wrote: > > size_t __error_not_const_arg(void) \ > __compiletime_error("const_max() used with non-compile-time constant arg"); > #define const_max(x, y) \ > __builtin_choose_expr(__builtin_constant_p(x) && \ > __builtin_constant_p(y), \ > (typeof(x))(x) > (typeof(y))(y) ? \ > (x) : (y), \ > __error_not_const_arg()) > > Is typeof() forcing enums to int? Regardless, I'll put this through > larger testing. How does that look? Ok, that alleviates my worry about one class of insane behavior, but it does raise a few other questions: - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky. - this does have the usual "what happen if you do const_max(-1,sizeof(x)) where the comparison will now be done in 'size_t', and -1 ends up being a very very big unsigned integer. Is there no way to get that type checking inserted? Maybe now is a good point for that __builtin_types_compatible(), and add it to the constness checking (and change the name of that error case function)? Linus
On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> wrote: >> >> size_t __error_not_const_arg(void) \ >> __compiletime_error("const_max() used with non-compile-time constant arg"); >> #define const_max(x, y) \ >> __builtin_choose_expr(__builtin_constant_p(x) && \ >> __builtin_constant_p(y), \ >> (typeof(x))(x) > (typeof(y))(y) ? \ >> (x) : (y), \ >> __error_not_const_arg()) >> >> Is typeof() forcing enums to int? Regardless, I'll put this through >> larger testing. How does that look? > > Ok, that alleviates my worry about one class of insane behavior, but > it does raise a few other questions: > > - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky. Yeah, that's why I didn't even try that originally. But in looking back at max() again, it seemed to be the only thing missing that would handle the enum evaluation, which turned out to be true. > - this does have the usual "what happen if you do > > const_max(-1,sizeof(x)) > > where the comparison will now be done in 'size_t', and -1 ends up > being a very very big unsigned integer. > > Is there no way to get that type checking inserted? Maybe now is a > good point for that __builtin_types_compatible(), and add it to the > constness checking (and change the name of that error case function)? So, AIUI, I can either get strict type checking, in which case, this is rejected (which I assume there is still a desire to have): int foo[const_max(6, sizeof(whatever))]; due to __builtin_types_compatible_p() rejecting it, or I can construct a "positive arguments only" test, in which the above is accepted, but this is rejected: int foo[const_max(-1, sizeof(whatever))]; By using this eye-bleed: size_t __error_not_const_arg(void) \ __compiletime_error("const_max() used with non-compile-time constant arg"); size_t __error_not_positive_arg(void) \ __compiletime_error("const_max() used with negative arg"); #define const_max(x, y) \ __builtin_choose_expr(__builtin_constant_p(x) && \ __builtin_constant_p(y), \ __builtin_choose_expr((x) >= 0 && (y) >= 0, \ (typeof(x))(x) > (typeof(y))(y) ? \ (x) : (y), \ __error_not_positive_arg()), \ __error_not_const_arg()) -Kees
On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keescook@chromium.org> wrote: >>> >>> size_t __error_not_const_arg(void) \ >>> __compiletime_error("const_max() used with non-compile-time constant arg"); >>> #define const_max(x, y) \ >>> __builtin_choose_expr(__builtin_constant_p(x) && \ >>> __builtin_constant_p(y), \ >>> (typeof(x))(x) > (typeof(y))(y) ? \ >>> (x) : (y), \ >>> __error_not_const_arg()) >>> >>> Is typeof() forcing enums to int? Regardless, I'll put this through >>> larger testing. How does that look? >> >> Ok, that alleviates my worry about one class of insane behavior, but >> it does raise a few other questions: >> >> - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky. > > Yeah, that's why I didn't even try that originally. But in looking > back at max() again, it seemed to be the only thing missing that would > handle the enum evaluation, which turned out to be true. > >> - this does have the usual "what happen if you do >> >> const_max(-1,sizeof(x)) >> >> where the comparison will now be done in 'size_t', and -1 ends up >> being a very very big unsigned integer. >> >> Is there no way to get that type checking inserted? Maybe now is a >> good point for that __builtin_types_compatible(), and add it to the >> constness checking (and change the name of that error case function)? > > So, AIUI, I can either get strict type checking, in which case, this > is rejected (which I assume there is still a desire to have): > > int foo[const_max(6, sizeof(whatever))]; Is it that bad to just call it with (size_t)6? > > due to __builtin_types_compatible_p() rejecting it, or I can construct > a "positive arguments only" test, in which the above is accepted, but > this is rejected: > > int foo[const_max(-1, sizeof(whatever))]; Do we need this case? > > By using this eye-bleed: > > size_t __error_not_const_arg(void) \ > __compiletime_error("const_max() used with non-compile-time constant arg"); > size_t __error_not_positive_arg(void) \ > __compiletime_error("const_max() used with negative arg"); > #define const_max(x, y) \ > __builtin_choose_expr(__builtin_constant_p(x) && \ > __builtin_constant_p(y), \ > __builtin_choose_expr((x) >= 0 && (y) >= 0, \ > (typeof(x))(x) > (typeof(y))(y) ? \ > (x) : (y), \ > __error_not_positive_arg()), \ > __error_not_const_arg()) > I was writing it like this: #define const_max(a, b) \ ({ \ if ((a) < 0) \ __const_max_called_with_negative_value(); \ if ((b) < 0) \ __const_max_called_with_negative_value(); \ if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \ __const_max_called_with_incompatible_types(); \ __builtin_choose_expr((a) > (b), (a), (b)); \ }) Cheers, Miguel > -Kees > > -- > Kees Cook > Pixel Security
On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> wrote: >> >> By using this eye-bleed: >> >> size_t __error_not_const_arg(void) \ >> __compiletime_error("const_max() used with non-compile-time constant arg"); >> size_t __error_not_positive_arg(void) \ >> __compiletime_error("const_max() used with negative arg"); >> #define const_max(x, y) \ >> __builtin_choose_expr(__builtin_constant_p(x) && \ >> __builtin_constant_p(y), \ >> __builtin_choose_expr((x) >= 0 && (y) >= 0, \ >> (typeof(x))(x) > (typeof(y))(y) ? \ >> (x) : (y), \ >> __error_not_positive_arg()), \ >> __error_not_const_arg()) >> > > I was writing it like this: > > #define const_max(a, b) \ > ({ \ > if ((a) < 0) \ > __const_max_called_with_negative_value(); \ > if ((b) < 0) \ > __const_max_called_with_negative_value(); \ > if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \ > __const_max_called_with_incompatible_types(); \ > __builtin_choose_expr((a) > (b), (a), (b)); \ > }) The full one, using your naming convention: #define const_max(x, y) \ ({ \ if (!__builtin_constant_p(x)) \ __error_not_const_arg(); \ if (!__builtin_constant_p(y)) \ __error_not_const_arg(); \ if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \ __error_incompatible_types(); \ if ((x) < 0) \ __error_not_positive_arg(); \ if ((y) < 0) \ __error_not_positive_arg(); \ __builtin_choose_expr((x) > (y), (x), (y)); \ }) Miguel
On Fri, Mar 16, 2018 at 12:08 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Thu, Mar 15, 2018 at 11:58 PM, Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >> On Thu, Mar 15, 2018 at 11:46 PM, Kees Cook <keescook@chromium.org> wrote: >>> >>> By using this eye-bleed: >>> >>> size_t __error_not_const_arg(void) \ >>> __compiletime_error("const_max() used with non-compile-time constant arg"); >>> size_t __error_not_positive_arg(void) \ >>> __compiletime_error("const_max() used with negative arg"); >>> #define const_max(x, y) \ >>> __builtin_choose_expr(__builtin_constant_p(x) && \ >>> __builtin_constant_p(y), \ >>> __builtin_choose_expr((x) >= 0 && (y) >= 0, \ >>> (typeof(x))(x) > (typeof(y))(y) ? \ >>> (x) : (y), \ >>> __error_not_positive_arg()), \ >>> __error_not_const_arg()) >>> >> >> I was writing it like this: >> >> #define const_max(a, b) \ >> ({ \ >> if ((a) < 0) \ >> __const_max_called_with_negative_value(); \ >> if ((b) < 0) \ >> __const_max_called_with_negative_value(); \ >> if (!__builtin_types_compatible_p(typeof(a), typeof(b))) \ >> __const_max_called_with_incompatible_types(); \ >> __builtin_choose_expr((a) > (b), (a), (b)); \ >> }) > > The full one, using your naming convention: > > #define const_max(x, y) \ > ({ \ > if (!__builtin_constant_p(x)) \ > __error_not_const_arg(); \ > if (!__builtin_constant_p(y)) \ > __error_not_const_arg(); \ > if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \ > __error_incompatible_types(); \ > if ((x) < 0) \ > __error_not_positive_arg(); \ > if ((y) < 0) \ > __error_not_positive_arg(); \ > __builtin_choose_expr((x) > (y), (x), (y)); \ > }) > Nevermind... gcc doesn't take that as a constant expr, even if it compiles as one at -O0.
On Thu, Mar 15, 2018 at 4:17 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: >> The full one, using your naming convention: >> >> #define const_max(x, y) \ >> ({ \ >> if (!__builtin_constant_p(x)) \ >> __error_not_const_arg(); \ >> if (!__builtin_constant_p(y)) \ >> __error_not_const_arg(); \ >> if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \ >> __error_incompatible_types(); \ >> if ((x) < 0) \ >> __error_not_positive_arg(); \ >> if ((y) < 0) \ >> __error_not_positive_arg(); \ >> __builtin_choose_expr((x) > (y), (x), (y)); \ >> }) >> > > Nevermind... gcc doesn't take that as a constant expr, even if it > compiles as one at -O0. Yeah, unfortunately. :( -Kees
On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook <keescook@chromium.org> wrote: > > So, AIUI, I can either get strict type checking, in which case, this > is rejected (which I assume there is still a desire to have): > > int foo[const_max(6, sizeof(whatever))]; Ehh, yes, that looks fairly sane, and erroring out would be annoying. But maybe we should just make the type explicit, and make it "const_max_t()"? I think all the existing users are of type "max_t()" anyway due to the very same issue, no? At least if there's an explicit type like 'size_t', then passing in "-1" becoming a large unsigned integer is understandable and clear, not just some odd silent behavior. Put another way: I think it's unacceptable that const_max(-1,6) magically becomes a huge positive number like in that patch of yours, but const_max_t(size_t, -1, 6) *obviously* is a huge positive number. The two things would *do* the same thing, but in the second case the type is explicit and visible. > due to __builtin_types_compatible_p() rejecting it, or I can construct > a "positive arguments only" test, in which the above is accepted, but > this is rejected: That sounds acceptable too, although the "const_max_t()" thing is presumably going to be simpler? Linus
On Thu, Mar 15, 2018 at 4:34 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook <keescook@chromium.org> wrote: >> >> So, AIUI, I can either get strict type checking, in which case, this >> is rejected (which I assume there is still a desire to have): >> >> int foo[const_max(6, sizeof(whatever))]; > > Ehh, yes, that looks fairly sane, and erroring out would be annoying. > > But maybe we should just make the type explicit, and make it "const_max_t()"? > > I think all the existing users are of type "max_t()" anyway due to the > very same issue, no? All but one are using max()[1]. One case uses max_t() to get u32. > At least if there's an explicit type like 'size_t', then passing in > "-1" becoming a large unsigned integer is understandable and clear, > not just some odd silent behavior. > > Put another way: I think it's unacceptable that > > const_max(-1,6) > > magically becomes a huge positive number like in that patch of yours, but > > const_max_t(size_t, -1, 6) > > *obviously* is a huge positive number. > > The two things would *do* the same thing, but in the second case the > type is explicit and visible. > >> due to __builtin_types_compatible_p() rejecting it, or I can construct >> a "positive arguments only" test, in which the above is accepted, but >> this is rejected: > > That sounds acceptable too, although the "const_max_t()" thing is > presumably going to be simpler? I much prefer explicit typing, but both you and Rasmus mentioned wanting the int/sizeof_t mixing. I'm totally happy with const_max_t() -- even if it makes my line-wrapping harder due to the longer name. ;) I'll resend in a moment... -Kees [1] https://patchwork.kernel.org/patch/10285709/
On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook <keescook@chromium.org> wrote: > > I much prefer explicit typing, but both you and Rasmus mentioned > wanting the int/sizeof_t mixing. Well, the explicit typing allows that mixing, in that you can just have "const_max_t(5,sizeof(x))" So I'm ok with that. What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring out, or silently causing insane behavior due to hidden subtle type casts.. Linus
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Well, the explicit typing allows that mixing, in that you can just > have "const_max_t(5,sizeof(x))" I obviously meant "const_max_t(size_t,5,sizeof(x))". Heh. Linus
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring > out, or silently causing insane behavior due to hidden subtle type > casts.. Yup! I like it as an explicit argument. Thanks! -Kees
On Fri, Mar 16, 2018 at 12:49 AM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring >> out, or silently causing insane behavior due to hidden subtle type >> casts.. > > Yup! I like it as an explicit argument. Thanks! > What about something like this? #define INTMAXT_MAX LLONG_MAX typedef int64_t intmax_t; #define const_max(x, y) \ __builtin_choose_expr( \ !__builtin_constant_p(x) || !__builtin_constant_p(y), \ __error_not_const_arg(), \ __builtin_choose_expr( \ (x) > INTMAXT_MAX || (y) > INTMAXT_MAX, \ __error_too_big(), \ __builtin_choose_expr( \ (intmax_t)(x) >= (intmax_t)(y), \ (x), \ (y) \ ) \ ) \ ) Works for different types, allows to mix negatives and positives and returns the original type, e.g.: const_max(-1, sizeof(char)); is of type 'long unsigned int', but: const_max(2, sizeof(char)); is of type 'int'. While I am not a fan that the return type depends on the arguments, it is useful if you are going to use the expression in something that needs expects a precise (a printk() for instance?). The check against the INTMAXT_MAX is there to avoid complexity (if we do not handle those cases, it is safe to use intmax_t for the comparison; otherwise you have to have another compile time branch for the case positive-positive using uintmax_t) and also avoids odd warnings for some cases above LLONG_MAX about comparisons with 0 for unsigned expressions being always true. On the positive side, it prevents using the macro for thing like "(size_t)-1". Cheers, Miguel
On 2018-03-16 00:46, Linus Torvalds wrote: > On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook <keescook@chromium.org> wrote: >> >> I much prefer explicit typing, but both you and Rasmus mentioned >> wanting the int/sizeof_t mixing. > > Well, the explicit typing allows that mixing, in that you can just > have "const_max_t(5,sizeof(x))" > > So I'm ok with that. > > What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring > out, or silently causing insane behavior due to hidden subtle type > casts.. I don't like const_max_t, at least not as the "primary" interface - forcing the user to pass in a type, or equivalently passing in cast expressions to a const_max(), can hide errors, e.g. if the -1 is really SOME_MACRO or some complicated expression that is usually positive, but that expression always gets cast to size_t because the user was forced to do const_max_t(size_t, SOME_MACRO, sizeof(foo)) to make the code compile. Not to mention that it's both easier to read and write if one could just do const_max(SOME_MACRO, sizeof(foo)) Can we instead do one of the following: (1) Effectively do the comparison in an infinitely wide signed integer, i.e. implement x < 0 && y >= 0 --> y x >= 0 && y < 0 --> x otherwise, if both have the same sign (but not necessarily the same signedness of their types), the type promotions do not alter either's value, so __builtin_choose_expr(x > y, x, y) will do the right thing with the resulting thing having the same type as the chosen one of x and y. [Or having type typeof(x+y), which would just be a cast in the macro.] This would allow const_max(-1, sizeof(foo)) and give sizeof(foo), but perhaps that's too magic. (2) Allow mixed types, but ensure the build fails if one of the values is not representable in typeof(x+y) (i.e., one value is negative but the common type is unsigned). That allows the const_max(SOME_MACRO, sizeof()), but prevents silent failure in case some weird combination of CONFIG options make SOME_MACRO evaluate to something negative. The user can always pass in (size_t)-1 explicitly if needed, or cast the sizeof() to int if that's what makes sense, but that's a case-by-case thing. I'd really like that the simple case const_max(16, sizeof(foo)) Just Works. Then if a lot users turn up that do need some casting, const_max_t can be implemented as a trivial const_max wrapper. Rasmus (1) something like __builtin_choose_expr((x >= 0 && y < 0) || \ (x >= 0 && y >= 0 && x > y) || \ (x < 0 && y < 0 && x > y), x, y) (2) something like // 1 or build error #define __check_promotion(t, x) ( 1/(((t)(x) < 0) == ((x) < 0)) ) __builtin_choose_expr(__check_promotion(typeof((x)+(y)), x) && \ __check_promotion(typeof((x)+(y)), y) && \ (x) > (y), x, y) Not sure how to get a more sensible error message, I'd like this to also work outside functions.
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..012f588b5a25 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -820,6 +820,25 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } x, y) /** + * const_max - return maximum of two positive compile-time constant values + * @x: first compile-time constant value + * @y: second compile-time constant value + * + * This has no type checking nor multi-evaluation defenses, and must + * only ever be used with positive compile-time constant values (for + * example when calculating a stack array size). + */ +size_t __error_not_const_arg(void) \ +__compiletime_error("const_max() used with non-compile-time constant arg"); +#define const_max(x, y) \ + __builtin_choose_expr(__builtin_constant_p(x) && \ + __builtin_constant_p(y), \ + (size_t)(x) > (size_t)(y) ? \ + (size_t)(x) : \ + (size_t)(y), \ + __error_not_const_arg()) + +/** * min3 - return minimum of three values * @x: first value * @y: second value
In the effort to remove all VLAs from the kernel[1], it is desirable to build with -Wvla. However, this warning is overly pessimistic, in that it is only happy with stack array sizes that are declared as constant expressions, and not constant values. One case of this is the evaluation of the max() macro which, due to its construction, ends up converting constant expression arguments into a constant value result. Attempts to adjust the behavior of max() ran afoul of version-dependent compiler behavior[2]. To work around this and still gain -Wvla coverage, this patch introduces a new macro, const_max(), for use in these cases of stack array size declaration, where the constant expressions are retained. Since this means losing the double-evaluation protections of the max() macro, this macro is designed to explicitly fail if used on non-constant arguments. Older compilers will fail with the unhelpful message: error: first argument to ‘__builtin_choose_expr’ not a constant Newer compilers will fail with a hopefully more helpful message: error: call to ‘__error_not_const_arg’ declared with attribute error: const_max() used with non-compile-time constant arg To gain the ability to compare differing types, the arguments are explicitly cast to size_t. Without this, some compiler versions will fail when comparing different enum types or similar constant expression cases. With the casting, it's possible to do things like: int foo[const_max(6, sizeof(something))]; [1] https://lkml.org/lkml/2018/3/7/621 [2] https://lkml.org/lkml/2018/3/10/170 Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/kernel.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)