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
