[RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()

[Paul Moore]

On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds at 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