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

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

[Helge Deller]

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

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

[John Johansen]

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.


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

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

[Helge Deller]

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

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

[John Johansen]

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@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.

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@canonical.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@canonical.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)
  
  #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);
  		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

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

[david laight]

On Wed, 26 Nov 2025 01:11:45 -0800
John Johansen <john.johansen@canonical.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@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.  
> 
> 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@canonical.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@canonical.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
> 
> 
> 

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

[Helge Deller]

On 11/26/25 11:44, david laight wrote:
> On Wed, 26 Nov 2025 01:11:45 -0800
> John Johansen <john.johansen@canonical.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@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.
>>
>> 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@canonical.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@canonical.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() ?

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?

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

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

[Helge Deller]

* Helge Deller <deller@gmx.de>:
> On 11/26/25 11:44, david laight wrote:
> > On Wed, 26 Nov 2025 01:11:45 -0800
> > John Johansen <john.johansen@canonical.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@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.
> > > 
> > > 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@canonical.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@canonical.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() ?
> 
> 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 <deller@gmx.de>

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)

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

[John Paul Adrian Glaubitz]

Hi Helge,

On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
> 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 <deller@gmx.de>
> 
> 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)

So, is this patch supposed to replace all the other proposed patches?

Adrian

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

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

[Helge Deller]

On 11/26/25 17:16, John Paul Adrian Glaubitz wrote:
> Hi Helge,
> 
> On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
>> 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 <deller@gmx.de>
>>
>> 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)
> 
> So, is this patch supposed to replace all the other proposed patches?

It just replaces the patch from John.
But please test the v2 patch I sent instead...

Helge

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

[John Paul Adrian Glaubitz]

Hi Helge,

On Wed, 2025-11-26 at 17:58 +0100, Helge Deller wrote:
> On 11/26/25 17:16, John Paul Adrian Glaubitz wrote:
> > Hi Helge,
> > 
> > On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
> > > 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 <deller@gmx.de>
> > > 
> > > 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)
> > 
> > So, is this patch supposed to replace all the other proposed patches?
> 
> It just replaces the patch from John.
> But please test the v2 patch I sent instead...

OK, so we need your previous patch that I already tested and your v2 of John's patch?

Adrian

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

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

[david laight]

On Wed, 26 Nov 2025 12:03:03 +0100
Helge Deller <deller@gmx.de> wrote:

> On 11/26/25 11:44, david laight wrote:
...   
> >> 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.

You've misread the quoting...
The 'data8' case that was only half there is a memcpy().

> 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()

true

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

There is a function that does a loop byteswap of a buffer - no reason
to re-invent it.
But I doubt it is always (if ever) faster to do a copy and then byteswap.
The loop control and extra memory accesses kill performance.

Not that I've seen a fast get_unaligned() - I don't think gcc or clang
generate optimal code - For LE I think it is something like:
	low = *(addr & ~3);
	high = *((addr + 3) & ~3);
	shift = (addr & 3) * 8;
	value = low << shift | high >> (32 - shift);
Note that it is only 2 aligned memory reads - even for 64bit.

	David

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

[Helge Deller]

* david laight <david.laight@runbox.com>:
> On Wed, 26 Nov 2025 12:03:03 +0100
> Helge Deller <deller@gmx.de> wrote:
> 
> > On 11/26/25 11:44, david laight wrote:
> ...   
> > >> 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.
> 
> You've misread the quoting...
> The 'data8' case that was only half there is a memcpy().
> 
> > 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()
> 
> true
> 
> > 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.
> 
> There is a function that does a loop byteswap of a buffer - no reason
> to re-invent it.

I assumed there must be something, but I did not see it. Which one?

> But I doubt it is always (if ever) faster to do a copy and then byteswap.
> The loop control and extra memory accesses kill performance.

Yes, you are probably right.

> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> generate optimal code - For LE I think it is something like:
> 	low = *(addr & ~3);
> 	high = *((addr + 3) & ~3);
> 	shift = (addr & 3) * 8;
> 	value = low << shift | high >> (32 - shift);
> Note that it is only 2 aligned memory reads - even for 64bit.

Ok, then maybe we should keep it simple like this patch:

