apparmor: Fix two bugs of aa_setup_dfa_engine's fail handling

[PATCH] apparmor: Fix two bugs of aa_setup_dfa_engine's fail handling

Posted by GONG Ruiqi 2 months, 2 weeks ago

First, aa_dfa_unpack returns ERR_PTR not NULL when it fails, but
aa_put_dfa only checks NULL for its input, which would cause invalid
memory access in aa_put_dfa. Set nulldfa to NULL explicitly to fix that.

Second, aa_put_pdb calls aa_pdb_free_kref -> aa_free_pdb -> aa_put_dfa,
i.e.  it will free nullpdb->dfa. But there's another aa_put_dfa(nulldfa)
after aa_put_pdb(nullpdb), which would cause double free. Remove that
redundant aa_put_dfa to fix that.

Fixes: 98b824ff8984 ("apparmor: refcount the pdb")
Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
---
 security/apparmor/lsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index c1d42fc72fdb..be82ec1b9fd9 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -2465,6 +2465,7 @@ static int __init aa_setup_dfa_engine(void)
 			    TO_ACCEPT2_FLAG(YYTD_DATA32));
 	if (IS_ERR(nulldfa)) {
 		error = PTR_ERR(nulldfa);
+		nulldfa = NULL;
 		goto fail;
 	}
 	nullpdb->dfa = aa_get_dfa(nulldfa);
@@ -2486,7 +2487,6 @@ static int __init aa_setup_dfa_engine(void)
 
 fail:
 	aa_put_pdb(nullpdb);
-	aa_put_dfa(nulldfa);
 	nullpdb = NULL;
 	nulldfa = NULL;
 	stacksplitdfa = NULL;
-- 
2.43.0

Re: [PATCH] apparmor: Fix two bugs of aa_setup_dfa_engine's fail handling

Posted by Georgia Garcia 1 month, 4 weeks ago

Hello,

On Fri, 2026-04-03 at 11:51 +0800, GONG Ruiqi wrote:
> First, aa_dfa_unpack returns ERR_PTR not NULL when it fails, but
> aa_put_dfa only checks NULL for its input, which would cause invalid
> memory access in aa_put_dfa. Set nulldfa to NULL explicitly to fix that.
> 
> Second, aa_put_pdb calls aa_pdb_free_kref -> aa_free_pdb -> aa_put_dfa,
> i.e.  it will free nullpdb->dfa. But there's another aa_put_dfa(nulldfa)
> after aa_put_pdb(nullpdb), which would cause double free. Remove that
> redundant aa_put_dfa to fix that.
> 
> Fixes: 98b824ff8984 ("apparmor: refcount the pdb")
> Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
> ---
>  security/apparmor/lsm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..be82ec1b9fd9 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -2465,6 +2465,7 @@ static int __init aa_setup_dfa_engine(void)
>  			    TO_ACCEPT2_FLAG(YYTD_DATA32));
>  	if (IS_ERR(nulldfa)) {
>  		error = PTR_ERR(nulldfa);
> +		nulldfa = NULL;
>  		goto fail;
>  	}
>  	nullpdb->dfa = aa_get_dfa(nulldfa);
> @@ -2486,7 +2487,6 @@ static int __init aa_setup_dfa_engine(void)
>  
>  fail:
>  	aa_put_pdb(nullpdb);
> -	aa_put_dfa(nulldfa);

This isn't right. aa_dfa_unpack does kref_init(&dfa->count), and later
we have nullpdb->dfa = aa_get_dfa(nulldfa);
So the second is put on aa_put_pdb but the first, from the init, does
need to be put too.

>  	nullpdb = NULL;
>  	nulldfa = NULL;
>  	stacksplitdfa = NULL;

Re: [PATCH] apparmor: Fix two bugs of aa_setup_dfa_engine's fail handling

Posted by GONG Ruiqi 1 month, 4 weeks ago

Hi Georgia,

On 4/23/2026 5:51 AM, Georgia Garcia wrote:
> ...
>> @@ -2486,7 +2487,6 @@ static int __init aa_setup_dfa_engine(void)
>>  
>>  fail:
>>  	aa_put_pdb(nullpdb);
>> -	aa_put_dfa(nulldfa);
> 
> This isn't right. aa_dfa_unpack does kref_init(&dfa->count), and later
> we have nullpdb->dfa = aa_get_dfa(nulldfa);
> So the second is put on aa_put_pdb but the first, from the init, does
> need to be put too.

Thanks for the feedback, and yes you're right. I didn't notice there's a
kref_init in aa_dfa_unpack...

I will submit a patch that only contains the first fix.

BR,
Ruiqi

> 
>>  	nullpdb = NULL;
>>  	nulldfa = NULL;
>>  	stacksplitdfa = NULL;
>

Re: [PATCH] apparmor: Fix two bugs of aa_setup_dfa_engine's fail handling

Posted by GONG Ruiqi 2 months ago

Kindly ping.

On 4/3/2026 11:51 AM, GONG Ruiqi wrote:
> First, aa_dfa_unpack returns ERR_PTR not NULL when it fails, but
> aa_put_dfa only checks NULL for its input, which would cause invalid
> memory access in aa_put_dfa. Set nulldfa to NULL explicitly to fix that.
> 
> Second, aa_put_pdb calls aa_pdb_free_kref -> aa_free_pdb -> aa_put_dfa,
> i.e.  it will free nullpdb->dfa. But there's another aa_put_dfa(nulldfa)
> after aa_put_pdb(nullpdb), which would cause double free. Remove that
> redundant aa_put_dfa to fix that.
> 
> Fixes: 98b824ff8984 ("apparmor: refcount the pdb")
> Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
> ---
>  security/apparmor/lsm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..be82ec1b9fd9 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -2465,6 +2465,7 @@ static int __init aa_setup_dfa_engine(void)
>  			    TO_ACCEPT2_FLAG(YYTD_DATA32));
>  	if (IS_ERR(nulldfa)) {
>  		error = PTR_ERR(nulldfa);
> +		nulldfa = NULL;
>  		goto fail;
>  	}
>  	nullpdb->dfa = aa_get_dfa(nulldfa);
> @@ -2486,7 +2487,6 @@ static int __init aa_setup_dfa_engine(void)
>  
>  fail:
>  	aa_put_pdb(nullpdb);
> -	aa_put_dfa(nulldfa);
>  	nullpdb = NULL;
>  	nulldfa = NULL;
>  	stacksplitdfa = NULL;