From 4791e604dcb618ed7ea1f42b2f6ca9cfe3c113c3 Mon Sep 17 00:00:00 2001 From: Manfred Spraul Date: Wed, 4 Jul 2018 10:04:49 +0200 Subject: [PATCH] ipc: fix races with kern_ipc_perm.id and .seq ipc_addid() initializes kern_ipc_perm.id and kern_ipc_perm.seq after having called ipc_idr_alloc(). Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_idr() may see an uninitialized value. The simple solution cannot be used, as the correct id is only known after ipc_idr_alloc(). Therefore: - Initialize kern_ipc_perm.seq to an invalid value, so that ipc_checkid() is guaranteed to fail. This fulfills the purpose of the sequence counter: If e.g. semget() and semop() run in parallel, then the semop() should not write into the newly created array. - Move the accesses to kern_ipc_perm.id into the code that is protected by kern_ipc_perm.lock. The patch also fixes a use-after free that can be triggered by concurrent semget() and semctl(IPC_RMID): reading kern_ipc_perm.id must happen before dropping the locks. Reported-by: syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com Signed-off-by: Manfred Spraul Cc: Dmitry Vyukov Cc: Kees Cook Cc: Davidlohr Bueso Signed-off-by: Manfred Spraul --- ipc/msg.c | 23 +++++++++++++++++------ ipc/sem.c | 23 ++++++++++++++++------- ipc/shm.c | 19 ++++++++++++++----- ipc/util.c | 8 +++++++- 4 files changed, 54 insertions(+), 19 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index 724000c15296..551c10be8d06 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -166,10 +166,12 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) return retval; } + retval = msq->q_perm.id; + ipc_unlock_object(&msq->q_perm); rcu_read_unlock(); - return msq->q_perm.id; + return retval; } static inline bool msg_fits_inqueue(struct msg_queue *msq, size_t msgsz) @@ -491,7 +493,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)); @@ -503,7 +504,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)) { @@ -548,10 +548,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 counter + */ + 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 c269fae05b24..42fcd0979a46 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -567,13 +567,14 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params) } ns->used_sems += nsems; + retval = sma->sem_perm.id; + sem_unlock(sma, -1); rcu_read_unlock(); - return sma->sem_perm.id; + return retval; } - /* * Called with sem_ids.rwsem and ipcp locked. */ @@ -1228,7 +1229,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)); @@ -1240,7 +1240,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)) { @@ -1280,10 +1279,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 counter + */ + 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 051a3e1fb8df..59fe8b3b3794 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -949,7 +949,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)); @@ -961,7 +960,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)) { @@ -1011,10 +1009,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 counter + */ + err = shp->shm_perm.id; + } + ipc_unlock_object(&shp->shm_perm); out_unlock: rcu_read_unlock(); return err; diff --git a/ipc/util.c b/ipc/util.c index ba7f96fc8a61..ba696e3e0574 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -22,7 +22,7 @@ * obtain the ipc object (kern_ipc_perm) by looking up the id in an idr * tree. * - perform initial checks (capabilities, auditing and permission, - * etc). + * check the sequence counter, etc). * - perform read-only operations, such as INFO command, that * do not demand atomicity * acquire the ipc lock (kern_ipc_perm.lock) through @@ -270,6 +270,12 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) new->cuid = new->uid = euid; new->gid = new->cgid = egid; + /* + * Initialize ->seq to ULONG_MAX, so that any early ipc_checkid() + * calls are guarenteed to fail. + */ + new->seq = ULONG_MAX; + id = ipc_idr_alloc(ids, new); idr_preload_end(); -- 2.14.4