[PATCH v2] 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 <deller@gmx.de>

diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
index 1fbe82f5021b..386da2023d50 100644
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -104,16 +104,20 @@ struct aa_dfa {
 	struct table_header *tables[YYTD_ID_TSIZE];
 };
 
-#define byte_to_byte(X) (X)
+#define byte_to_byte(X) (*(X))
 
 #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
 	do { \
 		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)); \
+		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
+			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
+		else /* copy & convert convert from big-endian */ \
+			for (__i = 0; __i < LEN; __i++) { \
+				__t[__i] = NTOHX(&__b[__i]); \
+			} \
 	} while (0)
 
 static inline size_t table_size(size_t len, size_t el_size)
diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index c5a91600842a..13e2f6873329 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"
@@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
 				     u8, u8, byte_to_byte);
 		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

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

[John Johansen]

On 11/26/25 07:12, Helge Deller wrote:
> * david laight <david.laight@runbox.com>:
>> On Wed, 26 Nov 2025 12:03:03 +0100
>> Helge Deller <deller@gmx.de> wrote:
>>
>>> On 11/26/25 11:44, david laight wrote:
>> ...
>>>>> 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.
>>
>> You've misread the quoting...
>> The 'data8' case that was only half there is a memcpy().
>>
>>> 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()
>>
>> true
>>
>>> 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.
>>
>> There is a function that does a loop byteswap of a buffer - no reason
>> to re-invent it.
> 
> I assumed there must be something, but I did not see it. Which one?
> 
>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>> The loop control and extra memory accesses kill performance.
> 
> Yes, you are probably right.
> 
>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>> generate optimal code - For LE I think it is something like:
>> 	low = *(addr & ~3);
>> 	high = *((addr + 3) & ~3);
>> 	shift = (addr & 3) * 8;
>> 	value = low << shift | high >> (32 - shift);
>> Note that it is only 2 aligned memory reads - even for 64bit.
> 
> Ok, then maybe we should keep it simple like this patch:
> 
> [PATCH v2] 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 <deller@gmx.de>
> 
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..386da2023d50 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,20 @@ struct aa_dfa {
>   	struct table_header *tables[YYTD_ID_TSIZE];
>   };
>   
> -#define byte_to_byte(X) (X)
> +#define byte_to_byte(X) (*(X))
>   
>   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>   	do { \
>   		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)); \
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> +		else /* copy & convert convert from big-endian */ \
> +			for (__i = 0; __i < LEN; __i++) { \
> +				__t[__i] = NTOHX(&__b[__i]); \
> +			} \
>   	} while (0)
>   
>   static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..13e2f6873329 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"
> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>   				     u8, u8, byte_to_byte);
>   		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

I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
ie.

diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index 26e82ba879d44..389202560675c 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -67,8 +67,7 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
  		table->td_flags = th.td_flags;
  		table->td_lolen = th.td_lolen;
  		if (th.td_flags == YYTD_DATA8)
-			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
-				     u8, u8, byte_to_byte);
+			memcp(table->td_data, blob, th.td_lolen);
  		else if (th.td_flags == YYTD_DATA16)
  			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
  				     u16, __be16, be16_to_cpu);

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

[Helge Deller]

