linux-kernel - Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <xa1ttw4b2z10.fsf@mina86.com>
Date:   Tue, 23 May 2017 20:44:59 +0200
From:   Michal Nazarewicz <mina86@...a86.com>
To:     Ian Abbott <abbotti@....co.uk>, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org
Cc:     Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
        Borislav Petkov <bp@...e.de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Johannes Berg <johannes.berg@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexander Potapenko <glider@...gle.com>,
        Ian Abbott <abbotti@....co.uk>
Subject: Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

On Tue, May 23 2017, 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 <linux/kernel.h>
>  #include <linux/module.h>
>
> 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 <abbotti@....co.uk>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Michal Nazarewicz <mina86@...a86.com>

Acked-by: Michal Nazarewicz <mina86@...a86.com>

> Cc: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
> Cc: Johannes Berg <johannes.berg@...el.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Alexander Potapenko <glider@...gle.com>
> ---
> 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.
> ---
>  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..f5dba37aaa60 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -11,6 +11,7 @@
>  #include <linux/log2.h>
>  #include <linux/typecheck.h>
>  #include <linux/printk.h>
> +#include <linux/bug.h>
>  #include <asm/byteorder.h>
>  #include <uapi/linux/kernel.h>
>  
> @@ -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

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»