Re: [PATCH AUTOSEL 5.5 190/542] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()

[Stephen Smalley]

On 2/14/20 10:43 AM, Sasha Levin wrote:
> From: Paul Moore <paul@xxxxxxxxxxxxxx>
>
> [ Upstream commit d8db60cb23e49a92cf8cada3297395c7fa50fdf8 ]
>
> 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.
>
> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
> Reported-by: rsiddoji@xxxxxxxxxxxxxx
> Suggested-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> Acked-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

You should also apply 030b995ad9ece9fa2d218af4429c1c78c2342096 
("selinux: ensure we cleanup the internal AVC counters on error in 
avc_update()") which fixes one additional instance of the same kind of 
bug not addressed by this patch.

> ---
>   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 23dc888ae3056..6646300f7ccb2 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;
>   }
>