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

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: <uwyzgvvgclj2zdxtmfqvuxsgedqr7b6orpwrzjb47zcdzu4pj5@ylsuqebms5yc>
Date: Wed, 19 Feb 2025 09:25:05 -0500
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Jeongjun Park <aha310510@...il.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, brauner@...nel.org,
        lorenzo.stoakes@...cle.com, segoon@...nwall.com,
        linux-kernel@...r.kernel.org,
        syzbot+a2b84e569d06ca3a949c@...kaller.appspotmail.com
Subject: Re: [PATCH] ipc: fix to protect IPCS lookups using RCU

* Jeongjun Park <aha310510@...il.com> [250219 08:29]:

Thanks for looking at this.

Your subject should be v2.

> 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.

The way rcu and reference counting is used in shm is very odd.

> 
> Therefore, when calling idr_for_each() in shm_destroy_orphaned(), it should
> be modify to protect it within the RCU read critical section.

Did you test this with lockdep and prove rcu?

Taking the rcu lock has some extra constraints that may be violated in
the call chain.

> 
> Reported-by: syzbot+a2b84e569d06ca3a949c@...kaller.appspotmail.com
> Fixes: b34a6b1da371 ("ipc: introduce shm_rmid_forced sysctl")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>

WBN to Cc: the person that looked at v1 for you (especially since he's
already Cc'ed on the email).

> ---
>  ipc/shm.c | 2 ++
>  1 file changed, 2 insertions(+)

Since this is a single patch without a cover letter (which is good), you
can put some stuff here that you don't want to have in the git log to be
used for review, such as:

v1 link
changes since v1


> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 99564c870084..baef5afadfb9 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -431,8 +431,10 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
>  void shm_destroy_orphaned(struct ipc_namespace *ns)
>  {
>  	down_write(&shm_ids(ns).rwsem);
> +	rcu_read_lock();
>  	if (shm_ids(ns).in_use)
>  		idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
> +	rcu_read_unlock();
>  	up_write(&shm_ids(ns).rwsem);
>  }
>  
> --