Re: Problem with commit ccf11dbaa07b ("evm: Fix memleak in init_desc")

["Guozihua (Scott)" <guozihua@huawei.com>]

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