linux-kernel - Re: ipc/msg: zalloc struct msg

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: <CACT4Y+anMTUCaRH5D4QoZR4bLdtH=_wtCxK2dM6isJozbVThDg@mail.gmail.com>
Date:   Mon, 25 Jun 2018 11:21:47 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Davidlohr Bueso <dave@...olabs.net>
Cc:     syzbot <syzbot+2827ef6b3385deb07eaf@...kaller.appspotmail.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>, linux@...inikbrodowski.net,
        manfred <manfred@...orfullife.com>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: ipc/msg: zalloc struct msg_queue when creating a new msq

On Sun, Jun 24, 2018 at 4:56 AM, Davidlohr Bueso <dave@...olabs.net> wrote:
> The following splat was reported around the msg_queue structure
> which can have uninitialized fields left over after newque().
> Future syscalls which make use of the msq id (now valid) can thus
> make KMSAN complain because not all fields are explicitly initialized
> and we have the padding as well. This is internal to the kernel,
> hence no bogus leaks.

Hi Davidlohr,

As far as I understand the root problem is that (1) we publish a
not-fully initialized objects and (2) finish it's initialization in a
racy manner when other threads already have access to it. As the
result other threads can act on a wrong object. I am not sure that
zeroing the object really solves these problems. It will sure get rid
of the report at hand (but probably not of KTSAN, data race detector,
report), other threads still can see wrong 0 id and the id is still
initialized in racy way. I would expect that a proper fix would be to
publish a fully initialized object with proper, final id. Am I missing
something?



> ==================================================================
> BUG: KMSAN: uninit-value in do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
> CPU: 0 PID: 4528 Comm: syz-executor852 Not tainted 4.17.0-rc5+ #103
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x185/0x1d0 lib/dump_stack.c:113
>  kmsan_report+0x149/0x260 mm/kmsan/kmsan.c:1084
>  __msan_warning_32+0x6e/0xc0 mm/kmsan/kmsan_instr.c:686
>  do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
>  ksys_msgrcv ipc/msg.c:1184 [inline]
>  __do_sys_msgrcv ipc/msg.c:1190 [inline]
>  __se_sys_msgrcv ipc/msg.c:1187 [inline]
>  __x64_sys_msgrcv+0x160/0x1b0 ipc/msg.c:1187
>  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x4459b9
> RSP: 002b:00007f0d57662db8 EFLAGS: 00000297 ORIG_RAX: 0000000000000046
> RAX: ffffffffffffffda RBX: 00000000006dac54 RCX: 00000000004459b9
> RDX: 00000000000000d0 RSI: 0000000020000000 RDI: 0000000000260007
> RBP: 00000000006dac50 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000297 R12: 0000000000000000
> R13: 00007ffd5ab7e25f R14: 00007f0d576639c0 R15: 0000000000000006
>
> Uninit was created at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:279 [inline]
>  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:189
>  kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:315
>  __kmalloc_node+0xe25/0x11f0 mm/slub.c:3865
>  kmalloc_node include/linux/slab.h:554 [inline]
>  kvmalloc_node+0x197/0x2f0 mm/util.c:421
>  kvmalloc include/linux/mm.h:550 [inline]
>  newque+0xb4/0x7d0 ipc/msg.c:139
>  ipcget_new ipc/util.c:315 [inline]
>  ipcget+0x27b/0xd90 ipc/util.c:653
>  ksys_msgget ipc/msg.c:289 [inline]
>  __do_sys_msgget ipc/msg.c:294 [inline]
>  __se_sys_msgget ipc/msg.c:292 [inline]
>  __x64_sys_msgget+0x14c/0x1d0 ipc/msg.c:292
>  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ==================================================================
>
> Fix this by simply zero init the whole structure, something that
> sysvsems also do; this is safe as it's a nop, having no secondary
> effect afaict.
>
> Reported-by: syzbot <syzbot+2827ef6b3385deb07eaf@...kaller.appspotmail.com>
> Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
> ---
> ipc/msg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 62545ce19173..da81b374f9fd 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -136,7 +136,7 @@ static int newque(struct ipc_namespace *ns, struct
> ipc_params *params)
>         key_t key = params->key;
>         int msgflg = params->flg;
>
> -       msq = kvmalloc(sizeof(*msq), GFP_KERNEL);
> +       msq = kvzalloc(sizeof(*msq), GFP_KERNEL);
>         if (unlikely(!msq))
>                 return -ENOMEM;
>
> --
> 2.16.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller-bugs+unsubscribe@...glegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/20180624025651.bvjlcfulbmycz5bf%40linux-r8p5.
> For more options, visit https://groups.google.com/d/optout.