Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <b9ffe11e-c63f-c5fb-f5e8-fc4eb8c4f0fa@colorfullife.com>
References: <000000000000e403b3056e76c786@google.com> <20180624025651.bvjlcfulbmycz5bf@linux-r8p5>
 <CACT4Y+anMTUCaRH5D4QoZR4bLdtH=_wtCxK2dM6isJozbVThDg@mail.gmail.com>
 <ff30f992-9c75-f230-c86a-902373f4ecaa@colorfullife.com> <CACT4Y+ZX0VSf23PTGpfwps4T0itMV-ZnSDQx6SuOucBE9sPSEw@mail.gmail.com>
 <b9ffe11e-c63f-c5fb-f5e8-fc4eb8c4f0fa@colorfullife.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Wed, 4 Jul 2018 16:22:12 +0200
Message-ID: <CACT4Y+a-yU9yWpZjY5XtRktdLedJHsyuJnQK+UZzf8Ow6U+Mog@mail.gmail.com>
Subject: Re: ipc/msg: zalloc struct msg_queue when creating a new msq
To:     Manfred Spraul <manfred@colorfullife.com>
Cc:     Davidlohr Bueso <dave@stgolabs.net>,
        syzbot <syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>, linux@dominikbrodowski.net,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Eric Dumazet <edumazet@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 4, 2018 at 4:08 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
> Hello Dmitry,
> On 07/04/2018 12:03 PM, Dmitry Vyukov wrote:
>>
>> On Wed, Jul 4, 2018 at 11:18 AM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>>
>>>
>>> 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.
>>
>> Hi Manfred,
>>
>> It still looks fishy to me. This code published uninitialized uid's
>> for years (which lead not only to accidentally accessing wrong
>> objects, but also to privilege escalation). Now it publishes uninit
>> id/seq. The first proposed fix still did not make it correct. I can't
>> say that I see a but in your patch, but initializing id/seq in a racy
>> manner rings bells for me. Say, if we write/read seq ahead of id, can
>> reader still get access to a wrong object?
>> It all suggests some design flaw to me. Could ipc_idr_alloc() do full
>> initialization, i.e. also do what ipc_buildid() does? This would
>> ensure that we publish a fully constructed object in the first place.
>> We already have cleanup for ipc_idr_alloc(), which is idr_remove(), so
>> if we care about seq space conservation even in error conditions
>> (ENOMEM?), idr_remove() could accept an additional flag saying "this
>> object should not have been used by sane users yet, so retake its
>> seq". Did I get your concern about seq properly?
>
> You have convinced me, I'll rewrite the patch:
>
> 1) kern_ipc_perm.seq should be accessible under rcu_read_lock(), this means
> replacing ipc_build_id() with two functions:
> One that initializes kern_ipc_perm.seq, and one that would set
> kern_ipc_perm.id.
> 2) the accesses to kern_ipc_perm.id must be moved to the position where the
> lock is held. This is trivial.
> 3) we need a clear table that describes which variables can be accessed
> under rcu_read_lock() and which need ipc_lock_object().
>   e.g.: kern_ipc_perm.id would end up under ipc_lock_object,
> kern_ipc_perm.seq or the xuid fields can be read under rcu_read_lock().
>   Everything that can be accessed without ipc_lock_object must be
> initialized before publication of a new object.
>
> Or, as all access to kern_ipc_perm.id are in rare codepaths:
> I'll remove kern_ipc_perm.id entirely, and build the id on demand.
>
> Ok?


Sounds like a more solid plan. If we remove kern_ipc_perm.id, then we
also don't need to split ipc_buildid() into two functions, right? And
since seq becomes constant throughout object lifetime, it probably
does not need any special access rules.

Thanks