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

[Paul Moore]

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