linux-kernel - Re: [PATCH v5 6/6] 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]

Date:   Fri, 26 May 2017 14:57:31 +0100
From:   Ian Abbott <abbotti@....co.uk>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Alexander Potapenko <glider@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...e.de>,
        Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Michal Nazarewicz <mina86@...a86.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v5 6/6] kernel.h: handle pointers to arrays better in
 container_of()

On 25/05/17 19:35, Kees Cook wrote:
> On Thu, May 25, 2017 at 5:03 AM, Ian Abbott <abbotti@....co.uk> 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>
>> 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>
>> Acked-by: Michal Nazarewicz <mina86@...a86.com>
>
> 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?

Yes for ARCH=i386 at least.

>
> Acked-by: Kees Cook <keescook@...omium.org>

Thanks!

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-