* John Johansen <john.johansen@canonical.com>:
> On 11/26/25 07:12, Helge Deller wrote:
> > * david laight <david.laight@runbox.com>:
> > > On Wed, 26 Nov 2025 12:03:03 +0100
> > > Helge Deller <deller@gmx.de> wrote:
> > > 
> > > > On 11/26/25 11:44, david laight wrote:
> > > ...
> > > > > > 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.
> > > 
> > > You've misread the quoting...
> > > The 'data8' case that was only half there is a memcpy().
> > > 
> > > > 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()
> > > 
> > > true
> > > 
> > > > 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.
> > > 
> > > There is a function that does a loop byteswap of a buffer - no reason
> > > to re-invent it.
> > 
> > I assumed there must be something, but I did not see it. Which one?
> > 
> > > But I doubt it is always (if ever) faster to do a copy and then byteswap.
> > > The loop control and extra memory accesses kill performance.
> > 
> > Yes, you are probably right.
> > 
> > > Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> > > generate optimal code - For LE I think it is something like:
> > > 	low = *(addr & ~3);
> > > 	high = *((addr + 3) & ~3);
> > > 	shift = (addr & 3) * 8;
> > > 	value = low << shift | high >> (32 - shift);
> > > Note that it is only 2 aligned memory reads - even for 64bit.
> > 
> > Ok, then maybe we should keep it simple like this patch:
> > 
> > [PATCH v2] 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 <deller@gmx.de>
> > 
> > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > index 1fbe82f5021b..386da2023d50 100644
> > --- a/security/apparmor/include/match.h
> > +++ b/security/apparmor/include/match.h
> > @@ -104,16 +104,20 @@ struct aa_dfa {
> >   	struct table_header *tables[YYTD_ID_TSIZE];
> >   };
> > -#define byte_to_byte(X) (X)
> > +#define byte_to_byte(X) (*(X))
> >   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
> >   	do { \
> >   		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)); \
> > +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
> > +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> > +		else /* copy & convert convert from big-endian */ \
> > +			for (__i = 0; __i < LEN; __i++) { \
> > +				__t[__i] = NTOHX(&__b[__i]); \
> > +			} \
> >   	} while (0)
> >   static inline size_t table_size(size_t len, size_t el_size)
> > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > index c5a91600842a..13e2f6873329 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"
> > @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> >   				     u8, u8, byte_to_byte);
> >   		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
> 
> I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
> ie.
> 
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 26e82ba879d44..389202560675c 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -67,8 +67,7 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  		table->td_flags = th.td_flags;
>  		table->td_lolen = th.td_lolen;
>  		if (th.td_flags == YYTD_DATA8)
> -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u8, u8, byte_to_byte);
> +			memcp(table->td_data, blob, th.td_lolen);

True.
Then byte_to_byte() can go away in match.h as well.
So, here is a (untested) v3:


[PATCH v3] 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 <deller@gmx.de>

diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
index 1fbe82f5021b..19e72b3e8f49 100644
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -104,16 +104,18 @@ struct aa_dfa {
 	struct table_header *tables[YYTD_ID_TSIZE];
 };
 
-#define byte_to_byte(X) (X)
-
 #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
 	do { \
 		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)); \
+		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
+			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
+		else /* copy & convert convert from big-endian */ \
+			for (__i = 0; __i < LEN; __i++) { \
+				__t[__i] = NTOHX(&__b[__i]); \
+			} \
 	} while (0)
 
 static inline size_t table_size(size_t len, size_t el_size)
diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index c5a91600842a..1e32c8ba14ae 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"
@@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
 		table->td_flags = th.td_flags;
 		table->td_lolen = th.td_lolen;
 		if (th.td_flags == YYTD_DATA8)
-			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
-				     u8, u8, byte_to_byte);
+			memcpy(table->td_data, blob, th.td_lolen);
 		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

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

[John Johansen]

