Message-ID: <e88c32c2-fb18-4f3e-9ec2-a749695aaf0a@canonical.com>
Date: Tue, 25 Nov 2025 11:20:54 -0800
From: John Johansen <john.johansen@...onical.com>
To: Helge Deller <deller@...nel.org>,
 John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
Cc: Helge Deller <deller@....de>, linux-kernel@...r.kernel.org,
 apparmor@...ts.ubuntu.com, linux-security-module@...r.kernel.org,
 linux-parisc@...r.kernel.org
Subject: Re: [PATCH 0/2] apparmor unaligned memory fixes

On 11/25/25 07:11, Helge Deller wrote:
> * John Johansen <john.johansen@...onical.com>:
>> On 11/18/25 04:49, Helge Deller wrote:
>>> Hi Adrian,
>>>
>>> On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
>>>> On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
>>>>> My patch fixed two call sites, but I suspect you see another call site which
>>>>> hasn't been fixed yet.
>>>>>
>>>>> Can you try attached patch? It might indicate the caller of the function and
>>>>> maybe prints the struct name/address which isn't aligned.
>>>>>
>>>>> Helge
>>>>>
>>>>>
>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>> index c5a91600842a..b477430c07eb 100644
>>>>> --- a/security/apparmor/match.c
>>>>> +++ b/security/apparmor/match.c
>>>>> @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>>>>>        if (size < sizeof(struct table_set_header))
>>>>>            goto fail;
>>>>> +    if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
>>>>> +        pr_warn("dfa blob stream %pS not aligned.\n", data);
>>>>> +
>>>>>        if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
>>>>>            goto fail;
>>>>
>>>> Here is the relevant output with the patch applied:
>>>>
>>>> [   73.840639] ------------[ cut here ]------------
>>>> [   73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
>>>> [   74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
>>>> sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
>>>> [   74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
>>>> [   74.536543] Call Trace:
>>>> [   74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
>>>> [   74.633757] [<0000000000476438>] __warn+0xd8/0x100
>>>> [   74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
>>>> [   74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
>>>> [   74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
>>>> [   74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
>>>> [   74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
>>>> [   75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
>>>> [   75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
>>>> [   75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
>>>> [   75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
>>>> [   75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
>>>> [   75.406932] [<0000000000767174>] sys_write+0x14/0x40
>>>> [   75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
>>>> [   75.548802] ---[ end trace 0000000000000000 ]---
>>>> [   75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
>>>> [   75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
>>>
>>> The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
>>> (via the write syscall).
>>> Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
>>> a source address which is not correctly aligned.
>>
>> the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
>> is but the userspace tooling hasn't been treating it as aligned. With that said,
>> the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
>> and there really should be some validation to catch it.
>>
>>> You should be able to debug/find the problematic code with strace from userspace.
>>> Maybe someone with apparmor knowledge here on the list has an idea?
>>>
>> This is likely an unaligned 2nd profile, being split out and loaded separately
>> from the rest of the container. Basically the loader for some reason (there
>> are a few different possible reasons) is poking into the container format and
>> pulling out the profile at some offset, this gets loaded to the kernel but
>> it would seem that its causing an issue with the dfa alignment within the container,
>> which should be aligned to the original container.
> 
> 
> 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?

no, most policy is loaded once on boot and then at package upgrades. There are some
bits that may be loaded at application startup like, snap, libvirt, lxd, basically
container managers might do some thing custom per container.

Its the run time we want to minimize, the cost of.

Policy already can be unaligned (the container format rework to fix this is low
priority), and is treated as untrusted. It goes through an unpack, and translation to
machine native, with as many bounds checks, necessary transforms etc done at unpack
time as possible, so that the run time costs can be minimized.

> 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@....de>
> 
lgtm,

Acked-by: John Johansen <john.johansen@...onical.com>

I'll pull this into my tree regardless of whether it fixes the issue
for Adrian, as it definitely fixes an issue.

We can added additional patches on top s needed.


> 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;

Powered by blists - more mailing lists