| Subject | Re: [PATCH AUTOSEL 5.5 190/542] selinux: ensure we cleanup the internal AVC counters on error in avc_insert() | From | Stephen Smalley <> | Date | Fri, 14 Feb 2020 11:07:37 -0500 |
| |
On 2/14/20 10:43 AM, Sasha Levin wrote: > From: Paul Moore <paul@paul-moore.com> > > [ Upstream commit d8db60cb23e49a92cf8cada3297395c7fa50fdf8 ] > > Fix avc_insert() to call avc_node_kill() if we've already allocated > an AVC node and the code fails to insert the node in the cache. > > Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") > Reported-by: rsiddoji@codeaurora.org > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > Signed-off-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Sasha Levin <sashal@kernel.org>
You should also apply 030b995ad9ece9fa2d218af4429c1c78c2342096 ("selinux: ensure we cleanup the internal AVC counters on error in avc_update()") which fixes one additional instance of the same kind of bug not addressed by this patch.
> --- > security/selinux/avc.c | 51 ++++++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 27 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 23dc888ae3056..6646300f7ccb2 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -617,40 +617,37 @@ static struct avc_node *avc_insert(struct selinux_avc *avc, > struct avc_node *pos, *node = NULL; > int hvalue; > unsigned long flag; > + spinlock_t *lock; > + struct hlist_head *head; > > if (avc_latest_notif_update(avc, avd->seqno, 1)) > - goto out; > + return NULL; > > node = avc_alloc_node(avc); > - if (node) { > - struct hlist_head *head; > - spinlock_t *lock; > - int rc = 0; > - > - hvalue = avc_hash(ssid, tsid, tclass); > - avc_node_populate(node, ssid, tsid, tclass, avd); > - rc = avc_xperms_populate(node, xp_node); > - if (rc) { > - kmem_cache_free(avc_node_cachep, node); > - return NULL; > - } > - head = &avc->avc_cache.slots[hvalue]; > - lock = &avc->avc_cache.slots_lock[hvalue]; > + if (!node) > + return NULL; > > - spin_lock_irqsave(lock, flag); > - hlist_for_each_entry(pos, head, list) { > - if (pos->ae.ssid == ssid && > - pos->ae.tsid == tsid && > - pos->ae.tclass == tclass) { > - avc_node_replace(avc, node, pos); > - goto found; > - } > + avc_node_populate(node, ssid, tsid, tclass, avd); > + if (avc_xperms_populate(node, xp_node)) { > + avc_node_kill(avc, node); > + return NULL; > + } > + > + hvalue = avc_hash(ssid, tsid, tclass); > + head = &avc->avc_cache.slots[hvalue]; > + lock = &avc->avc_cache.slots_lock[hvalue]; > + spin_lock_irqsave(lock, flag); > + hlist_for_each_entry(pos, head, list) { > + if (pos->ae.ssid == ssid && > + pos->ae.tsid == tsid && > + pos->ae.tclass == tclass) { > + avc_node_replace(avc, node, pos); > + goto found; > } > - hlist_add_head_rcu(&node->list, head); > -found: > - spin_unlock_irqrestore(lock, flag); > } > -out: > + hlist_add_head_rcu(&node->list, head); > +found: > + spin_unlock_irqrestore(lock, flag); > return node; > } > >
|