[PATCH] ipc: Fix error pointer dereference

[PATCH] ipc: Fix error pointer dereference

[Ethan Tidmore]

The function shm_lock() can return an error pointer and is not checked
for one. Add check for error pointer.

Detected by Smatch:
ipc/shm.c:1678 do_shmat() error:
'shp' dereferencing possible ERR_PTR()

Fixes: 00c2bf85d8feb ("ipc: get rid of ipc_lock_down()")
Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
 ipc/shm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index e8c7d1924c50..d4554f4e7063 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1675,12 +1675,15 @@ 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)) {
+		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.53.0

Re: [PATCH] ipc: Fix error pointer dereference

[Liam R. Howlett]

* Ethan Tidmore <ethantidmore06@gmail.com> [260217 20:17]:
> The function shm_lock() can return an error pointer and is not checked
> for one. Add check for error pointer.
> 
> Detected by Smatch:
> ipc/shm.c:1678 do_shmat() error:
> 'shp' dereferencing possible ERR_PTR()
> 
> Fixes: 00c2bf85d8feb ("ipc: get rid of ipc_lock_down()")
> Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>

You are also not extracting the error to return from the shp, so you will
return success if it fails - is that what you want to do?  If so, you
should probably state that in your change log.

It would be better to 'undo' the rwsem with a goto instead of wrapping
the entire thing in an 'if'.  That will make it easier to undo other
actions later, if necessary and keep the code more readable.

If you look at the comments to the shm_lock() call, it seems to indicate
that the lookup is shared with the readers (and that's supported by the
fact that the code uses rcu for locking internally), so I suspect that
this will not fail when holding the rwsem.

Meaning that the error return will not happen.  In fact, if you dig
through the git history, you will find that there used to be a BUG_ON()
in this case that has since been removed.


> ---
>  ipc/shm.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index e8c7d1924c50..d4554f4e7063 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1675,12 +1675,15 @@ 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)) {
> +		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.53.0
>