| Message ID | 1520479847-39174-2-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-03-08 04:30, Kees Cook wrote: > In the quest to remove all stack VLAs from the kernel[1], this introduces > a new "simple max" macro, and changes the "sym" array size calculation to > use it. The value is actually a fixed size, but since the max() macro uses > some extensive tricks for safety, it ends up looking like a variable size > to the compiler. > > [1] https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/kernel.h | 11 +++++++++++ > lib/vsprintf.c | 4 ++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 3fd291503576..1da554e9997f 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > x, y) > > /** > + * SIMPLE_MAX - return maximum of two values without any type checking > + * @x: first value > + * @y: second value > + * > + * This should only be used in stack array sizes, since the type-checking > + * from max() confuses the compiler into thinking a VLA is being used. > + */ > +#define SIMPLE_MAX(x, y) ((size_t)(x) > (size_t)(y) ? (size_t)(x) \ > + : (size_t)(y)) This will be abused at some point, leading to the usual double evaluation etc. etc. problems. The name is also too long (and in general we should avoid adjectives like "simple", "safe", people reading the code won't know what is simple or safe about it). I think this should work #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y)) That forces (x)>(y) to be a compile-time constant, so x and y must also be; hence there can be no side effects. The MIN version of this could replace the custom __const_min in fs/file.c, and probably other places as well. I tested that this at least works in the vsprintf case, -Wvla no longer complains. fs/file.c also compiles with the MIN version of this. I suppose MIN and MAX will collide with other uses in the tree. Hmm. Rasmus
On Thu, 8 Mar 2018, Rasmus Villemoes wrote: > On 2018-03-08 04:30, Kees Cook wrote: > > /** > > + * SIMPLE_MAX - return maximum of two values without any type checking > > + * @x: first value > > + * @y: second value > > + * > > + * This should only be used in stack array sizes, since the type-checking > > + * from max() confuses the compiler into thinking a VLA is being used. > > + */ > > +#define SIMPLE_MAX(x, y) ((size_t)(x) > (size_t)(y) ? (size_t)(x) \ > > + : (size_t)(y)) > > This will be abused at some point, leading to the usual double > evaluation etc. etc. problems. The name is also too long (and in general > we should avoid adjectives like "simple", "safe", people reading the > code won't know what is simple or safe about it). I think this should work > > #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y)) > > That forces (x)>(y) to be a compile-time constant, so x and y must also > be; hence there can be no side effects. The MIN version of this could > replace the custom __const_min in fs/file.c, and probably other places > as well. > > I tested that this at least works in the vsprintf case, -Wvla no longer > complains. fs/file.c also compiles with the MIN version of this. > > I suppose MIN and MAX will collide with other uses in the tree. Hmm. Make it CONST_MAX() or something like that which makes it entirely clear. Thanks, tglx
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..1da554e9997f 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } x, y) /** + * SIMPLE_MAX - return maximum of two values without any type checking + * @x: first value + * @y: second value + * + * This should only be used in stack array sizes, since the type-checking + * from max() confuses the compiler into thinking a VLA is being used. + */ +#define SIMPLE_MAX(x, y) ((size_t)(x) > (size_t)(y) ? (size_t)(x) \ + : (size_t)(y)) + +/** * min3 - return minimum of three values * @x: first value * @y: second value diff --git a/lib/vsprintf.c b/lib/vsprintf.c index d7a708f82559..50cce36e1cdc 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -744,8 +744,8 @@ char *resource_string(char *buf, char *end, struct resource *res, #define FLAG_BUF_SIZE (2 * sizeof(res->flags)) #define DECODED_BUF_SIZE sizeof("[mem - 64bit pref window disabled]") #define RAW_BUF_SIZE sizeof("[mem - flags 0x]") - char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE, - 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)]; + char sym[SIMPLE_MAX(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE, + 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)]; char *p = sym, *pend = sym + sizeof(sym); int decode = (fmt[0] == 'R') ? 1 : 0;
In the quest to remove all stack VLAs from the kernel[1], this introduces a new "simple max" macro, and changes the "sym" array size calculation to use it. The value is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size to the compiler. [1] https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/kernel.h | 11 +++++++++++ lib/vsprintf.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-)