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]

Date:   Wed, 4 Jul 2018 11:18:44 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>,
        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,
        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

Hello together,

On 06/25/2018 11:21 AM, Dmitry Vyukov wrote:
> 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?
There are 2 relevant values: kern_ipc_perm.id and kern_ipc_perm.seq.

For kern_ipc_perm.id, it is possible to move the access to the codepath 
that hold the lock.

For kern_ipc_perm.seq, there are two options:
1) set it before publication.
2) initialize to an invalid value, and correct that at the end.

I'm in favor of option 2, it avoids that we must think about reducing 
the next sequence number or not:

The purpose of the sequence counter is to minimize the risk that e.g. a 
semop() will write into a newly created array.
I intentially write "minimize the risk", as it is by design impossible 
to guarantee that this cannot happen, e.g. if semop() sleeps at the 
instruction before the syscall.

Therefore, we can set seq to ULONG_MAX, then ipc_checkid() will always 
fail and the corruption is avoided.

What do you think?

And, obviously:
Just set seq to 0 is dangerous, as the first allocated sequence number 
is 0, and if that object is destroyed, then the newly created object has 
temporarily sequence number 0 as well.

--
     Manfred

View attachment "0001-ipc-initialize-kern_ipc_perm.id-earlier.patch" of type "text/x-patch" (6455 bytes)