[PATCH] ipc/shm: check shm_lock() in do_shmat cleanup

[PATCH] ipc/shm: check shm_lock() in do_shmat cleanup

[Yi Xie]

shm_lock() can fail; don't dereference the error pointer.

Signed-off-by: Yi Xie <xieyi@kylinos.cn>
---
 ipc/shm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ipc/shm.c b/ipc/shm.c
index b3e8a58e177d..bdcf1c0f221d 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1677,6 +1677,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
 out_nattch:
 	down_write(&shm_ids(ns).rwsem);
 	shp = shm_lock(ns, shmid);
+	if (IS_ERR(shp)) {
+		up_write(&shm_ids(ns).rwsem);
+		return err;
+	}
 	shp->shm_nattch--;
 
 	if (shm_may_destroy(shp))
-- 
2.25.1

Re: [PATCH] ipc/shm: check shm_lock() in do_shmat cleanup

[Lorenzo Stoakes (ARM)]

On Tue, Jul 14, 2026 at 11:17:13AM +0800, Yi Xie wrote:
> shm_lock() can fail; don't dereference the error pointer.
>
> Signed-off-by: Yi Xie <xieyi@kylinos.cn>

Yikes yeah. Presumably this is pretty hard to hit, but seems legit.

> ---
>  ipc/shm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index b3e8a58e177d..bdcf1c0f221d 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1677,6 +1677,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
>  out_nattch:
>  	down_write(&shm_ids(ns).rwsem);
>  	shp = shm_lock(ns, shmid);
> +	if (IS_ERR(shp)) {
> +		up_write(&shm_ids(ns).rwsem);
> +		return err;
> +	}

You're duplicating the ugly up_write() here. Probably better to do something like:

-       shp->shm_nattch--;
+       if (IS_ERR(shp)) {
+               err = PTR_ERR(shp);
+       } else {
+               shp->shm_nattch--;

-       if (shm_may_destroy(shp))
-               shm_destroy(ns, shp);
-       else
-               shm_unlock(shp);
+               if (shm_may_destroy(shp))
+                       shm_destroy(ns, shp);
+               else
+                       shm_unlock(shp);
+       }

And keep the up_write() shared.

>  	shp->shm_nattch--;
>
>  	if (shm_may_destroy(shp))
> --
> 2.25.1
>

Thanks, Lorenzo

[PATCH v2] ipc/shm: check shm_lock() in do_shmat cleanup

[Yi Xie]

shm_lock() can fail; don't dereference the error pointer.

Signed-off-by: Yi Xie <xieyi@kylinos.cn>
---
v2: keep a single up_write(), as suggested by Lorenzo

 ipc/shm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index b3e8a58e177d..d243137c4dbd 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1677,12 +1677,16 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
 out_nattch:
 	down_write(&shm_ids(ns).rwsem);
 	shp = shm_lock(ns, shmid);
-	shp->shm_nattch--;
+	if (IS_ERR(shp)) {
+		err = PTR_ERR(shp);
+	} else {
+		shp->shm_nattch--;
 
-	if (shm_may_destroy(shp))
-		shm_destroy(ns, shp);
-	else
-		shm_unlock(shp);
+		if (shm_may_destroy(shp))
+			shm_destroy(ns, shp);
+		else
+			shm_unlock(shp);
+	}
 	up_write(&shm_ids(ns).rwsem);
 	return err;
 
-- 
2.25.1

Re: [PATCH v2] ipc/shm: check shm_lock() in do_shmat cleanup

[Lorenzo Stoakes (ARM)]

Please don't send v2 in-reply-to v1 :) it makes it really hard to keep
track of things.

Please wait _at least_ 24 hours before sending a respin. We have far too
much review load to have an immediate turnaround.

Also sending out so quick doesn't make me think you've tested this :)

On Tue, Jul 14, 2026 at 05:22:37PM +0800, Yi Xie wrote:
> shm_lock() can fail; don't dereference the error pointer.

Maybe something a bit longer, e.g.:

	do_shmat() calls shm_lock() in the out_nattch branch and
	immediately dereferences it, however shm_lock() can return an
	error.

	Check for an error and handle it if there is one.

>
> Signed-off-by: Yi Xie <xieyi@kylinos.cn>
> ---
> v2: keep a single up_write(), as suggested by Lorenzo
>
>  ipc/shm.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index b3e8a58e177d..d243137c4dbd 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1677,12 +1677,16 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
>  out_nattch:
>  	down_write(&shm_ids(ns).rwsem);
>  	shp = shm_lock(ns, shmid);
> -	shp->shm_nattch--;
> +	if (IS_ERR(shp)) {
> +		err = PTR_ERR(shp);
> +	} else {
> +		shp->shm_nattch--;
>
> -	if (shm_may_destroy(shp))
> -		shm_destroy(ns, shp);
> -	else
> -		shm_unlock(shp);
> +		if (shm_may_destroy(shp))
> +			shm_destroy(ns, shp);
> +		else
> +			shm_unlock(shp);
> +	}
>  	up_write(&shm_ids(ns).rwsem);
>  	return err;
>
> --
> 2.25.1
>

Cheers, Lorenzo