Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Fri, 09 Jun 2023 17:24:52 +0300
Message-Id: <CT86SNGF201H.2UZF8SN2MEKZ6@suppilovahvero>
Subject: Re: [PATCH v2] integrity: Fix possible multiple allocation in
 integrity_inode_get()
From:   "Jarkko Sakkinen" <jarkko@kernel.org>
To:     "Tianjia Zhang" <tianjia.zhang@linux.alibaba.com>,
        "Mimi Zohar" <zohar@linux.ibm.com>,
        "Dmitry Kasatkin" <dmitry.kasatkin@gmail.com>,
        "Paul Moore" <paul@paul-moore.com>,
        "James Morris" <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        <linux-integrity@vger.kernel.org>,
        <linux-security-module@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230530121453.10249-1-tianjia.zhang@linux.alibaba.com>
 <20230601064244.33633-1-tianjia.zhang@linux.alibaba.com>
In-Reply-To: <20230601064244.33633-1-tianjia.zhang@linux.alibaba.com>
Precedence: bulk

On Thu Jun 1, 2023 at 9:42 AM EEST, Tianjia Zhang wrote:
> When integrity_inode_get() is querying and inserting the cache, there
> is a conditional race in the concurrent environment.
>
> 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.
>
> Fixes: bf2276d10ce5 ("ima: allocating iint improvements")
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: <stable@vger.kernel.org> # v3.10+

s/v3.10/v4.14/

I.e. cover only currently maintained longterms, right?


> ---
>  security/integrity/iint.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c73858e8c6d5..a462df827de2 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 =3D n->rb_right;
>  		else
> -			break;
> +			return iint;
>  	}
> -	if (!n)
> -		return NULL;
> =20
> -	return iint;
> +	return NULL;
>  }
> =20
>  /*
> @@ -113,10 +111,15 @@ struct integrity_iint_cache *integrity_inode_get(st=
ruct inode *inode)
>  		parent =3D *p;
>  		test_iint =3D rb_entry(parent, struct integrity_iint_cache,
>  				     rb_node);
> -		if (inode < test_iint->inode)
> +		if (inode < test_iint->inode) {
>  			p =3D &(*p)->rb_left;
> -		else
> +		} else if (inode > test_iint->inode) {
>  			p =3D &(*p)->rb_right;
> +		} else {
> +			write_unlock(&integrity_iint_lock);
> +			kmem_cache_free(iint_cache, iint);
> +			return test_iint;
> +		}
>  	}
> =20
>  	iint->inode =3D inode;
> --=20
> 2.24.3 (Apple Git-128)

Mimi, are you picking this?

Off-topic: how do you compile kernel on macOS, you're using VM right?
I'm just interested because I recently bought Mac mini for both
compiling and testing arm64. Optimal would be to be able to compile
the kernel on bare metal and then deploy to a VM...


BR, Jarkko