linux-kernel - Re: [PATCH] integrity: Fix possible multiple allocation in integrity_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <01739d83cf13c83e0545c6d0d661ebea5ac39b6c.camel@linux.ibm.com>
Date:   Tue, 30 May 2023 09:58:20 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] integrity: Fix possible multiple allocation in
 integrity_inode_get()

Hi Tianjia,

On Tue, 2023-05-30 at 20:14 +0800, Tianjia Zhang wrote:
> When integrity_inode_get() is querying and inserting the cache, there
> is a conditional race in the concurrent environment.
> 
> Query iint within the read-lock. If there is no result, allocate iint
> first and insert the iint cache in the write-lock protection. When the
> iint cache does not exist, and when multiple execution streams come at
> the same time, there will be a race condition, and multiple copies of
> iint will be allocated at the same time, and then put into the cache
> one by one under the write-lock protection.

Right, the race condition is the result of not properly implementing
"double-checked locking".  In this case, it first checks to see if the
iint cache record exists before taking the lock, but doesn't check
again after taking the integrity_iint_lock.

> 
> This is mainly because the red-black tree insertion does not perform
> duplicate detection. This is not the desired result, when this
> happens, the repeated allocation should be freed and the existing
> iint cache should be returned.
> 
> Fixes: bf2276d10ce5 ("ima: allocating iint improvements")
> Signed-off-by: Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@...il.com>
> Cc: <stable@...r.kernel.org> # v3.10+
> ---
>  security/integrity/iint.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c73858e8c6d5..d49c843a88ee 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -43,12 +43,10 @@ static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode)
>  		else if (inode > iint->inode)
>  			n = n->rb_right;
>  		else
> -			break;
> +			return iint;
>  	}
> -	if (!n)
> -		return NULL;
>  
> -	return iint;
> +	return NULL;
>  }
>  
>  /*
> @@ -115,8 +113,13 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>  				     rb_node);
>  		if (inode < test_iint->inode)
>  			p = &(*p)->rb_left;
> -		else
> +		else if (inode > test_iint->inode)
>  			p = &(*p)->rb_right;
> +		else {
> +			write_unlock(&integrity_iint_lock);
> +			kmem_cache_free(iint_cache, iint);
> +			return test_iint;
> +		}
>  	}
>  
>  	iint->inode = inode;

scripts/checkpatch.pl with the -strict option complains:

CHECK: Unbalanced braces around else statement
#56: FILE: security/integrity/iint.c:118:
+		else {

total: 0 errors, 0 warnings, 1 checks, 28 lines checked

-- 
thanks,

Mimi