linux-kernel - Re: linux-next 20170519

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: <CAGXu5jJZPvpQBO_DoPtDsKJ5ntV_qjkmpVeZQ59e+sNTGaBJvQ@mail.gmail.com>
Date:   Sat, 20 May 2017 22:20:29 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Valdis.Kletnieks@...edu" <valdis.kletnieks@...edu>,
        Manfred Spraul <manfred@...orfullife.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: linux-next 20170519 - semaphores broken

On Sat, May 20, 2017 at 1:18 PM,  <valdis.kletnieks@...edu> wrote:
> Seeing problems with programs that use semaphores.  The one
> that I'm getting bit by is jackd.  strace says:
>
> getuid()                                = 967
> semget(0x282929, 0, 000)                = 229376
> semop(229376, [{0, -1, SEM_UNDO}], 1)   = -1 EIDRM (Identifier removed)
> write(2, "JACK semaphore error: semop (Ide"..., 49JACK semaphore error: semop (Identifier removed)
> ) = 49
>
> Bisects down to this commit, and reverting it from 20170519 makes things work
> again.  No idea why this causes indigestion, there's probably something subtly
> wrong here....
>
> commit 337f43326737b5eb28eb13f43c27a5788da0f913
> Author: Manfred Spraul <manfred@...orfullife.com>
> Date:   Fri May 19 07:39:23 2017 +1000
>
>     ipc: merge ipc_rcu and kern_ipc_perm
>
>     ipc has two management structures that exist for every id:
>     - struct kern_ipc_perm, it contains e.g. the permissions.
>     - struct ipc_rcu, it contains the rcu head for rcu handling and
>       the refcount.

I think I found the cause of this. Prior to this change, the RCU (with
refcount) is located ahead of the struct sem_array. After this change,
the RCU and refcount is within it, so this is happening:

        sma = container_of(ipc_rcu_alloc(size), struct sem_array, sem_perm);
        if (!sma)
                return -ENOMEM;

        memset(sma, 0, size);

ipc_rcu_alloc() initializes the refcount to 1, and the memset bumps it
back to zero.

A work-around would be to wrap the memset() like this:

struct ipc_kern_perm perm;
...
perm = sma->sem_perm;
memset(sma, 0, size);
sma->sem_perm = perm;

I actually have a series that changes things much more, and moves the
refcount set to ipc_addid() which is the only place it needs to happen
(though this requires fixing up the mistaken rcu freeing on error
paths). Here's the lightly tested series, on top of -next:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/gcc-plugin/randstruct-next-20170519-ipc

Manfred, I think I could get to the same results in fewer logical
steps, but I'm curious to see what you think of what I've got first.
Mainly I've done the following things:

- remove unneeded RCU free calls (since the IPC memory is only exposed
to RCUness once ipc_addid() succeeds
- move refcount init into ipc_addid() since it only needs to be
initialized from that point on
- remove utility allocators since now nothing special needs to be done
in the general case
- result is no requirement of ipc_kern_perms position in ipc
structures and cleaner code, IMO

-Kees

-- 
Kees Cook
Pixel Security