* [PATCH -next] Revert "evm: Fix memleak in init_desc"
@ 2022-05-27 11:17 Xiu Jianfeng
2022-06-10 3:13 ` xiujianfeng
2022-06-10 14:20 ` Mimi Zohar
0 siblings, 2 replies; 5+ messages in thread
From: Xiu Jianfeng @ 2022-05-27 11:17 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel
This reverts commit ccf11dbaa07b328fa469415c362d33459c140a37.
Commit ccf11dbaa07b ("evm: Fix memleak in init_desc") said there is
memleak in init_desc. That may be incorrect, as we can see, tmp_tfm is
saved in one of the two global variables hmac_tfm or evm_tfm[hash_algo],
then if init_desc is called next time, there is no need to alloc tfm
again, so in the error path of kmalloc desc or crypto_shash_init(desc),
It is not a problem without freeing tmp_tfm.
And also that commit did not reset the global variable to NULL after
freeing tmp_tfm and this makes *tfm a dangling pointer which may cause a
UAF issue.
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
security/integrity/evm/evm_crypto.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a733aff02006..708de9656bbd 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -75,7 +75,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
{
long rc;
const char *algo;
- struct crypto_shash **tfm, *tmp_tfm = NULL;
+ struct crypto_shash **tfm, *tmp_tfm;
struct shash_desc *desc;
if (type == EVM_XATTR_HMAC) {
@@ -120,16 +120,13 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
alloc:
desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
GFP_KERNEL);
- if (!desc) {
- crypto_free_shash(tmp_tfm);
+ if (!desc)
return ERR_PTR(-ENOMEM);
- }
desc->tfm = *tfm;
rc = crypto_shash_init(desc);
if (rc) {
- crypto_free_shash(tmp_tfm);
kfree(desc);
return ERR_PTR(rc);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next] Revert "evm: Fix memleak in init_desc"
2022-05-27 11:17 [PATCH -next] Revert "evm: Fix memleak in init_desc" Xiu Jianfeng
@ 2022-06-10 3:13 ` xiujianfeng
2022-06-10 14:20 ` Mimi Zohar
1 sibling, 0 replies; 5+ messages in thread
From: xiujianfeng @ 2022-06-10 3:13 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel
Hi, ping....
在 2022/5/27 19:17, Xiu Jianfeng 写道:
> This reverts commit ccf11dbaa07b328fa469415c362d33459c140a37.
>
> Commit ccf11dbaa07b ("evm: Fix memleak in init_desc") said there is
> memleak in init_desc. That may be incorrect, as we can see, tmp_tfm is
> saved in one of the two global variables hmac_tfm or evm_tfm[hash_algo],
> then if init_desc is called next time, there is no need to alloc tfm
> again, so in the error path of kmalloc desc or crypto_shash_init(desc),
> It is not a problem without freeing tmp_tfm.
>
> And also that commit did not reset the global variable to NULL after
> freeing tmp_tfm and this makes *tfm a dangling pointer which may cause a
> UAF issue.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
> security/integrity/evm/evm_crypto.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a733aff02006..708de9656bbd 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -75,7 +75,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
> {
> long rc;
> const char *algo;
> - struct crypto_shash **tfm, *tmp_tfm = NULL;
> + struct crypto_shash **tfm, *tmp_tfm;
> struct shash_desc *desc;
>
> if (type == EVM_XATTR_HMAC) {
> @@ -120,16 +120,13 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
> alloc:
> desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> GFP_KERNEL);
> - if (!desc) {
> - crypto_free_shash(tmp_tfm);
> + if (!desc)
> return ERR_PTR(-ENOMEM);
> - }
>
> desc->tfm = *tfm;
>
> rc = crypto_shash_init(desc);
> if (rc) {
> - crypto_free_shash(tmp_tfm);
> kfree(desc);
> return ERR_PTR(rc);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] Revert "evm: Fix memleak in init_desc"
2022-05-27 11:17 [PATCH -next] Revert "evm: Fix memleak in init_desc" Xiu Jianfeng
2022-06-10 3:13 ` xiujianfeng
@ 2022-06-10 14:20 ` Mimi Zohar
2022-06-11 2:38 ` Guozihua (Scott)
2022-06-11 3:24 ` xiujianfeng
1 sibling, 2 replies; 5+ messages in thread
From: Mimi Zohar @ 2022-06-10 14:20 UTC (permalink / raw)
To: Xiu Jianfeng, dmitry.kasatkin, jmorris, serge, Guozihua (Scott)
Cc: linux-integrity, linux-security-module, linux-kernel
On Fri, 2022-05-27 at 19:17 +0800, Xiu Jianfeng wrote:
> This reverts commit ccf11dbaa07b328fa469415c362d33459c140a37.
>
> Commit ccf11dbaa07b ("evm: Fix memleak in init_desc") said there is
> memleak in init_desc. That may be incorrect, as we can see, tmp_tfm is
> saved in one of the two global variables hmac_tfm ohr evm_tfm[hash_algo],
> then if init_desc is called next time, there is no need to alloc tfm
> again, so in the error path of kmalloc desc or crypto_shash_init(desc),
> It is not a problem without freeing tmp_tfm.
>
> And also that commit did not reset the global variable to NULL after
> freeing tmp_tfm and this makes *tfm a dangling pointer which may cause a
> UAF issue.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
Agreed, thanks. This was first reported by Guozihua (Scott) <
guozihua@huawei.com>. If neither you nor Scott object, I'll add his
Reported-by tag.
thanks,
Mimi
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] Revert "evm: Fix memleak in init_desc"
2022-06-10 14:20 ` Mimi Zohar
@ 2022-06-11 2:38 ` Guozihua (Scott)
2022-06-11 3:24 ` xiujianfeng
1 sibling, 0 replies; 5+ messages in thread
From: Guozihua (Scott) @ 2022-06-11 2:38 UTC (permalink / raw)
To: Mimi Zohar, Xiu Jianfeng, dmitry.kasatkin, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel
On 2022/6/10 22:20, Mimi Zohar wrote:
> On Fri, 2022-05-27 at 19:17 +0800, Xiu Jianfeng wrote:
>> This reverts commit ccf11dbaa07b328fa469415c362d33459c140a37.
>>
>> Commit ccf11dbaa07b ("evm: Fix memleak in init_desc") said there is
>> memleak in init_desc. That may be incorrect, as we can see, tmp_tfm is
>> saved in one of the two global variables hmac_tfm ohr evm_tfm[hash_algo],
>> then if init_desc is called next time, there is no need to alloc tfm
>> again, so in the error path of kmalloc desc or crypto_shash_init(desc),
>> It is not a problem without freeing tmp_tfm.
>>
>> And also that commit did not reset the global variable to NULL after
>> freeing tmp_tfm and this makes *tfm a dangling pointer which may cause a
>> UAF issue.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>
> Agreed, thanks. This was first reported by Guozihua (Scott) <
> guozihua@huawei.com>. If neither you nor Scott object, I'll add his
> Reported-by tag.
>
> thanks,
>
> Mimi
>
> .
Good for me.
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] Revert "evm: Fix memleak in init_desc"
2022-06-10 14:20 ` Mimi Zohar
2022-06-11 2:38 ` Guozihua (Scott)
@ 2022-06-11 3:24 ` xiujianfeng
1 sibling, 0 replies; 5+ messages in thread
From: xiujianfeng @ 2022-06-11 3:24 UTC (permalink / raw)
To: Mimi Zohar, dmitry.kasatkin, jmorris, serge, Guozihua (Scott)
Cc: linux-integrity, linux-security-module, linux-kernel
在 2022/6/10 22:20, Mimi Zohar 写道:
> On Fri, 2022-05-27 at 19:17 +0800, Xiu Jianfeng wrote:
>> This reverts commit ccf11dbaa07b328fa469415c362d33459c140a37.
>>
>> Commit ccf11dbaa07b ("evm: Fix memleak in init_desc") said there is
>> memleak in init_desc. That may be incorrect, as we can see, tmp_tfm is
>> saved in one of the two global variables hmac_tfm ohr evm_tfm[hash_algo],
>> then if init_desc is called next time, there is no need to alloc tfm
>> again, so in the error path of kmalloc desc or crypto_shash_init(desc),
>> It is not a problem without freeing tmp_tfm.
>>
>> And also that commit did not reset the global variable to NULL after
>> freeing tmp_tfm and this makes *tfm a dangling pointer which may cause a
>> UAF issue.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> Agreed, thanks. This was first reported by Guozihua (Scott) <
> guozihua@huawei.com>. If neither you nor Scott object, I'll add his
> Reported-by tag.
Good for me, thanks.
>
> thanks,
>
> Mimi
>
>
>
>
>
> .
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-11 3:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27 11:17 [PATCH -next] Revert "evm: Fix memleak in init_desc" Xiu Jianfeng
2022-06-10 3:13 ` xiujianfeng
2022-06-10 14:20 ` Mimi Zohar
2022-06-11 2:38 ` Guozihua (Scott)
2022-06-11 3:24 ` xiujianfeng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).