Re: [PATCH ipc] ipc/shm: fix race between shm_try_destroy_orphaned and do_shmat

[Davidlohr Bueso <dave@stgolabs.net>]

Cc Manfred.

On Sat, 28 Mar 2026, Xiang Mei wrote:

>shm_try_destroy_orphaned() reads shm_nattch via shm_may_destroy()
>without holding the per-object spinlock, relying on rwsem alone for
>synchronization. However, do_shmat() modifies shm_nattch under the
>per-object spinlock (not rwsem), creating a TOCTOU race:
>
>  CPU 0 (do_shmat)              CPU 1 (shm_try_destroy_orphaned)
>  ipc_lock_object()
>  shm_nattch++ (0 -> 1)
>  ipc_unlock_object()
>                                shm_may_destroy() -> reads stale nattch==0
>                                shm_lock_by_ptr()
>                                shm_destroy()  // nattch is actually 1!

More like:

   CPU 0 (do_shmat)              CPU 1 (shm_try_destroy_orphaned)
   ipc_lock_object()
				shm_may_destroy() -> reads nattch==0
   shm_nattch++ (0 -> 1)
   ipc_unlock_object()
				shm_lock_by_ptr()
				shm_destroy()  // nattch is actually 1!

>The segment is destroyed while do_shmat() has already incremented
>shm_nattch and is proceeding with the mmap setup.  When do_shmat()
>later reaches out_nattch, shm_lock() returns ERR_PTR (the IDR entry
>was removed by shm_destroy) and the code dereferences it without an
>IS_ERR() check, causing a null-ptr-deref crash:

I think that an IS_ERR() check should also be added to do_shmat(),
it makes the code more robust (mitigating the actual bug fixed here
to not trigger the crash).

>
> BUG: kernel NULL pointer dereference, address: 0000000000000072
> RIP: 0010:do_shmat (ipc/shm.c:1678)
> Call Trace:
>  __x64_sys_shmat (ipc/shm.c:1699 ipc/shm.c:1693 ipc/shm.c:1693)
>  do_syscall_64 (arch/x86/entry/syscall_64.c:94)
>  [...]
>
>Fix by taking the object lock before checking shm_may_destroy() in
>shm_try_destroy_orphaned(), so the check sees the up-to-date value
>of shm_nattch.

It's probably fine to unconditionally take the object lock in this
path (not performance critical). But I think it might be better to
just re-check shm_may_destroy() after taking the lock.

>
>Fixes: 4c677e2eefdb ("shm: optimize locking and ipc_namespace getting")

Old bug, seems not many users of shm_rmid_forced.

Thanks,
Davidlohr

>Reported-by: Weiming Shi <bestswngs@gmail.com>
>Signed-off-by: Xiang Mei <xmei5@asu.edu>
>---
> ipc/shm.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/ipc/shm.c b/ipc/shm.c
>index a95dae447707..50f9aa7ff33a 100644
>--- a/ipc/shm.c
>+++ b/ipc/shm.c
>@@ -416,17 +416,18 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
>
>	/*
>	 * We want to destroy segments without users and with already
>-	 * exit'ed originating process.
>-	 *
>-	 * As shp->* are changed under rwsem, it's safe to skip shp locking.
>+	 * exit'ed originating process. Take the object lock before
>+	 * checking shm_may_destroy() since shm_nattch can be modified
>+	 * under the object lock alone (e.g. by do_shmat).
>	 */
>	if (!list_empty(&shp->shm_clist))
>		return 0;
>
>-	if (shm_may_destroy(shp)) {
>-		shm_lock_by_ptr(shp);
>+	shm_lock_by_ptr(shp);
>+	if (shm_may_destroy(shp))
>		shm_destroy(ns, shp);
>-	}
>+	else
>+		shm_unlock(shp);
>	return 0;
> }
>
>--
>2.43.0
>