[PATCH] apparmor: avoid potential double free when remapping DFA tables

[PATCH] apparmor: avoid potential double free when remapping DFA tables

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

When unpacking a DFA, aadfaunpack() stores a table in dfa->tables
before attempting to remap a 16-bit table to 32-bit.

However, remapdata16todata32() frees the original table when
allocation of the new table fails. This leaves the corresponding
dfa->tables entry pointing to freed memory. If the unpacking then
fails, dfafree() attempts to free the same table again.

Only publish the table to dfa->tables after any required remapping
has completed successfully, so that dfa->tables never contains a
pointer to a table that has already been freed.

Fixes: 9208c05f9fdf ("apparmor: add support for 2^24 states to the dfa state machine.")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 security/apparmor/match.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index 7713484..ca30a42 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -397,7 +397,6 @@ struct aa_dfa *aa_dfa_unpack(const void *blob, size_t size, int flags)
 		/* check for duplicate table entry */
 		if (dfa->tables[table->td_id])
 			goto fail;
-		dfa->tables[table->td_id] = table;
 		data += table_size(table->td_lolen, table->td_flags);
 		size -= table_size(table->td_lolen, table->td_flags);
 
@@ -414,9 +413,9 @@ struct aa_dfa *aa_dfa_unpack(const void *blob, size_t size, int flags)
 				if (!table)
 					goto fail;
 			}
-			dfa->tables[table->td_id] = table;
 			break;
 		}
+		dfa->tables[table->td_id] = table;
 		table = NULL;
 	}
 	error = verify_table_headers(dfa->tables, flags);
-- 
2.9.4

Re: [PATCH] apparmor: avoid potential double free when remapping DFA tables

[sashiko-bot]

> From: Li RongQing <lirongqing@baidu.com>
> 
> When unpacking a DFA, aadfaunpack() stores a table in dfa->tables
> before attempting to remap a 16-bit table to 32-bit.
> 
> However, remapdata16todata32() frees the original table when
> allocation of the new table fails. This leaves the corresponding
> dfa->tables entry pointing to freed memory. If the unpacking then
> fails, dfafree() attempts to free the same table again.
> 
> Only publish the table to dfa->tables after any required remapping
> has completed successfully, so that dfa->tables never contains a
> pointer to a table that has already been freed.
> 
> Fixes: 9208c05f9fdf ("apparmor: add support for 2^24 states to the dfa state machine.")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008031533.2205-1-lirongqing@baidu.com?part=1