On 12/10/19 10:54 AM, Paul Moore wrote:
On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@xxxxxxxxxxxxx> 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.
