ipc: compute kern_ipc_perm.id under the ipc lock

ipc_addid() initializes kern_ipc_perm.id after having called
ipc_idr_alloc().

Thus a parallel semctl() or msgctl() that uses e.g.  MSG_STAT may use this
unitialized value as the return code.

The patch moves all accesses to kern_ipc_perm.id under the spin_lock().

The issues is related to the finding of
syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com: syzbot found an
issue with kern_ipc_perm.seq

Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Manfred Spraul <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Michael Kerrisk <[email protected]>
Cc: Michal Hocko <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>

diff --git a/ipc/msg.c b/ipc/msg.c
index 203281198079c660c18f9cabf8b9b61c26caf7d4..7cb89a9b24e2036f614e214352ae05e64c0317bf 100644 (file)
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -492,7 +492,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
                         int cmd, struct msqid64_ds *p)
 {
        struct msg_queue *msq;
-       int id = 0;
        int err;
 
        memset(p, 0, sizeof(*p));
@@ -504,7 +503,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
                        err = PTR_ERR(msq);
                        goto out_unlock;
                }
-               id = msq->q_perm.id;
        } else { /* IPC_STAT */
                msq = msq_obtain_object_check(ns, msqid);
                if (IS_ERR(msq)) {
@@ -549,10 +547,21 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
        p->msg_lspid  = pid_vnr(msq->q_lspid);
        p->msg_lrpid  = pid_vnr(msq->q_lrpid);
 
-       ipc_unlock_object(&msq->q_perm);
-       rcu_read_unlock();
-       return id;
+       if (cmd == IPC_STAT) {
+               /*
+                * As defined in SUS:
+                * Return 0 on success
+                */
+               err = 0;
+       } else {
+               /*
+                * MSG_STAT and MSG_STAT_ANY (both Linux specific)
+                * Return the full id, including the sequence number
+                */
+               err = msq->q_perm.id;
+       }
 
+       ipc_unlock_object(&msq->q_perm);
 out_unlock:
        rcu_read_unlock();
        return err;

diff --git a/ipc/sem.c b/ipc/sem.c
index 00ef2f743a628e0c11d33ed64da2295f9c2441ca..18e50f464f762e954dd3bcccf1e57f6591ea4541 100644 (file)
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1223,7 +1223,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 {
        struct sem_array *sma;
        time64_t semotime;
-       int id = 0;
        int err;
 
        memset(semid64, 0, sizeof(*semid64));
@@ -1235,7 +1234,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
                        err = PTR_ERR(sma);
                        goto out_unlock;
                }
-               id = sma->sem_perm.id;
        } else { /* IPC_STAT */
                sma = sem_obtain_object_check(ns, semid);
                if (IS_ERR(sma)) {
@@ -1275,10 +1273,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 #endif
        semid64->sem_nsems = sma->sem_nsems;
 
+       if (cmd == IPC_STAT) {
+               /*
+                * As defined in SUS:
+                * Return 0 on success
+                */
+               err = 0;
+       } else {
+               /*
+                * SEM_STAT and SEM_STAT_ANY (both Linux specific)
+                * Return the full id, including the sequence number
+                */
+               err = sma->sem_perm.id;
+       }
        ipc_unlock_object(&sma->sem_perm);
-       rcu_read_unlock();
-       return id;
-
 out_unlock:
        rcu_read_unlock();
        return err;

diff --git a/ipc/shm.c b/ipc/shm.c
index b204feb3827402c7655aeef27bd655b0f426a011..b3b089315d3bfc192536d9041cd6fcac0a9cf03a 100644 (file)
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -962,7 +962,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
                        int cmd, struct shmid64_ds *tbuf)
 {
        struct shmid_kernel *shp;
-       int id = 0;
        int err;
 
        memset(tbuf, 0, sizeof(*tbuf));
@@ -974,7 +973,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
                        err = PTR_ERR(shp);
                        goto out_unlock;
                }
-               id = shp->shm_perm.id;
        } else { /* IPC_STAT */
                shp = shm_obtain_object_check(ns, shmid);
                if (IS_ERR(shp)) {
@@ -1024,10 +1022,21 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
        tbuf->shm_lpid  = pid_vnr(shp->shm_lprid);
        tbuf->shm_nattch = shp->shm_nattch;
 
-       ipc_unlock_object(&shp->shm_perm);
-       rcu_read_unlock();
-       return id;
+       if (cmd == IPC_STAT) {
+               /*
+                * As defined in SUS:
+                * Return 0 on success
+                */
+               err = 0;
+       } else {
+               /*
+                * SHM_STAT and SHM_STAT_ANY (both Linux specific)
+                * Return the full id, including the sequence number
+                */
+               err = shp->shm_perm.id;
+       }
 
+       ipc_unlock_object(&shp->shm_perm);
 out_unlock:
        rcu_read_unlock();
        return err;