[PATCH] apparmor: make unpacked rename and attach strings owned copies

[PATCH] apparmor: make unpacked rename and attach strings owned copies

[Qihang]

unpack_profile() stores the optional rename and attach strings using
aa_unpack_str(), which returns pointers into the policy blob.
profile->rename is later freed with kfree_sensitive(), causing an
invalid free of an interior pointer on the failed-unpack path.
profile->attach.xmatch_str can instead become a dangling pointer when
the original policy blob is freed after compressed binary policy export.
Use aa_unpack_strdup() for both strings and free xmatch_str with the
attachment.

Fixes: 736ec752d95e9 ("AppArmor: policy routines for loading and unpacking policy")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 security/apparmor/policy.c        | 1 +
 security/apparmor/policy_unpack.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 94b4a7e727cc1..e9993cd53c903 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -281,6 +281,7 @@ static void free_attachment(struct aa_attachment *attach)
 	for (i = 0; i < attach->xattr_count; i++)
 		kfree_sensitive(attach->xattrs[i]);
 	kfree_sensitive(attach->xattrs);
+	kfree_sensitive(attach->xmatch_str);
 	aa_put_pdb(attach->xmatch);
 }
 
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index d9dcff167c480..9b534d56e38a5 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1120,6 +1120,7 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
 	size_t ns_len;
 	struct rhashtable_params params = { 0 };
 	char *key = NULL, *disconnected = NULL;
+	char *rename = NULL, *attach_str = NULL;
 	struct aa_data *data;
 	int error = -EPROTO;
 	kernel_cap_t tmpcap;
@@ -1159,10 +1160,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
 	rules = profile->label.rules[0];
 
 	/* profile renaming is optional */
-	(void) aa_unpack_str(e, &profile->rename, "rename");
+	(void) aa_unpack_strdup(e, &rename, "rename");
+	profile->rename = rename;
 
 	/* attachment string is optional */
-	(void) aa_unpack_str(e, &profile->attach.xmatch_str, "attach");
+	(void) aa_unpack_strdup(e, &attach_str, "attach");
+	profile->attach.xmatch_str = attach_str;
 
 	/* xmatch is optional and may be NULL */
 	error = unpack_pdb(e, &profile->attach.xmatch, false, false, &info);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] apparmor: make unpacked rename and attach strings owned copies

[Greg KH]

On Thu, Sep 03, 2026 at 12:01:38AM +0800, Qihang wrote:
> unpack_profile() stores the optional rename and attach strings using
> aa_unpack_str(), which returns pointers into the policy blob.
> profile->rename is later freed with kfree_sensitive(), causing an
> invalid free of an interior pointer on the failed-unpack path.
> profile->attach.xmatch_str can instead become a dangling pointer when
> the original policy blob is freed after compressed binary policy export.
> Use aa_unpack_strdup() for both strings and free xmatch_str with the
> attachment.
> 
> Fixes: 736ec752d95e9 ("AppArmor: policy routines for loading and unpacking policy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
> ---
>  security/apparmor/policy.c        | 1 +
>  security/apparmor/policy_unpack.c | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)

as this, and the other apparmor patch, are now public, please just do
this work on the public apparmor list, no need to get security@k.o
involved anymore.

thanks,

greg k-h

Re: [PATCH] apparmor: make unpacked rename and attach strings owned copies

[Qihang]

Hi John,

Just a quick ping on this patch.

Thanks, Qihang

On Thu, Sep 3, 2026 at 12:02 AM Qihang <q.h.hack.winter@gmail.com> wrote:
>
> unpack_profile() stores the optional rename and attach strings using
> aa_unpack_str(), which returns pointers into the policy blob.
> profile->rename is later freed with kfree_sensitive(), causing an
> invalid free of an interior pointer on the failed-unpack path.
> profile->attach.xmatch_str can instead become a dangling pointer when
> the original policy blob is freed after compressed binary policy export.
> Use aa_unpack_strdup() for both strings and free xmatch_str with the
> attachment.
>
> Fixes: 736ec752d95e9 ("AppArmor: policy routines for loading and unpacking policy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
> ---
>  security/apparmor/policy.c        | 1 +
>  security/apparmor/policy_unpack.c | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 94b4a7e727cc1..e9993cd53c903 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -281,6 +281,7 @@ static void free_attachment(struct aa_attachment *attach)
>         for (i = 0; i < attach->xattr_count; i++)
>                 kfree_sensitive(attach->xattrs[i]);
>         kfree_sensitive(attach->xattrs);
> +       kfree_sensitive(attach->xmatch_str);
>         aa_put_pdb(attach->xmatch);
>  }
>
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index d9dcff167c480..9b534d56e38a5 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -1120,6 +1120,7 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
>         size_t ns_len;
>         struct rhashtable_params params = { 0 };
>         char *key = NULL, *disconnected = NULL;
> +       char *rename = NULL, *attach_str = NULL;
>         struct aa_data *data;
>         int error = -EPROTO;
>         kernel_cap_t tmpcap;
> @@ -1159,10 +1160,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
>         rules = profile->label.rules[0];
>
>         /* profile renaming is optional */
> -       (void) aa_unpack_str(e, &profile->rename, "rename");
> +       (void) aa_unpack_strdup(e, &rename, "rename");
> +       profile->rename = rename;
>
>         /* attachment string is optional */
> -       (void) aa_unpack_str(e, &profile->attach.xmatch_str, "attach");
> +       (void) aa_unpack_strdup(e, &attach_str, "attach");
> +       profile->attach.xmatch_str = attach_str;
>
>         /* xmatch is optional and may be NULL */
>         error = unpack_pdb(e, &profile->attach.xmatch, false, false, &info);
> --
> 2.50.1 (Apple Git-155)
>