lkml.org 
[lkml]   [2021]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.10 124/154] ipc: WARN if trying to remove ipc object which is absent
    Date
    From: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>

    commit 126e8bee943e9926238c891e2df5b5573aee76bc upstream.

    Patch series "shm: shm_rmid_forced feature fixes".

    Some time ago I met kernel crash after CRIU restore procedure,
    fortunately, it was CRIU restore, so, I had dump files and could do
    restore many times and crash reproduced easily. After some
    investigation I've constructed the minimal reproducer. It was found
    that it's use-after-free and it happens only if sysctl
    kernel.shm_rmid_forced = 1.

    The key of the problem is that the exit_shm() function not handles shp's
    object destroy when task->sysvshm.shm_clist contains items from
    different IPC namespaces. In most cases this list will contain only
    items from one IPC namespace.

    How can this list contain object from different namespaces? The
    exit_shm() function is designed to clean up this list always when
    process leaves IPC namespace. But we made a mistake a long time ago and
    did not add a exit_shm() call into the setns() syscall procedures.

    The first idea was just to add this call to setns() syscall but it
    obviously changes semantics of setns() syscall and that's
    userspace-visible change. So, I gave up on this idea.

    The first real attempt to address the issue was just to omit forced
    destroy if we meet shp object not from current task IPC namespace [1].
    But that was not the best idea because task->sysvshm.shm_clist was
    protected by rwsem which belongs to current task IPC namespace. It
    means that list corruption may occur.

    Second approach is just extend exit_shm() to properly handle shp's from
    different IPC namespaces [2]. This is really non-trivial thing, I've
    put a lot of effort into that but not believed that it's possible to
    make it fully safe, clean and clear.

    Thanks to the efforts of Manfred Spraul working an elegant solution was
    designed. Thanks a lot, Manfred!

    Eric also suggested the way to address the issue in ("[RFC][PATCH] shm:
    In shm_exit destroy all created and never attached segments") Eric's
    idea was to maintain a list of shm_clists one per IPC namespace, use
    lock-less lists. But there is some extra memory consumption-related
    concerns.

    An alternative solution which was suggested by me was implemented in
    ("shm: reset shm_clist on setns but omit forced shm destroy"). The idea
    is pretty simple, we add exit_shm() syscall to setns() but DO NOT
    destroy shm segments even if sysctl kernel.shm_rmid_forced = 1, we just
    clean up the task->sysvshm.shm_clist list.

    This chages semantics of setns() syscall a little bit but in comparision
    to the "naive" solution when we just add exit_shm() without any special
    exclusions this looks like a safer option.

    [1] https://lkml.org/lkml/2021/7/6/1108
    [2] https://lkml.org/lkml/2021/7/14/736

    This patch (of 2):

    Let's produce a warning if we trying to remove non-existing IPC object
    from IPC namespace kht/idr structures.

    This allows us to catch possible bugs when the ipc_rmid() function was
    called with inconsistent struct ipc_ids*, struct kern_ipc_perm*
    arguments.

    Link: https://lkml.kernel.org/r/20211027224348.611025-1-alexander.mikhalitsyn@virtuozzo.com
    Link: https://lkml.kernel.org/r/20211027224348.611025-2-alexander.mikhalitsyn@virtuozzo.com
    Co-developed-by: Manfred Spraul <manfred@colorfullife.com>
    Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
    Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
    Cc: "Eric W. Biederman" <ebiederm@xmission.com>
    Cc: Davidlohr Bueso <dave@stgolabs.net>
    Cc: Greg KH <gregkh@linuxfoundation.org>
    Cc: Andrei Vagin <avagin@gmail.com>
    Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
    Cc: Vasily Averin <vvs@virtuozzo.com>
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    ipc/util.c | 6 +++---
    1 file changed, 3 insertions(+), 3 deletions(-)

    --- a/ipc/util.c
    +++ b/ipc/util.c
    @@ -446,8 +446,8 @@ static int ipcget_public(struct ipc_name
    static void ipc_kht_remove(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
    {
    if (ipcp->key != IPC_PRIVATE)
    - rhashtable_remove_fast(&ids->key_ht, &ipcp->khtnode,
    - ipc_kht_params);
    + WARN_ON_ONCE(rhashtable_remove_fast(&ids->key_ht, &ipcp->khtnode,
    + ipc_kht_params));
    }

    /**
    @@ -462,7 +462,7 @@ void ipc_rmid(struct ipc_ids *ids, struc
    {
    int idx = ipcid_to_idx(ipcp->id);

    - idr_remove(&ids->ipcs_idr, idx);
    + WARN_ON_ONCE(idr_remove(&ids->ipcs_idr, idx) != ipcp);
    ipc_kht_remove(ids, ipcp);
    ids->in_use--;
    ipcp->deleted = true;

    \
     
     \ /
      Last update: 2021-11-24 14:39    [W:3.296 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site