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 | 12 ++++++++++-- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index 829c2062ded4..5bf5cb8017ea 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 e8971fa1d847..9d49efeac2e5 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 59fe8b3b3794..06b7bf11a011 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 662c28c6c9fa..8b09496ed720 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -248,7 +248,9 @@ static inline void ipc_set_seq(struct ipc_ids *ids, * 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. */ @@ -258,6 +260,9 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) kgid_t egid; int id, err; + /* 1) Initialize the refcount so that ipc_rcu_putref works */ + refcount_set(&new->refcount, 1); + if (limit > IPCMNI) limit = IPCMNI; @@ -266,9 +271,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); @@ -277,6 +280,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) new->gid = new->cgid = egid; ipc_set_seq(ids, new); + new->deleted = false; /* * As soon as a new object is inserted into the idr, @@ -288,6 +292,9 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) * Thus the object must be fully initialized, and if something fails, * then the full tear-down sequence must be followed. * (i.e.: set new->deleted, reduce refcount, call_rcu()) + * + * This function sets new->deleted, the caller must use ipc_rcu_putef() + * for the remaining steps. */ id = ipc_idr_alloc(ids, new); idr_preload_end(); @@ -301,6 +308,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) } } if (id < 0) { + new->deleted = true; spin_unlock(&new->lock); rcu_read_unlock(); return id; -- 2.17.1
