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

[Helge Deller <deller@gmx.de>]

On 11/25/25 20:20, John Johansen wrote:
> On 11/25/25 07:11, Helge Deller wrote:
>> * John Johansen <john.johansen@canonical.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@gmx.de>
>>
> lgtm,
> 
> Acked-by: John Johansen <john.johansen@canonical.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.

My patch does not modify the UNPACK_ARRAY() macro, which we
possibly should adjust as well.
Adrian's testing seems to trigger only a few unaligned accesses,
so maybe it's not a issue currently.

Helge