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

[Stephen Smalley <sds@tycho.nsa.gov>]

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.
>