From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1036416AbdEYSfv (ORCPT ); Thu, 25 May 2017 14:35:51 -0400 Received: from mail-io0-f174.google.com ([209.85.223.174]:35965 "EHLO mail-io0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1036401AbdEYSfo (ORCPT ); Thu, 25 May 2017 14:35:44 -0400 MIME-Version: 1.0 In-Reply-To: <20170525120316.24473-7-abbotti@mev.co.uk> References: <20170525120316.24473-1-abbotti@mev.co.uk> <20170525120316.24473-7-abbotti@mev.co.uk> From: Kees Cook Date: Thu, 25 May 2017 11:35:42 -0700 X-Google-Sender-Auth: qMwp2qPU4yFUhbGeaH_MDLSQMek Message-ID: Subject: Re: [PATCH v5 6/6] kernel.h: handle pointers to arrays better in container_of() To: Ian Abbott Cc: LKML , linux-arch , Alexander Potapenko , Andrew Morton , Arnd Bergmann , Borislav Petkov , Hidehiro Kawai , Jakub Kicinski , Johannes Berg , Michal Nazarewicz , Peter Zijlstra , Rasmus Villemoes , Steven Rostedt Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v4PIa6lq022373 On Thu, May 25, 2017 at 5:03 AM, Ian Abbott wrote: > If the first parameter of container_of() is a pointer to a > non-const-qualified array type (and the third parameter names a > non-const-qualified array member), the local variable __mptr will be > defined with a const-qualified array type. In ISO C, these types are > incompatible. They work as expected in GNU C, but some versions will > issue warnings. For example, GCC 4.9 produces the warning > "initialization from incompatible pointer type". > > Here is an example of where the problem occurs: > > ------------------------------------------------------- > #include > #include > > MODULE_LICENSE("GPL"); > > struct st { > int a; > char b[16]; > }; > > static int __init example_init(void) { > struct st t = { .a = 101, .b = "hello" }; > char (*p)[16] = &t.b; > struct st *x = container_of(p, struct st, b); > printk(KERN_DEBUG "%p %p\n", (void *)&t, (void *)x); > return 0; > } > > static void __exit example_exit(void) { > } > > module_init(example_init); > module_exit(example_exit); > ------------------------------------------------------- > > Building the module with gcc-4.9 results in these warnings (where '{m}' > is the module source and '{k}' is the kernel source): > > ------------------------------------------------------- > In file included from {m}/example.c:1:0: > {m}/example.c: In function ‘example_init’: > {k}/include/linux/kernel.h:854:48: warning: initialization from > incompatible pointer type > const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > ^ > {m}/example.c:14:17: note: in expansion of macro ‘container_of’ > struct st *x = container_of(p, struct st, b); > ^ > {k}/include/linux/kernel.h:854:48: warning: (near initialization for > ‘x’) > const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > ^ > {m}/example.c:14:17: note: in expansion of macro ‘container_of’ > struct st *x = container_of(p, struct st, b); > ^ > ------------------------------------------------------- > > Replace the type checking performed by the macro to avoid these > warnings. Make sure `*(ptr)` either has type compatible with the > member, or has type compatible with `void`, ignoring qualifiers. Raise > compiler errors if this is not true. This is stronger than the previous > behaviour, which only resulted in compiler warnings for a type mismatch. > > Signed-off-by: Ian Abbott > Cc: Andrew Morton > Cc: Michal Nazarewicz > Cc: Hidehiro Kawai > Cc: Borislav Petkov > Cc: Rasmus Villemoes > Cc: Johannes Berg > Cc: Peter Zijlstra > Cc: Alexander Potapenko > Acked-by: Michal Nazarewicz Seems reasonable to me. I think this actually improves the errors reported when something is mismatched in container_of(). Silly question: does this pass an allyesconfig? Acked-by: Kees Cook -Kees > --- > v2: Rebased and altered description to provide an example of when the > compiler warnings occur. v1 (from 2016-10-10) also modified a > 'container_of_safe()' macro that never made it out of "linux-next". > v3: Added back some type checking at the suggestion of Michal > Nazarewicz with some helpful hints by Peter Zijlstra. > v4: No change. > v5: Added Acked-by for Michal Nazarewicz. Included > instead of to avoid a circular dependency that resulted in > build failures when was included before . > --- > include/linux/kernel.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 13bc08aba704..1c9c11c9f1a8 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -11,6 +11,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > * @member: the name of the member within the struct. > * > */ > -#define container_of(ptr, type, member) ({ \ > - const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > - (type *)( (char *)__mptr - offsetof(type,member) );}) > +#define container_of(ptr, type, member) ({ \ > + BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \ > + !__same_type(*(ptr), void), \ > + "pointer type mismatch in container_of()"); \ > + ((type *)((char *)(ptr) - offsetof(type, member))); }) > > /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */ > #ifdef CONFIG_FTRACE_MCOUNT_RECORD > -- > 2.11.0 > -- Kees Cook Pixel Security