On 11/26/25 12:15, Helge Deller wrote:
> * John Johansen <john.johansen@canonical.com>:
>> On 11/26/25 07:12, Helge Deller wrote:
>>> * david laight <david.laight@runbox.com>:
>>>> On Wed, 26 Nov 2025 12:03:03 +0100
>>>> Helge Deller <deller@gmx.de> wrote:
>>>>
>>>>> On 11/26/25 11:44, david laight wrote:
>>>> ...
>>>>>>> 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.
>>>>
>>>> You've misread the quoting...
>>>> The 'data8' case that was only half there is a memcpy().
>>>>
>>>>> 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()
>>>>
>>>> true
>>>>
>>>>> 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.
>>>>
>>>> There is a function that does a loop byteswap of a buffer - no reason
>>>> to re-invent it.
>>>
>>> I assumed there must be something, but I did not see it. Which one?
>>>
>>>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>>>> The loop control and extra memory accesses kill performance.
>>>
>>> Yes, you are probably right.
>>>
>>>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>>>> generate optimal code - For LE I think it is something like:
>>>> 	low = *(addr & ~3);
>>>> 	high = *((addr + 3) & ~3);
>>>> 	shift = (addr & 3) * 8;
>>>> 	value = low << shift | high >> (32 - shift);
>>>> Note that it is only 2 aligned memory reads - even for 64bit.
>>>
>>> Ok, then maybe we should keep it simple like this patch:
>>>
>>> [PATCH v2] 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 <deller@gmx.de>
>>>
>>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>>> index 1fbe82f5021b..386da2023d50 100644
>>> --- a/security/apparmor/include/match.h
>>> +++ b/security/apparmor/include/match.h
>>> @@ -104,16 +104,20 @@ struct aa_dfa {
>>>    	struct table_header *tables[YYTD_ID_TSIZE];
>>>    };
>>> -#define byte_to_byte(X) (X)
>>> +#define byte_to_byte(X) (*(X))
>>>    #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>>>    	do { \
>>>    		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)); \
>>> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
>>> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
>>> +		else /* copy & convert convert from big-endian */ \
>>> +			for (__i = 0; __i < LEN; __i++) { \
>>> +				__t[__i] = NTOHX(&__b[__i]); \
>>> +			} \
>>>    	} while (0)
>>>    static inline size_t table_size(size_t len, size_t el_size)
>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>> index c5a91600842a..13e2f6873329 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"
>>> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>>    				     u8, u8, byte_to_byte);
>>>    		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
>>
>> I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
>> ie.
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index 26e82ba879d44..389202560675c 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -67,8 +67,7 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>   		table->td_flags = th.td_flags;
>>   		table->td_lolen = th.td_lolen;
>>   		if (th.td_flags == YYTD_DATA8)
>> -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
>> -				     u8, u8, byte_to_byte);
>> +			memcp(table->td_data, blob, th.td_lolen);
> 
> True.
> Then byte_to_byte() can go away in match.h as well.
> So, here is a (untested) v3:
> 
and lightly tested now

I will pull it into my tree

> 
> [PATCH v3] 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 <deller@gmx.de>
> 
Acked-by: John Johansen <john.johansen@canonical.com>

> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..19e72b3e8f49 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,18 @@ struct aa_dfa {
>   	struct table_header *tables[YYTD_ID_TSIZE];
>   };
>   
> -#define byte_to_byte(X) (X)
> -
>   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>   	do { \
>   		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)); \
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> +		else /* copy & convert convert from big-endian */ \
> +			for (__i = 0; __i < LEN; __i++) { \
> +				__t[__i] = NTOHX(&__b[__i]); \
> +			} \
>   	} while (0)
>   
>   static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..1e32c8ba14ae 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"
> @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>   		table->td_flags = th.td_flags;
>   		table->td_lolen = th.td_lolen;
>   		if (th.td_flags == YYTD_DATA8)
> -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u8, u8, byte_to_byte);
> +			memcpy(table->td_data, blob, th.td_lolen);
>   		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

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

[John Paul Adrian Glaubitz]

Hi Helge,

On Wed, 2025-11-26 at 21:15 +0100, Helge Deller wrote:
> So, here is a (untested) v3:
> 
> 
> [PATCH v3] 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 <deller@gmx.de>
> 
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..19e72b3e8f49 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,18 @@ struct aa_dfa {
>  	struct table_header *tables[YYTD_ID_TSIZE];
>  };
>  
> -#define byte_to_byte(X) (X)
> -
>  #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>  	do { \
>  		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)); \
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> +		else /* copy & convert convert from big-endian */ \
> +			for (__i = 0; __i < LEN; __i++) { \
> +				__t[__i] = NTOHX(&__b[__i]); \
> +			} \
>  	} while (0)
>  
>  static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..1e32c8ba14ae 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"
> @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  		table->td_flags = th.td_flags;
>  		table->td_lolen = th.td_lolen;
>  		if (th.td_flags == YYTD_DATA8)
> -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u8, u8, byte_to_byte);
> +			memcpy(table->td_data, blob, th.td_lolen);
>  		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

This one does not apply:

glaubitz@node54:/data/home/glaubitz/linux> git am ../20251125_app_armor_unalign_2nd.mbx
Applying: apparmor unaligned memory fixes
error: patch failed: security/apparmor/match.c:15
error: security/apparmor/match.c: patch does not apply
Patch failed at 0001 apparmor unaligned memory fixes
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
glaubitz@node54:/data/home/glaubitz/linux>

