Commit 615c999c authored by Manfred Spraul's avatar Manfred Spraul Committed by Linus Torvalds

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/20180712185241.4017-2-manfred@colorfullife.comSigned-off-by: default avatarManfred Spraul <manfred@colorfullife.com>
Reviewed-by: default avatarDavidlohr Bueso <dbueso@suse.de>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Michal Hocko <mhocko@suse.com>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 5cb366bb
...@@ -492,7 +492,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid, ...@@ -492,7 +492,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
int cmd, struct msqid64_ds *p) int cmd, struct msqid64_ds *p)
{ {
struct msg_queue *msq; struct msg_queue *msq;
int id = 0;
int err; int err;
memset(p, 0, sizeof(*p)); memset(p, 0, sizeof(*p));
...@@ -504,7 +503,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid, ...@@ -504,7 +503,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
err = PTR_ERR(msq); err = PTR_ERR(msq);
goto out_unlock; goto out_unlock;
} }
id = msq->q_perm.id;
} else { /* IPC_STAT */ } else { /* IPC_STAT */
msq = msq_obtain_object_check(ns, msqid); msq = msq_obtain_object_check(ns, msqid);
if (IS_ERR(msq)) { if (IS_ERR(msq)) {
...@@ -549,10 +547,21 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid, ...@@ -549,10 +547,21 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
p->msg_lspid = pid_vnr(msq->q_lspid); p->msg_lspid = pid_vnr(msq->q_lspid);
p->msg_lrpid = pid_vnr(msq->q_lrpid); p->msg_lrpid = pid_vnr(msq->q_lrpid);
ipc_unlock_object(&msq->q_perm); if (cmd == IPC_STAT) {
rcu_read_unlock(); /*
return id; * 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: out_unlock:
rcu_read_unlock(); rcu_read_unlock();
return err; return err;
......
...@@ -1223,7 +1223,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid, ...@@ -1223,7 +1223,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
{ {
struct sem_array *sma; struct sem_array *sma;
time64_t semotime; time64_t semotime;
int id = 0;
int err; int err;
memset(semid64, 0, sizeof(*semid64)); memset(semid64, 0, sizeof(*semid64));
...@@ -1235,7 +1234,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid, ...@@ -1235,7 +1234,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
err = PTR_ERR(sma); err = PTR_ERR(sma);
goto out_unlock; goto out_unlock;
} }
id = sma->sem_perm.id;
} else { /* IPC_STAT */ } else { /* IPC_STAT */
sma = sem_obtain_object_check(ns, semid); sma = sem_obtain_object_check(ns, semid);
if (IS_ERR(sma)) { if (IS_ERR(sma)) {
...@@ -1275,10 +1273,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid, ...@@ -1275,10 +1273,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
#endif #endif
semid64->sem_nsems = sma->sem_nsems; 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); ipc_unlock_object(&sma->sem_perm);
rcu_read_unlock();
return id;
out_unlock: out_unlock:
rcu_read_unlock(); rcu_read_unlock();
return err; return err;
......
...@@ -962,7 +962,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid, ...@@ -962,7 +962,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
int cmd, struct shmid64_ds *tbuf) int cmd, struct shmid64_ds *tbuf)
{ {
struct shmid_kernel *shp; struct shmid_kernel *shp;
int id = 0;
int err; int err;
memset(tbuf, 0, sizeof(*tbuf)); memset(tbuf, 0, sizeof(*tbuf));
...@@ -974,7 +973,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid, ...@@ -974,7 +973,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
err = PTR_ERR(shp); err = PTR_ERR(shp);
goto out_unlock; goto out_unlock;
} }
id = shp->shm_perm.id;
} else { /* IPC_STAT */ } else { /* IPC_STAT */
shp = shm_obtain_object_check(ns, shmid); shp = shm_obtain_object_check(ns, shmid);
if (IS_ERR(shp)) { if (IS_ERR(shp)) {
...@@ -1024,10 +1022,21 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid, ...@@ -1024,10 +1022,21 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
tbuf->shm_lpid = pid_vnr(shp->shm_lprid); tbuf->shm_lpid = pid_vnr(shp->shm_lprid);
tbuf->shm_nattch = shp->shm_nattch; tbuf->shm_nattch = shp->shm_nattch;
ipc_unlock_object(&shp->shm_perm); if (cmd == IPC_STAT) {
rcu_read_unlock(); /*
return id; * 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: out_unlock:
rcu_read_unlock(); rcu_read_unlock();
return err; return err;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment