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

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

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;
>   }
>   
>