Adrian

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

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

[John Paul Adrian Glaubitz]

Hi Helge,


On Thu, 2025-11-27 at 10:25 +0100, John Paul Adrian Glaubitz wrote:
> Hi Helge,
> 
> On Wed, 2025-11-26 at 21:15 +0100, Helge Deller wrote:
> > So, here is a (untested) v3:
> > 
> > 
> > [PATCH v3] 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 <deller@gmx.de>
> > 
> > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > index 1fbe82f5021b..19e72b3e8f49 100644
> > --- a/security/apparmor/include/match.h
> > +++ b/security/apparmor/include/match.h
> > @@ -104,16 +104,18 @@ struct aa_dfa {
> >  	struct table_header *tables[YYTD_ID_TSIZE];
> >  };
> >  
> > -#define byte_to_byte(X) (X)
> > -
> >  #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
> >  	do { \
> >  		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)); \
> > +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> > +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> > +		else /* copy & convert convert from big-endian */ \
> > +			for (__i = 0; __i < LEN; __i++) { \
> > +				__t[__i] = NTOHX(&__b[__i]); \
> > +			} \
> >  	} while (0)
> >  
> >  static inline size_t table_size(size_t len, size_t el_size)
> > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > index c5a91600842a..1e32c8ba14ae 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"
> > @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> >  		table->td_flags = th.td_flags;
> >  		table->td_lolen = th.td_lolen;
> >  		if (th.td_flags == YYTD_DATA8)
> > -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> > -				     u8, u8, byte_to_byte);
> > +			memcpy(table->td_data, blob, th.td_lolen);
> >  		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
> 
> This one does not apply:
> 
> glaubitz@node54:/data/home/glaubitz/linux> git am ../20251125_app_armor_unalign_2nd.mbx
> Applying: apparmor unaligned memory fixes
> error: patch failed: security/apparmor/match.c:15
> error: security/apparmor/match.c: patch does not apply
> Patch failed at 0001 apparmor unaligned memory fixes
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> hint: When you have resolved this problem, run "git am --continue".
> hint: If you prefer to skip this patch, run "git am --skip" instead.
> hint: To restore the original branch and stop patching, run "git am --abort".
> hint: Disable this message with "git config set advice.mergeConflict false"
> glaubitz@node54:/data/home/glaubitz/linux>

The patch alone applies, i.e without your previous patch, but it does not fix the problem:

[   73.961582] Kernel unaligned access at TPC[8dabdc] aa_dfa_unpack+0x3c/0x6e0
[   74.053195] Kernel unaligned access at TPC[8dabec] aa_dfa_unpack+0x4c/0x6e0
[   74.144814] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[   74.237538] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[   74.330296] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0

Adrian

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

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

[John Paul Adrian Glaubitz]

Hi Helge,

On Wed, 2025-11-26 at 21:15 +0100, Helge Deller wrote:
> * John Johansen <john.johansen@canonical.com>:
> > On 11/26/25 07:12, Helge Deller wrote:
> > > * david laight <david.laight@runbox.com>:
> > > > On Wed, 26 Nov 2025 12:03:03 +0100
> > > > Helge Deller <deller@gmx.de> wrote:
> > > > 
> > > > > On 11/26/25 11:44, david laight wrote:
> > > > ...
> > > > > > > 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.
> > > > 
> > > > You've misread the quoting...
> > > > The 'data8' case that was only half there is a memcpy().
> > > > 
> > > > > 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()
> > > > 
> > > > true
> > > > 
> > > > > 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.
> > > > 
> > > > There is a function that does a loop byteswap of a buffer - no reason
> > > > to re-invent it.
> > > 
> > > I assumed there must be something, but I did not see it. Which one?
> > > 
> > > > But I doubt it is always (if ever) faster to do a copy and then byteswap.
> > > > The loop control and extra memory accesses kill performance.
> > > 
> > > Yes, you are probably right.
> > > 
> > > > Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> > > > generate optimal code - For LE I think it is something like:
> > > > 	low = *(addr & ~3);
> > > > 	high = *((addr + 3) & ~3);
> > > > 	shift = (addr & 3) * 8;
> > > > 	value = low << shift | high >> (32 - shift);
> > > > Note that it is only 2 aligned memory reads - even for 64bit.
> > > 
> > > Ok, then maybe we should keep it simple like this patch:
> > > 
> > > [PATCH v2] 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 <deller@gmx.de>
> > > 
> > > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > > index 1fbe82f5021b..386da2023d50 100644
> > > --- a/security/apparmor/include/match.h
> > > +++ b/security/apparmor/include/match.h
> > > @@ -104,16 +104,20 @@ struct aa_dfa {
> > >   	struct table_header *tables[YYTD_ID_TSIZE];
> > >   };
> > > -#define byte_to_byte(X) (X)
> > > +#define byte_to_byte(X) (*(X))
> > >   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
> > >   	do { \
> > >   		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)); \
> > > +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
> > > +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> > > +		else /* copy & convert convert from big-endian */ \
> > > +			for (__i = 0; __i < LEN; __i++) { \
> > > +				__t[__i] = NTOHX(&__b[__i]); \
> > > +			} \
> > >   	} while (0)
> > >   static inline size_t table_size(size_t len, size_t el_size)
> > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > > index c5a91600842a..13e2f6873329 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"
> > > @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > >   				     u8, u8, byte_to_byte);
> > >   		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
> > 
> > I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
> > ie.
> > 
> > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > index 26e82ba879d44..389202560675c 100644
> > --- a/security/apparmor/match.c
> > +++ b/security/apparmor/match.c
> > @@ -67,8 +67,7 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> >  		table->td_flags = th.td_flags;
> >  		table->td_lolen = th.td_lolen;
> >  		if (th.td_flags == YYTD_DATA8)
> > -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> > -				     u8, u8, byte_to_byte);
> > +			memcp(table->td_data, blob, th.td_lolen);
> 
> True.
> Then byte_to_byte() can go away in match.h as well.
> So, here is a (untested) v3:
> 
> 
> [PATCH v3] 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 <deller@gmx.de>
> 
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..19e72b3e8f49 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,18 @@ struct aa_dfa {
>  	struct table_header *tables[YYTD_ID_TSIZE];
>  };
>  
> -#define byte_to_byte(X) (X)
> -
>  #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>  	do { \
>  		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)); \
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> +		else /* copy & convert convert from big-endian */ \
> +			for (__i = 0; __i < LEN; __i++) { \
> +				__t[__i] = NTOHX(&__b[__i]); \
> +			} \
>  	} while (0)
>  
>  static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..1e32c8ba14ae 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"
> @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  		table->td_flags = th.td_flags;
>  		table->td_lolen = th.td_lolen;
>  		if (th.td_flags == YYTD_DATA8)
> -			UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> -				     u8, u8, byte_to_byte);
> +			memcpy(table->td_data, blob, th.td_lolen);
>  		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

I have applied both patches, the latter required minimal rework, and I confirm the issue is gone.

Could you post a cleaned up series with both patches so I can add my Tested-by?

Thanks,
Adrian

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

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

[david laight]

On Wed, 26 Nov 2025 16:12:23 +0100
Helge Deller <deller@kernel.org> wrote:

> * david laight <david.laight@runbox.com>:
> > On Wed, 26 Nov 2025 12:03:03 +0100
> > Helge Deller <deller@gmx.de> wrote:
> >   
> > > On 11/26/25 11:44, david laight wrote:  
> > ...     
> > > >> 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.  
> > 
> > You've misread the quoting...
> > The 'data8' case that was only half there is a memcpy().
> >   
> > > 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()  
> > 
> > true
> >   
> > > 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.  
> > 
> > There is a function that does a loop byteswap of a buffer - no reason
> > to re-invent it.  
> 
> I assumed there must be something, but I did not see it. Which one?

