* Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc")
@ 2022-02-08 8:53 Guozihua (Scott)
2022-02-08 15:20 ` Mimi Zohar
0 siblings, 1 reply; 3+ messages in thread
From: Guozihua (Scott) @ 2022-02-08 8:53 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, linux-integrity, linux-kernel,
linux-security-module, wangweiyang, xiujianfeng
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()")
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc")
2022-02-08 8:53 Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc") Guozihua (Scott)
@ 2022-02-08 15:20 ` Mimi Zohar
2022-02-09 1:42 ` Guozihua (Scott)
0 siblings, 1 reply; 3+ messages in thread
From: Mimi Zohar @ 2022-02-08 15:20 UTC (permalink / raw)
To: Guozihua (Scott)
Cc: Roberto Sassu, linux-integrity, linux-kernel,
linux-security-module, wangweiyang, xiujianfeng
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc")
2022-02-08 15:20 ` Mimi Zohar
@ 2022-02-09 1:42 ` Guozihua (Scott)
0 siblings, 0 replies; 3+ messages in thread
From: Guozihua (Scott) @ 2022-02-09 1:42 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, linux-integrity, linux-kernel,
linux-security-module, wangweiyang, xiujianfeng
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-09 2:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 8:53 Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc") Guozihua (Scott)
2022-02-08 15:20 ` Mimi Zohar
2022-02-09 1:42 ` Guozihua (Scott)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.