| Message ID | 157594281322.676903.11041338053333686450.stgit@chester (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RFC] selinux: ensure we cleanup the internal AVC counters on error in avc_insert() | expand |
On Mon, Dec 9, 2019 at 8:53 PM Paul Moore <paul@paul-moore.com> wrote: > In AVC insert we don't call avc_node_kill() when avc_xperms_populate() > fails, resulting in the avc->avc_cache.active_nodes counter having a > false value. This patch corrects this problem and does some cleanup > in avc_insert() while we are there. > > Reported-by: rsiddoji@codeaurora.org > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/avc.c | 51 +++++++++++++++++++++++------------------------- > 1 file changed, 24 insertions(+), 27 deletions(-) FYI, only compiled tested, thus the RFC.
On 12/9/19 8:53 PM, Paul Moore wrote: > In AVC insert we don't call avc_node_kill() when avc_xperms_populate() > fails, resulting in the avc->avc_cache.active_nodes counter having a > false value. incorrect value? This patch corrects this problem and does some cleanup > in avc_insert() while we are there. submitting-patches.rst recommends describing in imperative mood and avoiding the words "patch" in what will eventually just be a commit log, ala "Correct this problem and perform some cleanup..." Should probably add a: Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") Might be easier to back port if you split the cleanup from the fix, but your call of course. > > Reported-by: rsiddoji@codeaurora.org > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> > Signed-off-by: Paul Moore <paul@paul-moore.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > 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 23dc888ae305..6646300f7ccb 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; > } > >
On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 12/9/19 8:53 PM, Paul Moore wrote: > > In AVC insert we don't call avc_node_kill() when avc_xperms_populate() > > fails, resulting in the avc->avc_cache.active_nodes counter having a > > false value. > > incorrect value? > > This patch corrects this problem and does some cleanup > > in avc_insert() while we are there. > > submitting-patches.rst recommends describing in imperative mood and > avoiding the words "patch" in what will eventually just be a commit log, > ala "Correct this problem and perform some cleanup..." Well, you've made me feel better about my nit-picky comments on patches ;) Are you okay with the following? selinux: ensure we cleanup the internal AVC counters on error in avc_insert() 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. > Should probably add a: > > Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") > > Might be easier to back port if you split the cleanup from the fix, but > your call of course. I waffled on that last night when I wrote up the patch, and more generally if this should go to -stable or -next (despite what is claimed, adding a "Fixes:" tag means it gets picked up by -stable more often than not in my experience). At its worst, not fixing this bug means we could end up effectively shrinking the AVC cache if xperms are used *and* we happen to fail a memory allocation while adding a new entry to the AVC; we don't cause an incorrect node to be cached, we don't crash the system, we don't leak memory. My thinking is that this isn't a major concern, and not worth the risk to -stable, but if anyone has any data that shows otherwise, please let me know. I'll go ahead and add the "Fixes:" tag (technically this is the *right* thing to do), but I'm going to stick with -next and leave the cleanup as-is just to raise the bar a bit for the -stable backports which I'm sure are going to happen.
On 12/10/19 10:54 AM, Paul Moore wrote: > On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 12/9/19 8:53 PM, Paul Moore wrote: >>> In AVC insert we don't call avc_node_kill() when avc_xperms_populate() >>> fails, resulting in the avc->avc_cache.active_nodes counter having a >>> false value. >> >> incorrect value? >> >> This patch corrects this problem and does some cleanup >>> in avc_insert() while we are there. >> >> submitting-patches.rst recommends describing in imperative mood and >> avoiding the words "patch" in what will eventually just be a commit log, >> ala "Correct this problem and perform some cleanup..." > > Well, you've made me feel better about my nit-picky comments on patches ;) > > Are you okay with the following? > > selinux: ensure we cleanup the internal AVC counters on error in avc_insert() > > 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. Sure, or just "Fix the AVC to correctly decrement the count of AVC nodes if it encounters an allocation failure on an extended permissions node." > >> Should probably add a: >> >> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") >> >> Might be easier to back port if you split the cleanup from the fix, but >> your call of course. > > I waffled on that last night when I wrote up the patch, and more > generally if this should go to -stable or -next (despite what is > claimed, adding a "Fixes:" tag means it gets picked up by -stable more > often than not in my experience). At its worst, not fixing this bug > means we could end up effectively shrinking the AVC cache if xperms > are used *and* we happen to fail a memory allocation while adding a > new entry to the AVC; we don't cause an incorrect node to be cached, > we don't crash the system, we don't leak memory. My thinking is that > this isn't a major concern, and not worth the risk to -stable, but if > anyone has any data that shows otherwise, please let me know. > > I'll go ahead and add the "Fixes:" tag (technically this is the > *right* thing to do), but I'm going to stick with -next and leave the > cleanup as-is just to raise the bar a bit for the -stable backports > which I'm sure are going to happen. >
On Tue, Dec 10, 2019 at 11:12 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 12/10/19 10:54 AM, Paul Moore wrote: > > On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> On 12/9/19 8:53 PM, Paul Moore wrote: > >>> In AVC insert we don't call avc_node_kill() when avc_xperms_populate() > >>> fails, resulting in the avc->avc_cache.active_nodes counter having a > >>> false value. > >> > >> incorrect value? > >> > >> This patch corrects this problem and does some cleanup > >>> in avc_insert() while we are there. > >> > >> submitting-patches.rst recommends describing in imperative mood and > >> avoiding the words "patch" in what will eventually just be a commit log, > >> ala "Correct this problem and perform some cleanup..." > > > > Well, you've made me feel better about my nit-picky comments on patches ;) > > > > Are you okay with the following? > > > > selinux: ensure we cleanup the internal AVC counters on error in avc_insert() > > > > 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. > > Sure, or just "Fix the AVC to correctly decrement the count of AVC nodes > if it encounters an allocation failure on an extended permissions node." > > >> Should probably add a: > >> > >> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") > >> > >> Might be easier to back port if you split the cleanup from the fix, but > >> your call of course. > > > > I waffled on that last night when I wrote up the patch, and more > > generally if this should go to -stable or -next (despite what is > > claimed, adding a "Fixes:" tag means it gets picked up by -stable more > > often than not in my experience). At its worst, not fixing this bug > > means we could end up effectively shrinking the AVC cache if xperms > > are used *and* we happen to fail a memory allocation while adding a > > new entry to the AVC; we don't cause an incorrect node to be cached, > > we don't crash the system, we don't leak memory. My thinking is that > > this isn't a major concern, and not worth the risk to -stable, but if > > anyone has any data that shows otherwise, please let me know. > > > > I'll go ahead and add the "Fixes:" tag (technically this is the > > *right* thing to do), but I'm going to stick with -next and leave the > > cleanup as-is just to raise the bar a bit for the -stable backports > > which I'm sure are going to happen. Merged into selinux/next.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 23dc888ae305..6646300f7ccb 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; }
In AVC insert we don't call avc_node_kill() when avc_xperms_populate() fails, resulting in the avc->avc_cache.active_nodes counter having a false value. This patch corrects this problem and does some cleanup in avc_insert() while we are there. Reported-by: rsiddoji@codeaurora.org Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/avc.c | 51 +++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 27 deletions(-)