I can't find it either - just some functions to do an in-place swap.
(Which aren't usually a good idea)

> 
> > But I doubt it is always (if ever) faster to do a copy and then byteswap.
> > The loop control and extra memory accesses kill performance.  
> 
> Yes, you are probably right.
> 
> > Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> > generate optimal code - For LE I think it is something like:
> > 	low = *(addr & ~3);
> > 	high = *((addr + 3) & ~3);
> > 	shift = (addr & 3) * 8;
> > 	value = low << shift | high >> (32 - shift);
> > Note that it is only 2 aligned memory reads - even for 64bit.  
> 
> Ok, then maybe we should keep it simple like this patch:
> 
> [PATCH v2] 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.

Not sure that reads right.
'Allow for misaligned data from userspace when byteswapping source blob' ?

> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..386da2023d50 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,20 @@ struct aa_dfa {
>  	struct table_header *tables[YYTD_ID_TSIZE];
>  };
>  
> -#define byte_to_byte(X) (X)
> +#define byte_to_byte(X) (*(X))

It's a bit of a shame you need something for the above...

	David


>  
>  #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>  	do { \
>  		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)); \
> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> +		else /* copy & convert convert from big-endian */ \
> +			for (__i = 0; __i < LEN; __i++) { \
> +				__t[__i] = NTOHX(&__b[__i]); \
> +			} \
>  	} while (0)
>  
>  static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..13e2f6873329 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"
> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  				     u8, u8, byte_to_byte);
>  		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

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

[John Johansen]

On 11/26/25 13:23, david laight wrote:
> On Wed, 26 Nov 2025 16:12:23 +0100
> Helge Deller <deller@kernel.org> wrote:
> 
>> * david laight <david.laight@runbox.com>:
>>> On Wed, 26 Nov 2025 12:03:03 +0100
>>> Helge Deller <deller@gmx.de> wrote:
>>>    
>>>> On 11/26/25 11:44, david laight wrote:
>>> ...
>>>>>> 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.
>>>
>>> You've misread the quoting...
>>> The 'data8' case that was only half there is a memcpy().
>>>    
>>>> 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()
>>>
>>> true
>>>    
>>>> 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.
>>>
>>> There is a function that does a loop byteswap of a buffer - no reason
>>> to re-invent it.
>>
>> I assumed there must be something, but I did not see it. Which one?
> 
> I can't find it either - just some functions to do an in-place swap.
> (Which aren't usually a good idea)
> 
>>
>>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>>> The loop control and extra memory accesses kill performance.
>>
>> Yes, you are probably right.
>>
>>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>>> generate optimal code - For LE I think it is something like:
>>> 	low = *(addr & ~3);
>>> 	high = *((addr + 3) & ~3);
>>> 	shift = (addr & 3) * 8;
>>> 	value = low << shift | high >> (32 - shift);
>>> Note that it is only 2 aligned memory reads - even for 64bit.
>>
>> Ok, then maybe we should keep it simple like this patch:
>>
>> [PATCH v2] 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.
> 
> Not sure that reads right.
> 'Allow for misaligned data from userspace when byteswapping source blob' ?
> 
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b..386da2023d50 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -104,16 +104,20 @@ struct aa_dfa {
>>   	struct table_header *tables[YYTD_ID_TSIZE];
>>   };
>>   
>> -#define byte_to_byte(X) (X)
>> +#define byte_to_byte(X) (*(X))
> 
> It's a bit of a shame you need something for the above...
> 
We got rid of that in the last patch by just replacing the call to
UNPACK_ARRAY for bytes with just a call to memcpy.

> 	David
> 
> 
>>   
>>   #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX)	\
>>   	do { \
>>   		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)); \
>> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
>> +			memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
>> +		else /* copy & convert convert from big-endian */ \
>> +			for (__i = 0; __i < LEN; __i++) { \
>> +				__t[__i] = NTOHX(&__b[__i]); \
>> +			} \
>>   	} while (0)
>>   
>>   static inline size_t table_size(size_t len, size_t el_size)
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index c5a91600842a..13e2f6873329 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"
>> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>   				     u8, u8, byte_to_byte);
>>   		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
> 

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

[John Johansen]

On 11/26/25 02:44, david laight wrote:
> On Wed, 26 Nov 2025 01:11:45 -0800
> John Johansen <john.johansen@canonical.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@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.
>>
>> 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@canonical.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@canonical.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() ?
> 
yeah for the byte case it is and we should just replace that case of UNPACK_ARRAY

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

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

[John Paul Adrian Glaubitz]

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?

Yes, I'll test that one.

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

Adrian

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

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

[John Paul Adrian Glaubitz]

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