* [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
@ 2019-12-10 1:53 Paul Moore
2019-12-10 1:54 ` Paul Moore
2019-12-10 13:44 ` Stephen Smalley
0 siblings, 2 replies; 6+ messages in thread
From: Paul Moore @ 2019-12-10 1:53 UTC (permalink / raw)
To: selinux; +Cc: rsiddoji, sds, linux-security-module
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(-)
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
2019-12-10 1:53 [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert() Paul Moore
@ 2019-12-10 1:54 ` Paul Moore
2019-12-10 13:44 ` Stephen Smalley
1 sibling, 0 replies; 6+ messages in thread
From: Paul Moore @ 2019-12-10 1:54 UTC (permalink / raw)
To: selinux; +Cc: rsiddoji, Stephen Smalley, linux-security-module
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
2019-12-10 1:53 [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert() Paul Moore
2019-12-10 1:54 ` Paul Moore
@ 2019-12-10 13:44 ` Stephen Smalley
2019-12-10 15:54 ` Paul Moore
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-12-10 13:44 UTC (permalink / raw)
To: Paul Moore, selinux; +Cc: rsiddoji, linux-security-module
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;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
2019-12-10 13:44 ` Stephen Smalley
@ 2019-12-10 15:54 ` Paul Moore
2019-12-10 16:12 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2019-12-10 15:54 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, rsiddoji, linux-security-module
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
2019-12-10 15:54 ` Paul Moore
@ 2019-12-10 16:12 ` Stephen Smalley
2019-12-10 19:19 ` Paul Moore
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-12-10 16:12 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, rsiddoji, linux-security-module
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
2019-12-10 16:12 ` Stephen Smalley
@ 2019-12-10 19:19 ` Paul Moore
0 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2019-12-10 19:19 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, rsiddoji, linux-security-module
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-10 19:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 1:53 [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert() Paul Moore
2019-12-10 1:54 ` Paul Moore
2019-12-10 13:44 ` Stephen Smalley
2019-12-10 15:54 ` Paul Moore
2019-12-10 16:12 ` Stephen Smalley
2019-12-10 19:19 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.