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;
Message-ID: <ec4348e54b39811b727a29f3c23972eab616dcd3.camel@linux.ibm.com>
Subject: Re: Problem with commit ccf11dbaa07b ("evm: Fix memleak in
 init_desc")
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     "Guozihua (Scott)" <guozihua@huawei.com>
Cc:     Roberto Sassu <roberto.sassu@huawei.com>,
        "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-security-module@vger.kernel.org,
        wangweiyang <wangweiyang2@huawei.com>,
        "xiujianfeng@huawei.com" <xiujianfeng@huawei.com>
Date:   Tue, 08 Feb 2022 10:20:30 -0500
In-Reply-To: <e852660c-17fa-cd75-e361-45dd77b8884d@huawei.com>
References: <e852660c-17fa-cd75-e361-45dd77b8884d@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2022-02-08 at 16:53 +0800, Guozihua (Scott) wrote:
> Hi Mimi,
> 
> I found an issue with commit ccf11dbaa07b ("evm: Fix memleak in init_desc").
> 
> This commit tries to free variable "tmp_tfm" if something went wrong 
> after the "alloc" label in function init_desc, which would potentially 
> cause a user-after-free issue
> 
> The codes are as follows:
> 
>    1 static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>    2 {
>    3 	long rc;
>    4 	const char *algo;
>    5 	struct crypto_shash **tfm, *tmp_tfm = NULL;
>    6 	struct shash_desc *desc;
>    7
>    8 	if (type == EVM_XATTR_HMAC) {
>    9 		if (!(evm_initialized & EVM_INIT_HMAC)) {
>   10 			pr_err_once("HMAC key is not set\n");
>   11 			return ERR_PTR(-ENOKEY);
>   12 		}
>   13 		tfm = &hmac_tfm;
>   14 		algo = evm_hmac;
>   15 	} else {
>   16 		if (hash_algo >= HASH_ALGO__LAST)
>   17 			return ERR_PTR(-EINVAL);
>   18
>   19 		tfm = &evm_tfm[hash_algo];
>   20 		algo = hash_algo_name[hash_algo];
>   21 	}
>   22
>   23 	if (*tfm)
>   24 		goto alloc;
>   25 	mutex_lock(&mutex);
>   26 	if (*tfm)
>   27 		goto unlock;
>   28
>   29 	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
>   30 	if (IS_ERR(tmp_tfm)) {
>   31 		pr_err("Can not allocate %s (reason: %ld)\n", algo,
>   32 		       PTR_ERR(tmp_tfm));
>   33 		mutex_unlock(&mutex);
>   34 		return ERR_CAST(tmp_tfm);
>   35 	}
>   36 	if (type == EVM_XATTR_HMAC) {
>   37 		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
>   38 		if (rc) {
>   39 			crypto_free_shash(tmp_tfm);
>   40 			⋅mutex_unlock(&mutex);
>   41 			return ERR_PTR(rc);
>   42 		}
>   43 	}
>   44 	*tfm = tmp_tfm;
>   45 unlock:
>   46 	mutex_unlock(&mutex);
>   47 alloc:
>   48 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>   49 			GFP_KERNEL);
>   50 	if (!desc) {
>   51 		crypto_free_shash(tmp_tfm);
>   52 		return ERR_PTR(-ENOMEM);
>   53 	}
>   54
>   55 	desc->tfm = *tfm;
>   56
>   57 	rc = crypto_shash_init(desc);
>   58 	if (rc) {
>   59 		crypto_free_shash(tmp_tfm);
>   60 		kfree(desc);
>   61 		return ERR_PTR(rc);
>   62 	}
>   63 	return desc;
>   64 }
> 
> As we can see, variable *tfm points to one of the two global variable 
> hmac_tfm or evm_tfm[hash_algo]. tmp_tfm is used as an intermediate 
> variable for initializing these global variables. Freeing tmp_tfm after 
> line 44 would invalidate these global variables and potentially cause a 
> user-after-free issue.
> 
> I think this commit should be reverted.
> 
> Reference: commit 843385694721 ("evm: Fix a small race in init_desc()")

Why this one, as opposed to commit ccf11dbaa07b ("evm: Fix memleak in
init_desc")? 

-- 
thanks,

Mimi