On 2022/2/8 23:20, Mimi Zohar wrote:
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")?
Hi Mimi,
I mean commit ccf11dbaa07b ("evm: Fix memleak in init_desc") should be
reverted. commit 843385694721 ("evm: Fix a small race in init_desc()")
is just for reference.
--
Best
GUO Zihua
