linux-kernel - Re: [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed

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 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:   Thu, 23 Sep 2021 18:36:16 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Alexander Mihalicyn <alexander@...alicyn.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Milton Miller <miltonm@....com>,
        Jack Miller <millerjo@...ibm.com>,
        Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...e.com>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Andrei Vagin <avagin@...il.com>,
        Christian Brauner <christian@...uner.io>
Subject: Re: [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace
 was changed

Hi Eric,

I'd like to restart the discussion, the issue should be fixed.

On 7/12/21 9:18 PM, Eric W. Biederman wrote:
>
> Given that this is a bug I think c) is the safest option.
>
> A couple of suggestions.
> 1) We can replace the test "shm_creator != NULL" with
>     "list_empty(&shp->shm_clist)"

Yes, good idea. list_del() already contains WRITE_ONCE() and 
list_empty() contains READ_ONCE(), i.e. we get clear memory ordering rules.


>   and remove shm_creator.

I do not see how we can remove shm_creator:

- thread A: creates new segment, doesn't map it.

- thread B: shmctl(,IPC_RMID,).

thread B must now locate the lock that protects ->shm_clist. And if the 
lock is per-thread, then I do not see how we can avoid having a pointer 
in shp to the lock.


>     Along with replacing "shm_creator = NULL" with
>     "list_del_init(&shp->shm_clist)".
Correct, list_del_init() is better than shm_create = NULL.
> 2) We can update shmat to do "list_del_init(&shp->shm_clist)"
>     upon shmat.

That would be a (tiny) user space visible change:

echo 0 > /proc/sys/kernel/shm_rmid_forced
shmget()
shmat()
shmdt()
echo 1 > /proc/sys/kernel/shm_rmid_forced
exit()

Right now: segment is destroyed

After your proposal: Segment is not destroyed.

I don't think that we should mix that with the bugfix.


>    The last unmap will still shm_destroy the
>     shm segment as ns->shm_rmid_forced is set.
But what if shm_rmid_forced is modified?
>     For a multi-threaded process I think this will nicely clean up
>     the clist, and make it clear that the clist only cares about
>     those segments that have been created but never attached.

> 3) Put a non-reference counted struct ipc_namespace in struct
>     shmid_kernel, and use it to remove the namespace parameter
>     from shm_destroy.
>
> I think that is enough to fix this bug with no changes in semantics,
> no additional memory consumed, and an implementation that is easier
> to read and perhaps a little faster.

I do not see how this solves the list corruption:

A thread creates 2 shm segments, then switches the namespace and creates 
another 2 segment.

- corruption 1: in each of the namespaces, one thread calls 
shmctl(,IPC_RMID,) -> both will operate in parallel on ->shm_clist.

- corruption 2: exit_shm() in parallel to one thread

- corruption 3: one shmctl(,IPC_RMID,) in parallel to a shmget().

i.e.: we can have list_add() and multiple list_del() in parallel.

I don't see how this should work without a lock.

With regards to memory usage: I would propose to use task_lock(), that 
lock exists already.


--

     Manfred