Message-ID: <20251126104444.29002552@pumpkin>
Date: Wed, 26 Nov 2025 10:44:44 +0000
From: david laight <david.laight@...box.com>
To: John Johansen <john.johansen@...onical.com>
Cc: Helge Deller <deller@....de>, Helge Deller <deller@...nel.org>, John
 Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.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 Wed, 26 Nov 2025 01:11:45 -0800
John Johansen <john.johansen@...onical.com> wrote:

> On 11/25/25 13:13, Helge Deller wrote:
> > On 11/25/25 20:20, John Johansen wrote:  
> >> 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.  
> > 
> > My patch does not modify the UNPACK_ARRAY() macro, which we
> > possibly should adjust as well.  
> 
> Indeed. See the patch below. I am not surprised testing hasn't triggered this
> case, but a malicious userspace could certainly construct a policy that would
> trigger it. Yes it would have to be root, but I still would like to prevent
> root from being able to trigger this.
> 
> > Adrian's testing seems to trigger only a few unaligned accesses,
> > so maybe it's not a issue currently.
> >   
> I don't think the userspace compiler is generating one that is bad, but it
> possible to construct one and get it to the point where it can trip in
> UNPACK_ARRAY
> 
> commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
> Author: John Johansen <john.johansen@...onical.com>
> Date:   Tue Nov 25 13:59:32 2025 -0800
> 
>      apparmor: fix unaligned memory access of UNPACK_ARRAY
>      
>      The UNPACK_ARRAY macro has the potential to have unaligned memory
>      access when the unpacking an unaligned profile, which is caused by
>      userspace splitting up a profile container before sending it to the
>      kernel.
>      
>      While this is corner case, policy loaded from userspace should be
>      treated as untrusted so ensure that userspace can not trigger an
>      unaligned access.
>      
>      Signed-off-by: John Johansen <john.johansen@...onical.com>
> 
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b1..203f7c07529f5 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,7 +104,7 @@ struct aa_dfa {
>   	struct table_header *tables[YYTD_ID_TSIZE];
>   };
>   
> -#define byte_to_byte(X) (X)
> +#define byte_to_byte(X) *(X)

Even though is is only used once that ought to be (*(X))

>   
>   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>   	do { \
> @@ -112,7 +112,7 @@ struct aa_dfa {
>   		TTYPE *__t = (TTYPE *) TABLE; \
>   		BTYPE *__b = (BTYPE *) BLOB; \
>   		for (__i = 0; __i < LEN; __i++) { \
> -			__t[__i] = NTOHX(__b[__i]); \
> +			__t[__i] = NTOHX(&__b[__i]); \
>   		} \
>   	} while (0)
>   
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 26e82ba879d44..3dcc342337aca 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>   				     u8, u8, byte_to_byte);

Is that that just memcpy() ?

	David

>   		else if (th.td_flags == YYTD_DATA16)
>   			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u16, __be16, be16_to_cpu);
> +				     u16, __be16, get_unaligned_be16);
>   		else if (th.td_flags == YYTD_DATA32)
>   			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u32, __be32, be32_to_cpu);
> +				     u32, __be32, get_unaligned_be32);
>   		else
>   			goto fail;
>   		/* if table was vmalloced make sure the page tables are synced
> 
> 
> 

Powered by blists - more mailing lists