[PATCH] ipc: fix races with kern_ipc_perm.id and .seq
From: Manfred Spraul
Date: Wed Jul 04 2018 - 04:04:49 EST
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@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>
Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
---
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
--------------E94E9557229FCA8A59998C09--