lkml.org 
[lkml]   [2018]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: ipc/msg: zalloc struct msg_queue when creating a new msq
    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

    \
     
     \ /
      Last update: 2018-07-04 16:22    [W:5.404 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site