* Helge Deller <[email protected]>: > On 11/26/25 11:44, david laight wrote: > > On Wed, 26 Nov 2025 01:11:45 -0800 > > John Johansen <[email protected]> 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 <[email protected]>: > > > > > > > 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 <[email protected]> > > > > > lgtm, > > > > > > > > > > Acked-by: John Johansen <[email protected]> > > > > > > > > > > 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 <[email protected]> > > > 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 <[email protected]> > > > > > > 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() ? > > No, it's memcpy() only on big-endian machines. > On little-endian machines it converts from big-endian > 16/32-bit ints to little-endian 16/32-bit ints. > > But I see some potential for optimization here: > a) on big-endian machines just use memcpy() > b) on little-endian machines use memcpy() to copy from possibly-unaligned > memory to then known-to-be-aligned destination. Then use a loop with > be32_to_cpu() instead of get_unaligned_xx() as it's faster. > > Thoughts?
Like this (untested!) patch: [PATCH] apparmor: Optimize table creation from possibly unaligned memory Source blob may come from userspace and might be unaligned. Try to optize the copying process by avoiding unaligned memory accesses. Signed-off-by: Helge Deller <[email protected]> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h index 1fbe82f5021b..225df6495c84 100644 --- a/security/apparmor/include/match.h +++ b/security/apparmor/include/match.h @@ -111,9 +111,14 @@ struct aa_dfa { typeof(LEN) __i; \ TTYPE *__t = (TTYPE *) TABLE; \ BTYPE *__b = (BTYPE *) BLOB; \ - for (__i = 0; __i < LEN; __i++) { \ - __t[__i] = NTOHX(__b[__i]); \ - } \ + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \ + /* copy to naturally aligned table address */ \ + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \ + /* convert from big-endian if necessary */ \ + if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \ + for (__i = 0; __i < LEN; __i++, __t++) { \ + *__t = NTOHX(*__t); \ + } \ } while (0) static inline size_t table_size(size_t len, size_t el_size)
