From: Kees Cook <keescook@chromium.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org>, Josh Poimboeuf <jpoimboe@redhat.com>, Rasmus Villemoes <linux@rasmusvillemoes.dk>, "Gustavo A. R. Silva" <gustavo@embeddedor.com>, "Tobin C. Harding" <me@tobin.cc>, Steven Rostedt <rostedt@goodmis.org>, Jonathan Corbet <corbet@lwn.net>, Chris Mason <clm@fb.com>, Josef Bacik <jbacik@fb.com>, David Sterba <dsterba@suse.com>, "David S. Miller" <davem@davemloft.net>, Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Masahiro Yamada <yamada.masahiro@socionext.com>, Borislav Petkov <bp@suse.de>, Randy Dunlap <rdunlap@infradead.org>, Ian Abbott <abbotti@mev.co.uk>, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>, Petr Mladek <pmladek@suse.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Pantelis Antoniou <pantelis.antoniou@konsulko.com>, linux-btrfs <linux-btrfs@vger.kernel.org>, Network Development <netdev@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Kernel Hardening <kernel-hardening@lists.openwall.com> Subject: Re: [PATCH] kernel.h: Skip single-eval logic on literals in min()/max() Date: Thu, 8 Mar 2018 16:45:03 -0800 [thread overview] Message-ID: <CAGXu5j+JiYcsdaC4V9+SmUM2SKW1yRHr18d74G5SM_sSrQm=Nw@mail.gmail.com> (raw) In-Reply-To: <CA+55aFyf-ajRzW2sUwFrrMOAu5UyuOk5L8g9xfkr4VuJeWjV9w@mail.gmail.com> On Thu, Mar 8, 2018 at 3:48 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 8, 2018 at 1:40 PM, Kees Cook <keescook@chromium.org> wrote: >> +#define __min(t1, t2, 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), \ > > I understand why you use __builtin_types_compatible_p(), but please don't. > > It will mean that trivial constants like "5" and "sizeof(x)" won't > simplify, because they have different types. Rasmus mentioned this too. What I said there was that I was shy to make that change, since we already can't mix that kind of thing with the existing min()/max() implementation. The existing min()/max() is already extremely strict, so there are no instances of this in the tree. If I explicitly add one, I see this with or without the patch: In file included from drivers/misc/lkdtm.h:7:0, from drivers/misc/lkdtm_core.c:33: drivers/misc/lkdtm_core.c: In function ‘lkdtm_module_exit’: ./include/linux/kernel.h:809:16: warning: comparison of distinct pointer types lacks a cast (void) (&max1 == &max2); \ ^ ./include/linux/kernel.h:818:2: note: in expansion of macro ‘__max’ __max(typeof(x), typeof(y), \ ^~~~~ ./include/linux/printk.h:308:34: note: in expansion of macro ‘max’ printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ drivers/misc/lkdtm_core.c:500:2: note: in expansion of macro ‘pr_info’ pr_info("%lu\n", max(16, sizeof(unsigned long))); ^~~~~~~ > The ?: will give the right combined type anyway, and if you want the > type comparison warning, just add a comma-expression with something > like like > > (t1 *)1 == (t2 *)1 > > to get the type compatibility warning. When I tried removing __builtin_types_compatible_p(), I still got the type-check warning because I think the preprocessor still sees the "(void) (&min1 == &min2)" before optimizing? So, I technically _can_ drop the __builtin_types_compatible_p(), and still keep the type warning. :P > Yeah, yeah, maybe none of the VLA cases triggered that, but it seems > silly to not just get that obvious constant case right. > > Hmm? So are you saying you _want_ the type enforcement weakened here, or that I should just not use __builtin_types_compatible_p()? Thanks! -Kees -- Kees Cook Pixel Security
WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org>, Josh Poimboeuf <jpoimboe@redhat.com>, Rasmus Villemoes <linux@rasmusvillemoes.dk>, "Gustavo A. R. Silva" <gustavo@embeddedor.com>, "Tobin C. Harding" <me@tobin.cc>, Steven Rostedt <rostedt@goodmis.org>, Jonathan Corbet <corbet@lwn.net>, Chris Mason <clm@fb.com>, Josef Bacik <jbacik@fb.com>, David Sterba <dsterba@suse.com>, "David S. Miller" <davem@davemloft.net>, Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Masahiro Yamada <yamada.masahiro@socionext.com>, Borislav Petkov <bp@suse.de>, Randy Dunlap <rdunlap@infradead.org>, Ian Abbott <abbotti@mev.co.uk>, Subject: Re: [PATCH] kernel.h: Skip single-eval logic on literals in min()/max() Date: Thu, 8 Mar 2018 16:45:03 -0800 [thread overview] Message-ID: <CAGXu5j+JiYcsdaC4V9+SmUM2SKW1yRHr18d74G5SM_sSrQm=Nw@mail.gmail.com> (raw) In-Reply-To: <CA+55aFyf-ajRzW2sUwFrrMOAu5UyuOk5L8g9xfkr4VuJeWjV9w@mail.gmail.com> On Thu, Mar 8, 2018 at 3:48 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Mar 8, 2018 at 1:40 PM, Kees Cook <keescook@chromium.org> wrote: >> +#define __min(t1, t2, 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), \ > > I understand why you use __builtin_types_compatible_p(), but please don't. > > It will mean that trivial constants like "5" and "sizeof(x)" won't > simplify, because they have different types. Rasmus mentioned this too. What I said there was that I was shy to make that change, since we already can't mix that kind of thing with the existing min()/max() implementation. The existing min()/max() is already extremely strict, so there are no instances of this in the tree. If I explicitly add one, I see this with or without the patch: In file included from drivers/misc/lkdtm.h:7:0, from drivers/misc/lkdtm_core.c:33: drivers/misc/lkdtm_core.c: In function ‘lkdtm_module_exit’: ./include/linux/kernel.h:809:16: warning: comparison of distinct pointer types lacks a cast (void) (&max1 == &max2); \ ^ ./include/linux/kernel.h:818:2: note: in expansion of macro ‘__max’ __max(typeof(x), typeof(y), \ ^~~~~ ./include/linux/printk.h:308:34: note: in expansion of macro ‘max’ printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ drivers/misc/lkdtm_core.c:500:2: note: in expansion of macro ‘pr_info’ pr_info("%lu\n", max(16, sizeof(unsigned long))); ^~~~~~~ > The ?: will give the right combined type anyway, and if you want the > type comparison warning, just add a comma-expression with something > like like > > (t1 *)1 == (t2 *)1 > > to get the type compatibility warning. When I tried removing __builtin_types_compatible_p(), I still got the type-check warning because I think the preprocessor still sees the "(void) (&min1 == &min2)" before optimizing? So, I technically _can_ drop the __builtin_types_compatible_p(), and still keep the type warning. :P > Yeah, yeah, maybe none of the VLA cases triggered that, but it seems > silly to not just get that obvious constant case right. > > Hmm? So are you saying you _want_ the type enforcement weakened here, or that I should just not use __builtin_types_compatible_p()? Thanks! -Kees -- Kees Cook Pixel Security
next prev parent reply other threads:[~2018-03-09 0:45 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-08 21:40 [PATCH] kernel.h: Skip single-eval logic on literals in min()/max() Kees Cook 2018-03-08 21:40 ` Kees Cook 2018-03-08 21:59 ` Ian Campbell 2018-03-08 21:59 ` Ian Campbell 2018-03-08 21:59 ` Ian Campbell 2018-03-08 22:18 ` Andrew Morton 2018-03-08 22:18 ` Andrew Morton 2018-03-08 22:49 ` Kees Cook 2018-03-08 22:49 ` Kees Cook 2018-03-08 23:48 ` Linus Torvalds 2018-03-08 23:48 ` Linus Torvalds 2018-03-09 0:45 ` Kees Cook [this message] 2018-03-09 0:45 ` Kees Cook 2018-03-09 1:35 ` Linus Torvalds 2018-03-09 1:35 ` Linus Torvalds 2018-03-09 1:46 ` Kees Cook 2018-03-09 1:46 ` Kees Cook
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAGXu5j+JiYcsdaC4V9+SmUM2SKW1yRHr18d74G5SM_sSrQm=Nw@mail.gmail.com' \ --to=keescook@chromium.org \ --cc=abbotti@mev.co.uk \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bp@suse.de \ --cc=clm@fb.com \ --cc=corbet@lwn.net \ --cc=davem@davemloft.net \ --cc=dsterba@suse.com \ --cc=gustavo@embeddedor.com \ --cc=jbacik@fb.com \ --cc=jpoimboe@redhat.com \ --cc=kernel-hardening@lists.openwall.com \ --cc=kuznet@ms2.inr.ac.ru \ --cc=linux-btrfs@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ --cc=me@tobin.cc \ --cc=mingo@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=pantelis.antoniou@konsulko.com \ --cc=peterz@infradead.org \ --cc=pmladek@suse.com \ --cc=rdunlap@infradead.org \ --cc=rostedt@goodmis.org \ --cc=sergey.senozhatsky.work@gmail.com \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ --cc=yamada.masahiro@socionext.com \ --cc=yoshfuji@linux-ipv6.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.