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
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