ipc_addid() is impossible to use: - for certain failures, the caller must not use ipc_rcu_putref(), because the reference counter is not yet initialized. - for other failures, the caller must use ipc_rcu_putref(), because parallel operations could be ongoing already.
The patch cleans that up, by initializing the refcount early, and by modifying all callers. The issues is related to the finding of [email protected]: syzbot found an issue with reading kern_ipc_perm.seq, here both read and write to already released memory could happen. Signed-off-by: Manfred Spraul <[email protected]> Cc: Dmitry Vyukov <[email protected]> Cc: Kees Cook <[email protected]> Cc: Davidlohr Bueso <[email protected]> --- ipc/msg.c | 2 +- ipc/sem.c | 2 +- ipc/shm.c | 2 ++ ipc/util.c | 10 ++++++++-- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index 49358f474fc9..38119c1f0da3 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -162,7 +162,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) /* ipc_addid() locks msq upon success. */ retval = ipc_addid(&msg_ids(ns), &msq->q_perm, ns->msg_ctlmni); if (retval < 0) { - call_rcu(&msq->q_perm.rcu, msg_rcu_free); + ipc_rcu_putref(&msq->q_perm, msg_rcu_free); return retval; } diff --git a/ipc/sem.c b/ipc/sem.c index d89ce69b2613..8a0a1eb05765 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -556,7 +556,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params) /* ipc_addid() locks sma upon success. */ retval = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni); if (retval < 0) { - call_rcu(&sma->sem_perm.rcu, sem_rcu_free); + ipc_rcu_putref(&sma->sem_perm, sem_rcu_free); return retval; } ns->used_sems += nsems; diff --git a/ipc/shm.c b/ipc/shm.c index f3bae59bed08..92d71abe9e8f 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -671,6 +671,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) if (is_file_hugepages(file) && shp->mlock_user) user_shm_unlock(size, shp->mlock_user); fput(file); + ipc_rcu_putref(&shp->shm_perm, shm_rcu_free); + return error; no_file: call_rcu(&shp->shm_perm.rcu, shm_rcu_free); return error; diff --git a/ipc/util.c b/ipc/util.c index 4998f8fa8ce0..f3447911c81e 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -250,7 +250,9 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) * Add an entry 'new' to the ipc ids idr. The permissions object is * initialised and the first free entry is set up and the id assigned * is returned. The 'new' entry is returned in a locked state on success. + * * On failure the entry is not locked and a negative err-code is returned. + * The caller must use ipc_rcu_putref() to free the identifier. * * Called with writer ipc_ids.rwsem held. */ @@ -260,6 +262,9 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) kgid_t egid; int idx, err; + /* 1) Initialize the refcount so that ipc_rcu_putref works */ + refcount_set(&new->refcount, 1); + if (limit > IPCMNI) limit = IPCMNI; @@ -268,9 +273,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) idr_preload(GFP_KERNEL); - refcount_set(&new->refcount, 1); spin_lock_init(&new->lock); - new->deleted = false; rcu_read_lock(); spin_lock(&new->lock); @@ -278,6 +281,8 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) new->cuid = new->uid = euid; new->gid = new->cgid = egid; + new->deleted = false; + idx = ipc_idr_alloc(ids, new); idr_preload_end(); @@ -290,6 +295,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) } } if (idx < 0) { + new->deleted = true; spin_unlock(&new->lock); rcu_read_unlock(); return idx; -- 2.17.1
