linux-kernel - Re: [PATCH] ipc: fix to protect IPCS lookups using RCU instead of semaphore

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] [day] [month] [year] [list]

Message-ID: <CAO9qdTEUvMg-z9XUnLU35hpXHc_CVWK_7koCHHzw7wVqN68ufw@mail.gmail.com>
Date: Wed, 19 Feb 2025 19:07:37 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Liam.Howlett@...cle.com, brauner@...nel.org, 
	lorenzo.stoakes@...cle.com, Davidlohr Bueso <dave@...olabs.net>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipc: fix to protect IPCS lookups using RCU instead of semaphore

Matthew Wilcox <willy@...radead.org> wrote:
>
> [Fixing Davidlohr's email address -- David could you pop an entry into
> .mailmap?]
>
> On Sat, Feb 15, 2025 at 02:00:23PM +0900, Jeongjun Park wrote:
> > Andrew Morton <akpm@...ux-foundation.org> wrote:
> > >
> > > On Sat, 15 Feb 2025 03:01:57 +0900 Jeongjun Park <aha310510@...il.com> wrote:
> > >
> > > > In shm_destroy_orphaned(), we are not performing updates to the IPCS and are
> > > > only calling idr_for_each(), which can be protected by the RCU read-critical
> > > > section.
> > > >
> > > > And if idr_for_each() is not protected by the RCU read-critical section,
> > > > then when radix_tree_node_free() is called to free the struct radix_tree_node
> > > > through call_rcu(), the node will be freed immediately, and when reading the
> > > > next node in radix_tree_for_each_slot(), the memory that has already been
> > > > freed may be read.
> > >
> > > A use-after-free?
> > >
> > > Is there any report of this occurring, or was this change a result of
> > > code inspection?  If the former, please share details (Link:,
> > > Reported-by:, Closes:, etc).
> >
> > Reported-by: syzbot+a2b84e569d06ca3a949c@...kaller.appspotmail.com
>
> For anyone else trying to understand what the hell this is about,
> the report is at:
>
> https://lore.kernel.org/linux-kernel/67af13f8.050a0220.21dd3.0038.GAE@google.com/
>
> > Sorry I forgot the Reported-by tag. I think the vulnerability is caused by
> > misusing RCU. In addition, since it is a function that does not perform
> > an update operation, it is possible to protect it through RCU, so we can
> > safely get some performance small benefits by using RCU instead of
> > semaphore.
>
> shm uses RCU in a very weird way.  You're absolutely right that we need
> to hold the RCU read lock here to prevent the tree nodes from being
> freed below us.
>
> But I'm not convinced that removing the rwsem is safe.  Look at what
> else is in the call chain:
>
>        if (shm_ids(ns).in_use)
>                 idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
>
> shm_try_destroy_orphaned:
>                 shm_destroy(ns, shp);
> shm_destroy:
>         shm_rmid(shp);
> shm_rmid:
>         ipc_rmid(&shm_ids(s->ns), &s->shm_perm);
> ipc_rmid:
>         WARN_ON_ONCE(idr_remove(&ids->ipcs_idr, idx) != ipcp);
>
> So what's protecting that idr_remove() if not the rwsem?

After looking into it again, I guess removing the semaphore
won't work. Thanks for letting me know!

I'll fix it and send you a patch right away.

Regards,

Jeongjun Park

>
> (the shortcut for this is that shm_destroy() actually documents that it
> needs shm_ids.rwsem held.  But it doesn't have a lockdep assertion.
> Perhaps you could add one?)