From: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx> Subject: 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@xxxxxxxxxxxxxxxxxxxxxxxxx: syzbot found an issue with kern_ipc_perm.seq Link: http://lkml.kernel.org/r/20180712185241.4017-2-manfred@xxxxxxxxxxxxxxxx Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx> Reviewed-by: Davidlohr Bueso <dbueso@xxxxxxx> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx> Cc: Kees Cook <keescook@xxxxxxxxxxxx> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> Cc: Michael Kerrisk <mtk.manpages@xxxxxxxxx> Cc: Michal Hocko <mhocko@xxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- ipc/msg.c | 19 ++++++++++++++----- ipc/sem.c | 18 +++++++++++++----- ipc/shm.c | 19 ++++++++++++++----- 3 files changed, 41 insertions(+), 15 deletions(-) --- a/ipc/msg.c~ipc-ipc-compute-kern_ipc_permid-under-the-ipc-lock +++ a/ipc/msg.c @@ -492,7 +492,6 @@ static int msgctl_stat(struct ipc_namesp 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_namesp 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_namesp 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; --- a/ipc/sem.c~ipc-ipc-compute-kern_ipc_permid-under-the-ipc-lock +++ a/ipc/sem.c @@ -1223,7 +1223,6 @@ static int semctl_stat(struct ipc_namesp { 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_namesp 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_namesp #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; --- a/ipc/shm.c~ipc-ipc-compute-kern_ipc_permid-under-the-ipc-lock +++ a/ipc/shm.c @@ -962,7 +962,6 @@ static int shmctl_stat(struct ipc_namesp 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_namesp 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_namesp 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; _
