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

[Davidlohr Bueso]

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@xxxxxxxxx>
> Signed-off-by: Xiang Mei <xmei5@xxxxxxx>
> ---
> 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