Re: [PATCH 0/2] apparmor unaligned memory fixes

[John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>]

Hi Helge,

On Tue, 2025-11-25 at 16:11 +0100, Helge Deller wrote:
> Regarding this:
> 
> > Kernel side, we are going to need to add some extra verification checks, it should
> > be catching this, as unaligned as part of the unpack. Userspace side, we will have
> > to verify my guess and fix the loader.
> 
> I wonder if loading those tables are really time critical?
> If not, maybe just making the kernel aware that the tables might be unaligned
> can help, e.g. with the following (untested) patch.
> Adrian, maybe you want to test?
> 
> ------------------------
> 
> [PATCH] Allow apparmor to handle unaligned dfa tables
> 
> The dfa tables can originate from kernel or userspace and 8-byte alignment
> isn't always guaranteed and as such may trigger unaligned memory accesses
> on various architectures.
> Work around it by using the get_unaligned_xx() helpers.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..26e82ba879d4 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -15,6 +15,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/err.h>
>  #include <linux/kref.h>
> +#include <linux/unaligned.h>
>  
>  #include "include/lib.h"
>  #include "include/match.h"
> @@ -42,11 +43,11 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  	/* loaded td_id's start at 1, subtract 1 now to avoid doing
>  	 * it every time we use td_id as an index
>  	 */
> -	th.td_id = be16_to_cpu(*(__be16 *) (blob)) - 1;
> +	th.td_id = get_unaligned_be16(blob) - 1;
>  	if (th.td_id > YYTD_ID_MAX)
>  		goto out;
> -	th.td_flags = be16_to_cpu(*(__be16 *) (blob + 2));
> -	th.td_lolen = be32_to_cpu(*(__be32 *) (blob + 8));
> +	th.td_flags = get_unaligned_be16(blob + 2);
> +	th.td_lolen = get_unaligned_be32(blob + 8);
>  	blob += sizeof(struct table_header);
>  
>  	if (!(th.td_flags == YYTD_DATA16 || th.td_flags == YYTD_DATA32 ||
> @@ -313,14 +314,14 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>  	if (size < sizeof(struct table_set_header))
>  		goto fail;
>  
> -	if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
> +	if (get_unaligned_be32(data) != YYTH_MAGIC)
>  		goto fail;
>  
> -	hsize = ntohl(*(__be32 *) (data + 4));
> +	hsize = get_unaligned_be32(data + 4);
>  	if (size < hsize)
>  		goto fail;
>  
> -	dfa->flags = ntohs(*(__be16 *) (data + 12));
> +	dfa->flags = get_unaligned_be16(data + 12);
>  	if (dfa->flags & ~(YYTH_FLAGS))
>  		goto fail;
>  
> @@ -329,7 +330,7 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>  	 * if (dfa->flags & YYTH_FLAGS_OOB_TRANS) {
>  	 *	if (hsize < 16 + 4)
>  	 *		goto fail;
> -	 *	dfa->max_oob = ntol(*(__be32 *) (data + 16));
> +	 *	dfa->max_oob = get_unaligned_be32(data + 16);
>  	 *	if (dfa->max <= MAX_OOB_SUPPORTED) {
>  	 *		pr_err("AppArmor DFA OOB greater than supported\n");
>  	 *		goto fail;

I can confirm that this fixes the unaligned access warnings.

Without the patch:

[   72.073526] audit: type=1400 audit(1764145307.711:2): apparmor="STATUS" operation="profile_load" profile="unconfined" name="1password" pid=292 comm="apparmor_parser"
[   72.413269] audit: type=1400 audit(1764145308.051:3): apparmor="STATUS" operation="profile_load" profile="unconfined" name="Discord" pid=294 comm="apparmor_parser"
[   72.645135] audit: type=1400 audit(1764145308.283:4): apparmor="STATUS" operation="profile_load" profile="unconfined" name=4D6F6E676F444220436F6D70617373 pid=296 comm="apparmor_parser"
[   72.901297] audit: type=1400 audit(1764145308.539:5): apparmor="STATUS" operation="profile_load" profile="unconfined" name="QtWebEngineProcess" pid=297 comm="apparmor_parser"
[   73.245252] audit: type=1400 audit(1764145308.879:6): apparmor="STATUS" operation="profile_load" profile="unconfined" name="Xorg" pid=298 comm="apparmor_parser"
[   73.468571] audit: type=1400 audit(1764145309.107:7): apparmor="STATUS" operation="profile_load" profile="unconfined" name="balena-etcher" pid=299 comm="apparmor_parser"
[   73.688642] audit: type=1400 audit(1764145309.327:8): apparmor="STATUS" operation="profile_load" profile="unconfined" name="brave" pid=300 comm="apparmor_parser"
[   73.897068] audit: type=1400 audit(1764145309.531:9): apparmor="STATUS" operation="profile_load" profile="unconfined" name="buildah" pid=301 comm="apparmor_parser"
[   74.104434] audit: type=1400 audit(1764145309.739:10): apparmor="STATUS" operation="profile_load" profile="unconfined" name="busybox" pid=302 comm="apparmor_parser"
[   74.313359] audit: type=1400 audit(1764145309.951:11): apparmor="STATUS" operation="profile_load" profile="unconfined" name="cam" pid=303 comm="apparmor_parser"
[   74.808437] Kernel unaligned access at TPC[8dabdc] aa_dfa_unpack+0x3c/0x6e0
[   74.900032] Kernel unaligned access at TPC[8dabec] aa_dfa_unpack+0x4c/0x6e0
[   74.991608] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[   75.084339] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[   75.176997] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0

With the patch:

[   78.058157] audit: type=1400 audit(1764145018.691:2): apparmor="STATUS" operation="profile_load" profile="unconfined" name="1password" pid=294 comm="apparmor_parser"
[   78.294742] audit: type=1400 audit(1764145018.927:3): apparmor="STATUS" operation="profile_load" profile="unconfined" name="Discord" pid=295 comm="apparmor_parser"
[   78.516989] audit: type=1400 audit(1764145019.127:4): apparmor="STATUS" operation="profile_load" profile="unconfined" name=4D6F6E676F444220436F6D70617373 pid=297 comm="apparmor_parser"
[   78.748842] audit: type=1400 audit(1764145019.379:5): apparmor="STATUS" operation="profile_load" profile="unconfined" name="QtWebEngineProcess" pid=298 comm="apparmor_parser"
[   79.101544] audit: type=1400 audit(1764145019.731:6): apparmor="STATUS" operation="profile_load" profile="unconfined" name="Xorg" pid=299 comm="apparmor_parser"
[   79.335655] audit: type=1400 audit(1764145019.967:7): apparmor="STATUS" operation="profile_load" profile="unconfined" name="balena-etcher" pid=300 comm="apparmor_parser"
[   79.559475] audit: type=1400 audit(1764145020.191:8): apparmor="STATUS" operation="profile_load" profile="unconfined" name="brave" pid=301 comm="apparmor_parser"
[   79.768389] audit: type=1400 audit(1764145020.399:9): apparmor="STATUS" operation="profile_load" profile="unconfined" name="buildah" pid=302 comm="apparmor_parser"
[   79.974008] audit: type=1400 audit(1764145020.607:10): apparmor="STATUS" operation="profile_load" profile="unconfined" name="busybox" pid=303 comm="apparmor_parser"
[   80.194378] audit: type=1400 audit(1764145020.827:11): apparmor="STATUS" operation="profile_load" profile="unconfined" name="cam" pid=304 comm="apparmor_parser"

So, it seems your approach works as expected.

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913