| Message ID | 20180308150236.5tysfbm3xdouii5n@treble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: >> This series adds SIMPLE_MAX() to be used in places where a stack array >> is actually fixed, but the compiler still warns about VLA usage due to >> confusion caused by the safety checks in the max() macro. >> >> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), >> and they should all have no operational differences. > > What if we instead simplify the max() macro's type checking so that GCC > can more easily fold the array size constants? The below patch seems to > work: Oooooh magic! Very nice. I couldn't figure out how to do this when I stared at it. Yes, let me respin. (I assume I can add your S-o-b?) -Kees > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 3fd291503576..ec863726da29 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap) > static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > #endif /* CONFIG_TRACING */ > > -/* > - * min()/max()/clamp() macros that also do > - * strict type-checking.. See the > - * "unnecessary" pointer comparison. > - */ > -#define __min(t1, t2, min1, min2, x, y) ({ \ > - t1 min1 = (x); \ > - t2 min2 = (y); \ > - (void) (&min1 == &min2); \ > - min1 < min2 ? min1 : min2; }) > +extern long __error_incompatible_types_in_min_macro; > +extern long __error_incompatible_types_in_max_macro; > + > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > + (t1)__error_incompatible_types_in_min_macro) > > /** > * min - return minimum of two values of the same or compatible types > * @x: first value > * @y: second value > */ > -#define min(x, y) \ > - __min(typeof(x), typeof(y), \ > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > - x, y) > +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ > > -#define __max(t1, t2, max1, max2, x, y) ({ \ > - t1 max1 = (x); \ > - t2 max2 = (y); \ > - (void) (&max1 == &max2); \ > - max1 > max2 ? max1 : max2; }) > +#define __max(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > + (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ > + (t1)__error_incompatible_types_in_max_macro) > > /** > * max - return maximum of two values of the same or compatible types > * @x: first value > * @y: second value > */ > -#define max(x, y) \ > - __max(typeof(x), typeof(y), \ > - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ > - x, y) > +#define max(x, y) __max(typeof(x), typeof(y), x, y) > > /** > * min3 - return minimum of three values > @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > * @x: first value > * @y: second value > */ > -#define min_t(type, x, y) \ > - __min(type, type, \ > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > - x, y) > +#define min_t(type, x, y) __min(type, type, x, y) > > /** > * max_t - return maximum of two values, using the specified type > @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > * @x: first value > * @y: second value > */ > -#define max_t(type, x, y) \ > - __max(type, type, \ > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > - x, y) > +#define max_t(type, x, y) __max(type, type, x, y) \ > > /** > * clamp_t - return a value clamped to a given range using a given type
On Thu, 8 Mar 2018 09:02:36 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: > > This series adds SIMPLE_MAX() to be used in places where a stack array > > is actually fixed, but the compiler still warns about VLA usage due to > > confusion caused by the safety checks in the max() macro. > > > > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), > > and they should all have no operational differences. > > What if we instead simplify the max() macro's type checking so that GCC > can more easily fold the array size constants? The below patch seems to > work: Nice. Have you tried to do a allmodconfig and build on various archs? Of course pushing it to kernel.org will have the zero day bot do it for you ;-) -- Steve
On Thu, Mar 08, 2018 at 10:02:01AM -0800, Kees Cook wrote: > On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: > >> This series adds SIMPLE_MAX() to be used in places where a stack array > >> is actually fixed, but the compiler still warns about VLA usage due to > >> confusion caused by the safety checks in the max() macro. > >> > >> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), > >> and they should all have no operational differences. > > > > What if we instead simplify the max() macro's type checking so that GCC > > can more easily fold the array size constants? The below patch seems to > > work: > > Oooooh magic! Very nice. I couldn't figure out how to do this when I > stared at it. Yes, let me respin. (I assume I can add your S-o-b?) I'm going to be traveling for a few days, so I bequeath the patch to you. You can add my SOB. I agree with Steve's suggestion to run it through 0-day. > > -Kees > > > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > > index 3fd291503576..ec863726da29 100644 > > --- a/include/linux/kernel.h > > +++ b/include/linux/kernel.h > > @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap) > > static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > > #endif /* CONFIG_TRACING */ > > > > -/* > > - * min()/max()/clamp() macros that also do > > - * strict type-checking.. See the > > - * "unnecessary" pointer comparison. > > - */ > > -#define __min(t1, t2, min1, min2, x, y) ({ \ > > - t1 min1 = (x); \ > > - t2 min2 = (y); \ > > - (void) (&min1 == &min2); \ > > - min1 < min2 ? min1 : min2; }) > > +extern long __error_incompatible_types_in_min_macro; > > +extern long __error_incompatible_types_in_max_macro; > > + > > +#define __min(t1, t2, x, y) \ > > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > > + (t1)__error_incompatible_types_in_min_macro) > > > > /** > > * min - return minimum of two values of the same or compatible types > > * @x: first value > > * @y: second value > > */ > > -#define min(x, y) \ > > - __min(typeof(x), typeof(y), \ > > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > > - x, y) > > +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ > > > > -#define __max(t1, t2, max1, max2, x, y) ({ \ > > - t1 max1 = (x); \ > > - t2 max2 = (y); \ > > - (void) (&max1 == &max2); \ > > - max1 > max2 ? max1 : max2; }) > > +#define __max(t1, t2, x, y) \ > > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > > + (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ > > + (t1)__error_incompatible_types_in_max_macro) > > > > /** > > * max - return maximum of two values of the same or compatible types > > * @x: first value > > * @y: second value > > */ > > -#define max(x, y) \ > > - __max(typeof(x), typeof(y), \ > > - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ > > - x, y) > > +#define max(x, y) __max(typeof(x), typeof(y), x, y) > > > > /** > > * min3 - return minimum of three values > > @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > > * @x: first value > > * @y: second value > > */ > > -#define min_t(type, x, y) \ > > - __min(type, type, \ > > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > > - x, y) > > +#define min_t(type, x, y) __min(type, type, x, y) > > > > /** > > * max_t - return maximum of two values, using the specified type > > @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > > * @x: first value > > * @y: second value > > */ > > -#define max_t(type, x, y) \ > > - __max(type, type, \ > > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > > - x, y) > > +#define max_t(type, x, y) __max(type, type, x, y) \ > > > > /** > > * clamp_t - return a value clamped to a given range using a given type > > > > -- > Kees Cook > Pixel Security
On 2018-03-08 16:02, Josh Poimboeuf wrote: > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: >> This series adds SIMPLE_MAX() to be used in places where a stack array >> is actually fixed, but the compiler still warns about VLA usage due to >> confusion caused by the safety checks in the max() macro. >> >> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), >> and they should all have no operational differences. > > What if we instead simplify the max() macro's type checking so that GCC > can more easily fold the array size constants? The below patch seems to > work: > > +extern long __error_incompatible_types_in_min_macro; > +extern long __error_incompatible_types_in_max_macro; > + > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > + (t1)__error_incompatible_types_in_min_macro) > > /** > * min - return minimum of two values of the same or compatible types > * @x: first value > * @y: second value > */ > -#define min(x, y) \ > - __min(typeof(x), typeof(y), \ > - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ > - x, y) > +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ > But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice problem. Maybe we don't care? But until we get a __builtin_assert_this_has_no_side_effects() I think that's a little dangerous. Rasmus
On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 2018-03-08 16:02, Josh Poimboeuf wrote: >> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: >>> This series adds SIMPLE_MAX() to be used in places where a stack array >>> is actually fixed, but the compiler still warns about VLA usage due to >>> confusion caused by the safety checks in the max() macro. >>> >>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), >>> and they should all have no operational differences. >> >> What if we instead simplify the max() macro's type checking so that GCC >> can more easily fold the array size constants? The below patch seems to >> work: >> > >> +extern long __error_incompatible_types_in_min_macro; >> +extern long __error_incompatible_types_in_max_macro; >> + >> +#define __min(t1, t2, x, y) \ >> + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ >> + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ >> + (t1)__error_incompatible_types_in_min_macro) >> >> /** >> * min - return minimum of two values of the same or compatible types >> * @x: first value >> * @y: second value >> */ >> -#define min(x, y) \ >> - __min(typeof(x), typeof(y), \ >> - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ >> - x, y) >> +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ >> > > But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice > problem. Maybe we don't care? But until we get a > __builtin_assert_this_has_no_side_effects() I think that's a little > dangerous. Eek, yes, we can't do the double-eval. The proposed change breaks things badly. :) a: 20 b: 40 max(a++, b++): 40 a: 21 b: 41 a: 20 b: 40 new_max(a++, b++): 41 a: 21 b: 42 However, this works for me: #define __new_max(t1, t2, max1, max2, x, y) \ __builtin_choose_expr(__builtin_constant_p(x) && \ __builtin_constant_p(y) && \ __builtin_types_compatible_p(t1, t2), \ (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ __max(t1, t2, max1, max2, x, y)) #define new_max(x, y) \ __new_max(typeof(x), typeof(y), \ __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ x, y) (pardon the whitespace damage...) Let me spin a sane patch and test it... -Kees
On Thu, 8 Mar 2018 09:02:36 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: > > This series adds SIMPLE_MAX() to be used in places where a stack array > > is actually fixed, but the compiler still warns about VLA usage due to > > confusion caused by the safety checks in the max() macro. > > > > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(), > > and they should all have no operational differences. > > What if we instead simplify the max() macro's type checking so that GCC > can more easily fold the array size constants? The below patch seems to > work: > > -/* > - * min()/max()/clamp() macros that also do > - * strict type-checking.. See the > - * "unnecessary" pointer comparison. > - */ > -#define __min(t1, t2, min1, min2, x, y) ({ \ > - t1 min1 = (x); \ > - t2 min2 = (y); \ > - (void) (&min1 == &min2); \ > - min1 < min2 ? min1 : min2; }) > +extern long __error_incompatible_types_in_min_macro; > +extern long __error_incompatible_types_in_max_macro; > + > +#define __min(t1, t2, x, y) \ > + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ > + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > + (t1)__error_incompatible_types_in_min_macro) This will move the error detection from compile-time to link-time. That's tolerable I guess, but a bit sad and should be flagged in the changelog at least.
On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote: > On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> On 2018-03-08 16:02, Josh Poimboeuf wrote: >>> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote: >>> +extern long __error_incompatible_types_in_min_macro; >>> +extern long __error_incompatible_types_in_max_macro; >>> + >>> +#define __min(t1, t2, x, y) \ >>> + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ >>> + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ >>> + (t1)__error_incompatible_types_in_min_macro) >>> >>> /** >>> * min - return minimum of two values of the same or compatible types >>> * @x: first value >>> * @y: second value >>> */ >>> -#define min(x, y) \ >>> - __min(typeof(x), typeof(y), \ >>> - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ >>> - x, y) >>> +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ >>> >> >> But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice >> problem. Maybe we don't care? But until we get a >> __builtin_assert_this_has_no_side_effects() I think that's a little >> dangerous. > > Eek, yes, we can't do the double-eval. The proposed change breaks > things badly. :) > > a: 20 > b: 40 > max(a++, b++): 40 > a: 21 > b: 41 > > a: 20 > b: 40 > new_max(a++, b++): 41 > a: 21 > b: 42 > > However, this works for me: > > #define __new_max(t1, t2, max1, max2, x, y) \ > __builtin_choose_expr(__builtin_constant_p(x) && \ > __builtin_constant_p(y) && \ > __builtin_types_compatible_p(t1, t2), \ > (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ > __max(t1, t2, max1, max2, x, y)) > > #define new_max(x, y) \ > __new_max(typeof(x), typeof(y), \ > __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ > x, y) Yes, that would seem to do the trick. Thinking out loud: do we really want or need the __builtin_types_compatible condition when x and y are compile-time constants? I think it would be nice to be able to use max(16, sizeof(bla)) without having to cast either the literal or the sizeof. Just omitting the type compatibility check might be dangerous, but perhaps it could be relaxed to a check that both values are representable in their common promoted type. Something like (type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0) should be safe (if the types have same signedness, or the value of signed type is positive), though it doesn't allow a few corner cases (e.g. short vs. unsigned char is always ok due to promotion to int, and also if the signed type is strictly wider than the unsigned type). Rasmus
On Thu, Mar 8, 2018 at 2:12 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote: >> However, this works for me: >> >> #define __new_max(t1, t2, max1, max2, x, y) \ >> __builtin_choose_expr(__builtin_constant_p(x) && \ >> __builtin_constant_p(y) && \ >> __builtin_types_compatible_p(t1, t2), \ >> (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ >> __max(t1, t2, max1, max2, x, y)) >> >> #define new_max(x, y) \ >> __new_max(typeof(x), typeof(y), \ >> __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ >> x, y) > > Yes, that would seem to do the trick. > > Thinking out loud: do we really want or need the > __builtin_types_compatible condition when x and y are compile-time > constants? I think it would be nice to be able to use max(16, > sizeof(bla)) without having to cast either the literal or the sizeof. > Just omitting the type compatibility check might be dangerous, but > perhaps it could be relaxed to a check that both values are > representable in their common promoted type. Something like > > (type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0) > > should be safe (if the types have same signedness, or the value of > signed type is positive), though it doesn't allow a few corner cases > (e.g. short vs. unsigned char is always ok due to promotion to int, > and also if the signed type is strictly wider than the unsigned type). I agree, it would be nice. However, I think it'd be better to continue to depend on max_t() for these kinds of cases though. For example: char foo[max_t(size_t, 6, sizeof(something))]; Works with the proposed patch. Also, I think this mismatch would already be triggering warnings, so we shouldn't have any currently. -Kees
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..ec863726da29 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap) static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } #endif /* CONFIG_TRACING */ -/* - * min()/max()/clamp() macros that also do - * strict type-checking.. See the - * "unnecessary" pointer comparison. - */ -#define __min(t1, t2, min1, min2, x, y) ({ \ - t1 min1 = (x); \ - t2 min2 = (y); \ - (void) (&min1 == &min2); \ - min1 < min2 ? min1 : min2; }) +extern long __error_incompatible_types_in_min_macro; +extern long __error_incompatible_types_in_max_macro; + +#define __min(t1, t2, x, y) \ + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ + (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ + (t1)__error_incompatible_types_in_min_macro) /** * min - return minimum of two values of the same or compatible types * @x: first value * @y: second value */ -#define min(x, y) \ - __min(typeof(x), typeof(y), \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define min(x, y) __min(typeof(x), typeof(y), x, y) \ -#define __max(t1, t2, max1, max2, x, y) ({ \ - t1 max1 = (x); \ - t2 max2 = (y); \ - (void) (&max1 == &max2); \ - max1 > max2 ? max1 : max2; }) +#define __max(t1, t2, x, y) \ + __builtin_choose_expr(__builtin_types_compatible_p(t1, t2), \ + (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y), \ + (t1)__error_incompatible_types_in_max_macro) /** * max - return maximum of two values of the same or compatible types * @x: first value * @y: second value */ -#define max(x, y) \ - __max(typeof(x), typeof(y), \ - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ - x, y) +#define max(x, y) __max(typeof(x), typeof(y), x, y) /** * min3 - return minimum of three values @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * @x: first value * @y: second value */ -#define min_t(type, x, y) \ - __min(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define min_t(type, x, y) __min(type, type, x, y) /** * max_t - return maximum of two values, using the specified type @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * @x: first value * @y: second value */ -#define max_t(type, x, y) \ - __max(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define max_t(type, x, y) __max(type, type, x, y) \ /** * clamp_t - return a value clamped to a given range using a given type