* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: david laight @ 2025-11-26 10:44 UTC (permalink / raw)
To: John Johansen
Cc: Helge Deller, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <d35010b3-7d07-488c-b5a4-a13380d0ef7c@canonical.com>
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
>
>
>
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 9:11 UTC (permalink / raw)
To: Helge Deller, Helge Deller, John Paul Adrian Glaubitz
Cc: linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <c192140a-0575-41e9-8895-6c8257ce4682@gmx.de>
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
^ permalink raw reply related
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-26 7:52 UTC (permalink / raw)
To: Helge Deller, John Johansen
Cc: Helge Deller, linux-kernel, apparmor, linux-security-module,
linux-parisc
In-Reply-To: <aSXHCyH_rS-c5BgP@p100>
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
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-26 7:27 UTC (permalink / raw)
To: Helge Deller, John Johansen
Cc: Helge Deller, linux-kernel, apparmor, linux-security-module,
linux-parisc
In-Reply-To: <aSXHCyH_rS-c5BgP@p100>
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
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-25 21:13 UTC (permalink / raw)
To: John Johansen, Helge Deller, John Paul Adrian Glaubitz
Cc: linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <e88c32c2-fb18-4f3e-9ec2-a749695aaf0a@canonical.com>
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
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-25 19:20 UTC (permalink / raw)
To: Helge Deller, John Paul Adrian Glaubitz
Cc: Helge Deller, linux-kernel, apparmor, linux-security-module,
linux-parisc
In-Reply-To: <aSXHCyH_rS-c5BgP@p100>
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;
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Bernd Edlinger @ 2025-11-25 16:19 UTC (permalink / raw)
To: Eric W. Biederman, Oleg Nesterov
Cc: Alexander Viro, Alexey Dobriyan, Kees Cook, Andy Lutomirski,
Will Drewry, Christian Brauner, Andrew Morton, Michal Hocko,
Serge Hallyn, James Morris, Randy Dunlap, Suren Baghdasaryan,
Yafang Shao, Helge Deller, Adrian Reber, Thomas Gleixner,
Jens Axboe, Alexei Starovoitov, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest, linux-mm,
linux-security-module, tiozhang, Luis Chamberlain,
Paulo Alcantara (SUSE), Sergey Senozhatsky, Frederic Weisbecker,
YueHaibing, Paul Moore, Aleksa Sarai, Stefan Roesch, Chao Yu,
xu xin, Jeff Layton, Jan Kara, David Hildenbrand, Dave Chinner,
Shuah Khan, Elena Reshetova, David Windsor, Mateusz Guzik,
Ard Biesheuvel, Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <87tsykuyf7.fsf@email.froward.int.ebiederm.org>
On 11/24/25 00:22, Eric W. Biederman wrote:
> Oleg Nesterov <oleg@redhat.com> writes:
>
>> Eric,
>>
>> sorry for delay, I am on PTO, didn't read emails this week...
>>
>> On 11/20, Eric W. Biederman wrote:
>>>
>>> Instead of computing the new cred before we pass the point of no
>>> return compute the new cred just before we use it.
>>>
>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>
>>> I am not certain why we wanted to compute the cred for the new
>>> executable so early. Perhaps I missed something but I did not see any
>>> common errors being signaled. So I don't think we loose anything by
>>> computing the new cred later.
>>>
>>> We gain a lot.
>>
>> Yes. I LIKE your approach after a quick glance. And I swear, I thought about
>> it too ;)
>>
>> But is it correct? I don't know. I'll try to actually read your patch next
>> week (I am on PTO untill the end of November), but I am not sure I can
>> provide a valuable feedback.
>>
>> One "obvious" problem is that, after this patch, the execing process can crash
>> in a case when currently exec() returns an error...
>
> Yes.
>
> I have been testing and looking at it, and I have found a few issues,
> and I am trying to see if I can resolve them.
>
> The good news is that with the advent of AT_EXECVE_CHECK we have a
> really clear API boundary between errors that must be diagnosed
> and errors of happenstance like running out of memory.
>
> The bad news is that the implementation of AT_EXECVE_CHECK seems to been
> rather hackish especially with respect to security_bprm_creds_for_exec.
>
> What I am hoping for is to get the 3 causes of errors of brpm->unsafe
> ( LSM_UNSAFE_SHARE, LSM_UNSAFE_PTRACE, and LSM_UNSAFE_NO_NEW_PRIVS )
> handled cleanly outside of the cred_guard_mutex, and simply
> retested when it is time to build the credentials of the new process.
>
> In practice that should get the same failures modes as we have now
> but it would get SIGSEGV in rare instances where things changed
> during exec. That feels acceptable.
>
>
>
> I thought of one other approach that might be enough to put the issue to
> bed if cleaning up exec is too much work. We could have ptrace_attach
> use a trylock and fail when it doesn't succeed. That would solve the
> worst of the symptoms.
>
> I think this would be a complete patch:
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 75a84efad40f..5dd2144e5789 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -444,7 +444,7 @@ static int ptrace_attach(struct task_struct *task, long request,
> * SUID, SGID and LSM creds get determined differently
> * under ptrace.
> */
> - scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> + scoped_cond_guard (mutex_try, return -EAGAIN,
> &task->signal->cred_guard_mutex) {
>
> scoped_guard (task_lock, task) {
This is very similar to my initial attempt of fixing the problem, as you
can see the test expectaion of the currently failing test in vmattach.c
is that ptrace(PTRACE_ATTACH, pid, 0L, 0L) returns -1 with errno = EAGAIN.
The disadvantage of that approach was, that it is a user-visible API-change,
but also that the debugger does not know when to retry the PTRACE_ATTACH,
in worst case it will go into an endless loop not knowing that a waitpid
and/or PTRACE_CONT is necessary to unblock the traced process.
But The main reason why I preferred the overlapping lifetime of the current
and the new credentials, is that the tracee can escape the PTRACE_ATTACH
if it is very short-lived, and indeed I had to cheat a little to make the
test case function TEST(attach) pass reliably:
The traced process does execlp("sleep", "sleep", "2", NULL);
If it did execlp("true", "true", NULL); like the first test case, it would
have failed randomly, because the debugger could not attach quickly enoguh,
and IMHO the expectaion of the debugger is probably to be able to stop the
new process at the first instruction after the execve.
Bernd.
^ permalink raw reply
* Re: [PATCH v2] security: Rename functions and add namespace mapping tests
From: Serge E. Hallyn @ 2025-11-25 15:15 UTC (permalink / raw)
To: Ryan Foster; +Cc: serge, paul, linux-security-module, linux-kernel
In-Reply-To: <20251121174826.190381-1-foster.ryan.r@gmail.com>
On Fri, Nov 21, 2025 at 09:48:26AM -0800, Ryan Foster wrote:
> Rename rootid_owns_currentns() to uid_owns_currentns() and
> rootid_owns_userns() to uid_owns_ns() for clarity, as the function checks
> any UID, not just root. Update all call sites accordingly.
>
> Add tests that create actual user namespaces with different UID mappings
> to verify namespace traversal logic. The tests create namespaces where
> uid 0 maps to different kuids (e.g., kuid 1000, 2000) and verify that
> uid_owns_ns() correctly identifies ownership based on the namespace
> hierarchy traversal.
>
> This addresses feedback to use clearer function naming and test actual
> namespace functionality with real user namespace creation and mappings,
> rather than just basic input validation.
Hi Ryan,
did you see https://lore.kernel.org/all/aR0JrOvDxDKZPELd@mail.hallyn.com ?
That is now in linux-next, and should be merged into 6.19 when that window
opens. So please base your patch on that (so you can drop your uid_owns_ns()
renames).
I haven't looked closely at the tests, but at a cursory glance this is what
I had in mind, thanks! I'll look more closely when you send next version.
> ---
> security/commoncap.c | 26 ++--
> security/commoncap_test.c | 286 ++++++++++++++++++++++++++++++++------
> 2 files changed, 254 insertions(+), 58 deletions(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 15d8147a34c4..cca291df9551 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -359,16 +359,16 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry)
> }
>
> #ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
> -bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
> -bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
> +bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
> +bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
> #else
> -static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
> +static bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
> #endif
> {
> struct user_namespace *iter;
>
> for (iter = ns;; iter = iter->parent) {
> - if (from_kuid(iter, kroot) == 0)
> + if (from_kuid(iter, kuid) == 0)
> return true;
> if (iter == &init_user_ns)
> break;
> @@ -378,19 +378,19 @@ static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
> }
>
> #ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
> -bool rootid_owns_currentns(vfsuid_t rootvfsuid);
> -bool rootid_owns_currentns(vfsuid_t rootvfsuid)
> +bool uid_owns_currentns(vfsuid_t vfsuid);
> +bool uid_owns_currentns(vfsuid_t vfsuid)
> #else
> -static bool rootid_owns_currentns(vfsuid_t rootvfsuid)
> +static bool uid_owns_currentns(vfsuid_t vfsuid)
> #endif
> {
> - kuid_t kroot;
> + kuid_t kuid;
>
> - if (!vfsuid_valid(rootvfsuid))
> + if (!vfsuid_valid(vfsuid))
> return false;
>
> - kroot = vfsuid_into_kuid(rootvfsuid);
> - return rootid_owns_userns(current_user_ns(), kroot);
> + kuid = vfsuid_into_kuid(vfsuid);
> + return uid_owns_ns(current_user_ns(), kuid);
> }
>
> static __u32 sansflags(__u32 m)
> @@ -497,7 +497,7 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
> goto out_free;
> }
>
> - if (!rootid_owns_currentns(vfsroot)) {
> + if (!uid_owns_currentns(vfsroot)) {
> size = -EOVERFLOW;
> goto out_free;
> }
> @@ -738,7 +738,7 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap,
> /* Limit the caps to the mounter of the filesystem
> * or the more limited uid specified in the xattr.
> */
> - if (!rootid_owns_currentns(rootvfsuid))
> + if (!uid_owns_currentns(rootvfsuid))
> return -ENODATA;
>
> cpu_caps->permitted.val = le32_to_cpu(caps->data[0].permitted);
> diff --git a/security/commoncap_test.c b/security/commoncap_test.c
> index 962aa899455d..7f066dc0df5d 100644
> --- a/security/commoncap_test.c
> +++ b/security/commoncap_test.c
> @@ -10,6 +10,8 @@
> #include <linux/user_namespace.h>
> #include <linux/uidgid.h>
> #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/refcount.h>
>
> /* Forward declare types and functions we need from mnt_idmapping.h
> * We avoid including the full header because it contains inline functions
> @@ -50,38 +52,38 @@ static inline kuid_t vfsuid_into_kuid(vfsuid_t vfsuid)
> #ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
>
> /* Forward declarations - functions are exported when KUNIT_TEST is enabled */
> -extern bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
> -extern bool rootid_owns_currentns(vfsuid_t rootvfsuid);
> +extern bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
> +extern bool uid_owns_currentns(vfsuid_t vfsuid);
>
> /**
> - * test_rootid_owns_currentns_init_ns - Test rootid_owns_currentns with init ns
> + * test_uid_owns_currentns_init_ns - Test uid_owns_currentns with init ns
> *
> - * Verifies that a root ID in the init namespace correctly owns the current
> + * Verifies that UID 0 in the init namespace correctly owns the current
> * namespace when running in init_user_ns.
> *
> * @test: KUnit test context
> */
> -static void test_rootid_owns_currentns_init_ns(struct kunit *test)
> +static void test_uid_owns_currentns_init_ns(struct kunit *test)
> {
> - vfsuid_t root_vfsuid;
> - kuid_t root_kuid;
> + vfsuid_t vfsuid;
> + kuid_t kuid;
>
> - /* Create a root UID in init namespace */
> - root_kuid = KUIDT_INIT(0);
> - root_vfsuid = VFSUIDT_INIT(root_kuid);
> + /* Create UID 0 in init namespace */
> + kuid = KUIDT_INIT(0);
> + vfsuid = VFSUIDT_INIT(kuid);
>
> - /* In init namespace, root should own current namespace */
> - KUNIT_EXPECT_TRUE(test, rootid_owns_currentns(root_vfsuid));
> + /* In init namespace, UID 0 should own current namespace */
> + KUNIT_EXPECT_TRUE(test, uid_owns_currentns(vfsuid));
> }
>
> /**
> - * test_rootid_owns_currentns_invalid - Test rootid_owns_currentns with invalid vfsuid
> + * test_uid_owns_currentns_invalid - Test uid_owns_currentns with invalid vfsuid
> *
> * Verifies that an invalid vfsuid correctly returns false.
> *
> * @test: KUnit test context
> */
> -static void test_rootid_owns_currentns_invalid(struct kunit *test)
> +static void test_uid_owns_currentns_invalid(struct kunit *test)
> {
> vfsuid_t invalid_vfsuid;
>
> @@ -89,74 +91,268 @@ static void test_rootid_owns_currentns_invalid(struct kunit *test)
> invalid_vfsuid = INVALID_VFSUID;
>
> /* Invalid vfsuid should return false */
> - KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(invalid_vfsuid));
> + KUNIT_EXPECT_FALSE(test, uid_owns_currentns(invalid_vfsuid));
> }
>
> /**
> - * test_rootid_owns_currentns_nonroot - Test rootid_owns_currentns with non-root UID
> + * test_uid_owns_currentns_nonzero - Test uid_owns_currentns with non-zero UID
> *
> - * Verifies that a non-root UID correctly returns false.
> + * Verifies that a non-zero UID correctly returns false.
> *
> * @test: KUnit test context
> */
> -static void test_rootid_owns_currentns_nonroot(struct kunit *test)
> +static void test_uid_owns_currentns_nonzero(struct kunit *test)
> {
> - vfsuid_t nonroot_vfsuid;
> - kuid_t nonroot_kuid;
> + vfsuid_t vfsuid;
> + kuid_t kuid;
>
> - /* Create a non-root UID */
> - nonroot_kuid = KUIDT_INIT(1000);
> - nonroot_vfsuid = VFSUIDT_INIT(nonroot_kuid);
> + /* Create a non-zero UID */
> + kuid = KUIDT_INIT(1000);
> + vfsuid = VFSUIDT_INIT(kuid);
>
> - /* Non-root UID should return false */
> - KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(nonroot_vfsuid));
> + /* Non-zero UID should return false */
> + KUNIT_EXPECT_FALSE(test, uid_owns_currentns(vfsuid));
> }
>
> /**
> - * test_rootid_owns_userns_init_ns - Test rootid_owns_userns with init namespace
> + * test_uid_owns_ns_init_ns_uid0 - Test uid_owns_ns with init namespace and UID 0
> *
> - * Verifies that rootid_owns_userns correctly identifies root UID in init namespace.
> - * This tests the core namespace traversal logic.
> + * Verifies that uid_owns_ns correctly identifies UID 0 in init namespace.
> + * This tests the core namespace traversal logic. In init namespace, UID 0
> + * maps to itself, so it should own the namespace.
> *
> * @test: KUnit test context
> */
> -static void test_rootid_owns_userns_init_ns(struct kunit *test)
> +static void test_uid_owns_ns_init_ns_uid0(struct kunit *test)
> {
> - kuid_t root_kuid;
> + kuid_t kuid;
> struct user_namespace *init_ns;
>
> - root_kuid = KUIDT_INIT(0);
> + kuid = KUIDT_INIT(0);
> init_ns = &init_user_ns;
>
> - /* Root UID should own init namespace */
> - KUNIT_EXPECT_TRUE(test, rootid_owns_userns(init_ns, root_kuid));
> + /* UID 0 should own init namespace */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
> }
>
> /**
> - * test_rootid_owns_userns_nonroot - Test rootid_owns_userns with non-root UID
> + * test_uid_owns_ns_init_ns_nonzero - Test uid_owns_ns with init namespace and non-zero UID
> *
> - * Verifies that rootid_owns_userns correctly rejects non-root UIDs.
> + * Verifies that uid_owns_ns correctly rejects non-zero UIDs in init namespace.
> + * Only UID 0 should own a namespace.
> *
> * @test: KUnit test context
> */
> -static void test_rootid_owns_userns_nonroot(struct kunit *test)
> +static void test_uid_owns_ns_init_ns_nonzero(struct kunit *test)
> {
> - kuid_t nonroot_kuid;
> + kuid_t kuid;
> struct user_namespace *init_ns;
>
> - nonroot_kuid = KUIDT_INIT(1000);
> + kuid = KUIDT_INIT(1000);
> init_ns = &init_user_ns;
>
> - /* Non-root UID should not own namespace */
> - KUNIT_EXPECT_FALSE(test, rootid_owns_userns(init_ns, nonroot_kuid));
> + /* Non-zero UID should not own namespace */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
> +}
> +
> +/**
> + * test_uid_owns_ns_init_ns_various_uids - Test uid_owns_ns with various UIDs
> + *
> + * Verifies that uid_owns_ns correctly identifies only UID 0 as owning
> + * the namespace, regardless of the UID value tested.
> + *
> + * @test: KUnit test context
> + */
> +static void test_uid_owns_ns_init_ns_various_uids(struct kunit *test)
> +{
> + struct user_namespace *init_ns;
> + kuid_t kuid;
> +
> + init_ns = &init_user_ns;
> +
> + /* UID 0 should own the namespace */
> + kuid = KUIDT_INIT(0);
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
> +
> + /* Other UIDs should not own the namespace */
> + kuid = KUIDT_INIT(1);
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
> +
> + kuid = KUIDT_INIT(1000);
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
> +
> + kuid = KUIDT_INIT(65534);
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
> +}
> +
> +/**
> + * create_test_user_ns_with_mapping - Create a test user namespace with uid mapping
> + *
> + * Creates a minimal user namespace for testing where uid 0 in the namespace
> + * maps to the specified kuid in the parent namespace.
> + *
> + * The mapping semantics:
> + * - first: uid in this namespace (0)
> + * - lower_first: kuid in parent namespace (mapped_kuid)
> + * - count: range size (1)
> + *
> + * This means: from_kuid(ns, mapped_kuid) will return 0
> + * because map_id_up looks for kuid in [lower_first, lower_first+count)
> + * and returns first + (kuid - lower_first) = 0 + (mapped_kuid - mapped_kuid) = 0
> + *
> + * @test: KUnit test context
> + * @parent_ns: Parent user namespace
> + * @mapped_kuid: The kuid that uid 0 in the new namespace maps to
> + *
> + * Returns: The new user namespace, or NULL on failure
> + */
> +static struct user_namespace *create_test_user_ns_with_mapping(struct kunit *test,
> + struct user_namespace *parent_ns,
> + kuid_t mapped_kuid)
> +{
> + struct user_namespace *ns;
> + struct uid_gid_extent extent;
> +
> + /* Allocate a test namespace - use kzalloc to zero all fields */
> + ns = kunit_kzalloc(test, sizeof(*ns), GFP_KERNEL);
> + if (!ns)
> + return NULL;
> +
> + /* Initialize basic namespace structure fields */
> + ns->parent = parent_ns;
> + ns->level = parent_ns ? parent_ns->level + 1 : 0;
> + ns->owner = mapped_kuid;
> + ns->group = KGIDT_INIT(0);
> +
> + /* Initialize ns_common structure */
> + refcount_set(&ns->ns.__ns_ref, 1);
> +
> + /* Set up uid mapping: uid 0 in this namespace maps to mapped_kuid in parent
> + * Format: first (uid in ns) : lower_first (kuid in parent) : count
> + * So: uid 0 in ns -> kuid mapped_kuid in parent
> + * This means from_kuid(ns, mapped_kuid) returns 0
> + */
> + extent.first = 0; /* uid 0 in this namespace */
> + extent.lower_first = __kuid_val(mapped_kuid); /* maps to this kuid in parent */
> + extent.count = 1;
> +
> + ns->uid_map.extent[0] = extent;
> + ns->uid_map.nr_extents = 1;
> +
> + /* Set up gid mapping: gid 0 maps to gid 0 in parent (simplified) */
> + extent.first = 0;
> + extent.lower_first = 0;
> + extent.count = 1;
> +
> + ns->gid_map.extent[0] = extent;
> + ns->gid_map.nr_extents = 1;
> +
> + return ns;
> +}
> +
> +/**
> + * test_uid_owns_ns_with_mapping - Test uid_owns_ns with namespace where uid 0
> + * maps to different kuid
> + *
> + * Creates a user namespace where uid 0 maps to kuid 1000 in the parent namespace.
> + * Verifies that uid_owns_ns correctly identifies kuid 1000 as owning the namespace.
> + *
> + * Note: uid_owns_ns walks up the namespace hierarchy, so it checks the current
> + * namespace first, then parent, then parent's parent, etc. So:
> + * - kuid 1000 owns test_ns because from_kuid(test_ns, 1000) == 0
> + * - kuid 0 also owns test_ns because from_kuid(init_user_ns, 0) == 0
> + * (checked in parent)
> + *
> + * This tests the actual functionality as requested: creating namespaces with
> + * different values for the namespace's uid 0.
> + *
> + * @test: KUnit test context
> + */
> +static void test_uid_owns_ns_with_mapping(struct kunit *test)
> +{
> + struct user_namespace *test_ns;
> + struct user_namespace *parent_ns;
> + kuid_t mapped_kuid, other_kuid;
> +
> + parent_ns = &init_user_ns;
> + mapped_kuid = KUIDT_INIT(1000); /* uid 0 in test_ns maps to kuid 1000 */
> + other_kuid = KUIDT_INIT(2000); /* This should not own the namespace */
> +
> + /* Create test namespace where uid 0 maps to kuid 1000 */
> + test_ns = create_test_user_ns_with_mapping(test, parent_ns, mapped_kuid);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_ns);
> +
> + /* kuid 1000 should own the namespace (because uid 0 in test_ns maps to it) */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, mapped_kuid));
> +
> + /* kuid 0 also owns the namespace because it maps to 0 in init_user_ns (parent) */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, KUIDT_INIT(0)));
> +
> + /* Other kuids that don't map to 0 in test_ns or any parent should not own */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, other_kuid));
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, KUIDT_INIT(500)));
> +}
> +
> +/**
> + * test_uid_owns_ns_with_different_mappings - Test with multiple namespaces
> + * having different mappings
> + *
> + * Creates multiple test namespaces with different uid 0 mappings to verify
> + * the function correctly identifies ownership based on the mapping.
> + *
> + * Since uid_owns_ns walks up the hierarchy, kuids that map to 0 in init_user_ns
> + * (like kuid 0) will own all namespaces. But we can still verify that the
> + * specific mapped kuids own their respective namespaces.
> + *
> + * @test: KUnit test context
> + */
> +static void test_uid_owns_ns_with_different_mappings(struct kunit *test)
> +{
> + struct user_namespace *ns1, *ns2, *ns3;
> + struct user_namespace *parent_ns;
> +
> + parent_ns = &init_user_ns;
> +
> + /* Namespace 1: uid 0 maps to kuid 1000 */
> + ns1 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(1000));
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns1);
> + /* kuid 1000 owns ns1 because it maps to uid 0 in ns1 */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(1000)));
> + /* kuid 0 also owns ns1 because it maps to 0 in init_user_ns (parent) */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(0)));
> + /* kuid 2000 doesn't map to 0 in ns1 or any parent */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns1, KUIDT_INIT(2000)));
> +
> + /* Namespace 2: uid 0 maps to kuid 2000 */
> + ns2 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(2000));
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns2);
> + /* kuid 2000 owns ns2 because it maps to uid 0 in ns2 */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(2000)));
> + /* kuid 0 also owns ns2 because it maps to 0 in init_user_ns (parent) */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(0)));
> + /* kuid 1000 doesn't map to 0 in ns2 or any parent */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns2, KUIDT_INIT(1000)));
> +
> + /* Namespace 3: uid 0 maps to kuid 0 (identity mapping) */
> + ns3 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(0));
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns3);
> + /* kuid 0 owns ns3 because it maps to uid 0 in ns3 */
> + KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns3, KUIDT_INIT(0)));
> + /* kuid 1000 doesn't map to 0 in ns3 or any parent */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(1000)));
> + /* kuid 2000 doesn't map to 0 in ns3 or any parent */
> + KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(2000)));
> }
>
> static struct kunit_case commoncap_test_cases[] = {
> - KUNIT_CASE(test_rootid_owns_currentns_init_ns),
> - KUNIT_CASE(test_rootid_owns_currentns_invalid),
> - KUNIT_CASE(test_rootid_owns_currentns_nonroot),
> - KUNIT_CASE(test_rootid_owns_userns_init_ns),
> - KUNIT_CASE(test_rootid_owns_userns_nonroot),
> + KUNIT_CASE(test_uid_owns_currentns_init_ns),
> + KUNIT_CASE(test_uid_owns_currentns_invalid),
> + KUNIT_CASE(test_uid_owns_currentns_nonzero),
> + KUNIT_CASE(test_uid_owns_ns_init_ns_uid0),
> + KUNIT_CASE(test_uid_owns_ns_init_ns_nonzero),
> + KUNIT_CASE(test_uid_owns_ns_init_ns_various_uids),
> + KUNIT_CASE(test_uid_owns_ns_with_mapping),
> + KUNIT_CASE(test_uid_owns_ns_with_different_mappings),
> {}
> };
>
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-25 15:11 UTC (permalink / raw)
To: John Johansen, John Paul Adrian Glaubitz
Cc: Helge Deller, linux-kernel, apparmor, linux-security-module,
linux-parisc
In-Reply-To: <ba3d5651-fa68-4bb5-84aa-35576044e7b0@canonical.com>
* 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;
^ permalink raw reply related
* Re: [PATCH 2/6] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
From: Justin Suess @ 2025-11-25 12:06 UTC (permalink / raw)
To: m; +Cc: gnoack, jack, linux-security-module, mic, utilityemal77, xandfury
In-Reply-To: <59aa2857-46d0-4527-990f-03fd6bf13305@maowtm.org>
Good catch.
Probably just gonna add that comment to the add_rule_path_beneath
since LANDLOCK_ADD_RULE_NO_INHERIT doesn't really apply to networking
stuff at all and really doesn't make sense in those rules.
I may even include some code barring the flag from being included in
irrelevant scopes.
Networking, sockets, and signals don't really have an inheritance
behavior.
I personally don't really see how this flag could apply to any
other scopes but if anyone has ideas I'd love to hear them.
If other hierarchical scopes get added then this flag can support those.
Or maybe this flag can have in a different meaning in those contexts.
Thank You,
Justin Suess
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Roberto Sassu @ 2025-11-25 11:55 UTC (permalink / raw)
To: Eric W. Biederman, Bernd Edlinger
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity
In-Reply-To: <87h5uoxw06.fsf_-_@email.froward.int.ebiederm.org>
On Thu, 2025-11-20 at 14:57 -0600, Eric W. Biederman wrote:
> Instead of computing the new cred before we pass the point of no
> return compute the new cred just before we use it.
>
> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>
> I am not certain why we wanted to compute the cred for the new
> executable so early. Perhaps I missed something but I did not see any
> common errors being signaled. So I don't think we loose anything by
> computing the new cred later.
>
> We gain a lot.
>
> We stop holding the cred_guard_mutex over places where the code sleeps
> and waits for userspace. These places include the waiting for the
> tracer in PTRACE_EVENT_EXIT, "put_user(0, tsk->clear_child_tid)" in
> mm_release, and "get_user(futex_offset, ...") in exit_robust_mutex.
>
> We can remove fs_struct->in_exec. The case where it was used simply
> never comes up, when we compute the cred after de_thread completes.
>
> We remove the possibility of a hang between a tracer calling
> PTRACE_ATTACH/PTRACE_SIEZE and the kernel waiting for the tracer
> in PTRACE_EVENT_EXIT.
>
> ---
> Oleg, Kees, Bernd, Can you see anything I am missing?
+ Mimi, linux-integrity (would be nice if we are in CC when linux-
security-module is in CC).
Apologies for not answering earlier, it seems I don't receive the
emails from the linux-security-module mailing list (thanks Serge for
letting me know!).
I tested your patch but there are a few warnings like this:
[ 2.702374] =====================================
[ 2.702854] WARNING: bad unlock balance detected!
[ 2.703350] 6.18.0-rc6+ #409 Not tainted
[ 2.703755] -------------------------------------
[ 2.704241] init/1 is trying to release lock (init_fs.seq) at:
[ 2.704829] [<ffffffff81836100>] begin_new_exec+0xfe0/0x1710
[ 2.705421] but there are no more locks to release!
[ 2.705931]
[ 2.705931] other info that might help us debug this:
[ 2.706610] 1 lock held by init/1:
[ 2.706958] #0: ffff88810083e538 (&sig->exec_update_lock){+.+.}-{4:4}, at: begin_new_exec+0x769/0x1710
and then the system hangs.
I see two main effects of this patch. First, the bprm_check_security
hook implementations will not see bprm->cred populated. That was a
problem before we made this patch:
https://patchew.org/linux/20251008113503.2433343-1-roberto.sassu@huaweicloud.com/
to work around the problem of not calculating the final DAC credentials
early enough (well, we actually had to change our CREDS_CHECK hook
behavior).
The second, I could not check. If I remember well, unlike the
capability LSM, SELinux/Apparmor/SMACK calculate the final credentials
based on the first file being executed (thus the script, not the
interpreter). Is this patch keeping the same behavior despite preparing
the credentials when the final binary is found?
Thanks
Roberto
> The code compiles but I haven't test it yet.
>
> I thought I was going to move commit_creds before de_thread, but that
> would have taken commit_cred out of exec_update_lock (which introduces
> races).
>
> However I can't see any drawbacks of going the other direction.
>
>
> fs/exec.c | 88 ++++++++++++++----------------------
> fs/fs_struct.c | 1 -
> fs/proc/base.c | 4 +-
> include/linux/fs_struct.h | 1 -
> include/linux/sched/signal.h | 6 ---
> init/init_task.c | 1 -
> kernel/cred.c | 2 +-
> kernel/fork.c | 8 +---
> kernel/ptrace.c | 4 +-
> kernel/seccomp.c | 12 ++---
> 10 files changed, 45 insertions(+), 82 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 4298e7e08d5d..5ae96584dab0 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1090,6 +1090,9 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
> perf_event_comm(tsk, exec);
> }
>
> +static int prepare_bprm_creds(struct linux_binprm *bprm);
> +static void check_unsafe_exec(struct linux_binprm *bprm);
> +
> /*
> * Calling this is the point of no return. None of the failures will be
> * seen by userspace since either the process is already taking a fatal
> @@ -1101,10 +1104,6 @@ int begin_new_exec(struct linux_binprm * bprm)
> struct task_struct *me = current;
> int retval;
>
> - /* Once we are committed compute the creds */
> - retval = bprm_creds_from_file(bprm);
> - if (retval)
> - return retval;
>
> /*
> * This tracepoint marks the point before flushing the old exec where
> @@ -1123,8 +1122,6 @@ int begin_new_exec(struct linux_binprm * bprm)
> retval = de_thread(me);
> if (retval)
> goto out;
> - /* see the comment in check_unsafe_exec() */
> - current->fs->in_exec = 0;
> /*
> * Cancel any io_uring activity across execve
> */
> @@ -1251,6 +1248,25 @@ int begin_new_exec(struct linux_binprm * bprm)
> WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1);
> flush_signal_handlers(me, 0);
>
> + retval = prepare_bprm_creds(bprm);
> + if (retval)
> + goto out_unlock;
> +
> + /*
> + * Check for unsafe execution states before exec_binprm(), which
> + * will call back into begin_new_exec(), into bprm_creds_from_file(),
> + * where setuid-ness is evaluated.
> + */
> + check_unsafe_exec(bprm);
> +
> + /* Set the unchanging part of bprm->cred */
> + retval = security_bprm_creds_for_exec(bprm);
> +
> + /* Once we are committed compute the creds */
> + retval = bprm_creds_from_file(bprm);
> + if (retval)
> + goto out_unlock;
> +
> retval = set_cred_ucounts(bprm->cred);
> if (retval < 0)
> goto out_unlock;
> @@ -1272,9 +1288,9 @@ int begin_new_exec(struct linux_binprm * bprm)
> if (get_dumpable(me->mm) != SUID_DUMP_USER)
> perf_event_exit_task(me);
> /*
> - * cred_guard_mutex must be held at least to this point to prevent
> + * exec_update_lock must be held at least to this point to prevent
> * ptrace_attach() from altering our determination of the task's
> - * credentials; any time after this it may be unlocked.
> + * credentials.
> */
> security_bprm_committed_creds(bprm);
>
> @@ -1291,8 +1307,6 @@ int begin_new_exec(struct linux_binprm * bprm)
>
> out_unlock:
> up_write(&me->signal->exec_update_lock);
> - if (!bprm->cred)
> - mutex_unlock(&me->signal->cred_guard_mutex);
>
> out:
> return retval;
> @@ -1336,7 +1350,6 @@ void setup_new_exec(struct linux_binprm * bprm)
> */
> me->mm->task_size = TASK_SIZE;
> up_write(&me->signal->exec_update_lock);
> - mutex_unlock(&me->signal->cred_guard_mutex);
> }
> EXPORT_SYMBOL(setup_new_exec);
>
> @@ -1351,21 +1364,15 @@ void finalize_exec(struct linux_binprm *bprm)
> EXPORT_SYMBOL(finalize_exec);
>
> /*
> - * Prepare credentials and lock ->cred_guard_mutex.
> - * setup_new_exec() commits the new creds and drops the lock.
> - * Or, if exec fails before, free_bprm() should release ->cred
> - * and unlock.
> + * Prepare credentials. begin_new_exec() commits the new creds.
> + * Or, if exec fails before, free_bprm() should release ->cred.
> */
> static int prepare_bprm_creds(struct linux_binprm *bprm)
> {
> - if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
> - return -ERESTARTNOINTR;
> -
> bprm->cred = prepare_exec_creds();
> if (likely(bprm->cred))
> return 0;
>
> - mutex_unlock(¤t->signal->cred_guard_mutex);
> return -ENOMEM;
> }
>
> @@ -1386,9 +1393,7 @@ static void free_bprm(struct linux_binprm *bprm)
> }
> free_arg_pages(bprm);
> if (bprm->cred) {
> - /* in case exec fails before de_thread() succeeds */
> - current->fs->in_exec = 0;
> - mutex_unlock(¤t->signal->cred_guard_mutex);
> + /* in case exec fails before commit_creds succeeds */
> abort_creds(bprm->cred);
> }
> do_close_execat(bprm->file);
> @@ -1486,13 +1491,12 @@ EXPORT_SYMBOL(bprm_change_interp);
>
> /*
> * determine how safe it is to execute the proposed program
> - * - the caller must hold ->cred_guard_mutex to protect against
> + * - the caller must hold ->exec_update_lock to protect against
> * PTRACE_ATTACH or seccomp thread-sync
> */
> static void check_unsafe_exec(struct linux_binprm *bprm)
> {
> - struct task_struct *p = current, *t;
> - unsigned n_fs;
> + struct task_struct *p = current;
>
> if (p->ptrace)
> bprm->unsafe |= LSM_UNSAFE_PTRACE;
> @@ -1509,25 +1513,9 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
> * suid exec because the differently privileged task
> * will be able to manipulate the current directory, etc.
> * It would be nice to force an unshare instead...
> - *
> - * Otherwise we set fs->in_exec = 1 to deny clone(CLONE_FS)
> - * from another sub-thread until de_thread() succeeds, this
> - * state is protected by cred_guard_mutex we hold.
> */
> - n_fs = 1;
> - read_seqlock_excl(&p->fs->seq);
> - rcu_read_lock();
> - for_other_threads(p, t) {
> - if (t->fs == p->fs)
> - n_fs++;
> - }
> - rcu_read_unlock();
> -
> - /* "users" and "in_exec" locked for copy_fs() */
> - if (p->fs->users > n_fs)
> + if (p->fs->users > 1)
> bprm->unsafe |= LSM_UNSAFE_SHARE;
> - else
> - p->fs->in_exec = 1;
> read_sequnlock_excl(&p->fs->seq);
> }
>
> @@ -1731,25 +1719,15 @@ static int bprm_execve(struct linux_binprm *bprm)
> {
> int retval;
>
> - retval = prepare_bprm_creds(bprm);
> - if (retval)
> - return retval;
> + if (bprm->is_check)
> + return 0;
>
> - /*
> - * Check for unsafe execution states before exec_binprm(), which
> - * will call back into begin_new_exec(), into bprm_creds_from_file(),
> - * where setuid-ness is evaluated.
> - */
> - check_unsafe_exec(bprm);
> current->in_execve = 1;
> sched_mm_cid_before_execve(current);
>
> sched_exec();
>
> - /* Set the unchanging part of bprm->cred */
> - retval = security_bprm_creds_for_exec(bprm);
> - if (retval || bprm->is_check)
> - goto out;
> +
>
> retval = exec_binprm(bprm);
> if (retval < 0)
> diff --git a/fs/fs_struct.c b/fs/fs_struct.c
> index 28be762ac1c6..945bc0916f65 100644
> --- a/fs/fs_struct.c
> +++ b/fs/fs_struct.c
> @@ -109,7 +109,6 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)
> /* We don't need to lock fs - think why ;-) */
> if (fs) {
> fs->users = 1;
> - fs->in_exec = 0;
> seqlock_init(&fs->seq);
> fs->umask = old->umask;
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6299878e3d97..7041fb4d1689 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2834,14 +2834,14 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> }
>
> /* Guard against adverse ptrace interaction */
> - rv = mutex_lock_interruptible(¤t->signal->cred_guard_mutex);
> + rv = down_write_killable(¤t->signal->exec_update_lock);
> if (rv < 0)
> goto out_free;
>
> rv = security_setprocattr(PROC_I(inode)->op.lsmid,
> file->f_path.dentry->d_name.name, page,
> count);
> - mutex_unlock(¤t->signal->cred_guard_mutex);
> + up_write(¤t->signal->exec_update_lock);
> out_free:
> kfree(page);
> out:
> diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
> index baf200ab5c77..29d0f7d57743 100644
> --- a/include/linux/fs_struct.h
> +++ b/include/linux/fs_struct.h
> @@ -10,7 +10,6 @@ struct fs_struct {
> int users;
> seqlock_t seq;
> int umask;
> - int in_exec;
> struct path root, pwd;
> } __randomize_layout;
>
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 7d6449982822..7e9259c8fb2b 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -241,12 +241,6 @@ struct signal_struct {
> struct mm_struct *oom_mm; /* recorded mm when the thread group got
> * killed by the oom killer */
>
> - struct mutex cred_guard_mutex; /* guard against foreign influences on
> - * credential calculations
> - * (notably. ptrace)
> - * Deprecated do not use in new code.
> - * Use exec_update_lock instead.
> - */
> struct rw_semaphore exec_update_lock; /* Held while task_struct is
> * being updated during exec,
> * and may have inconsistent
> diff --git a/init/init_task.c b/init/init_task.c
> index a55e2189206f..4813bffe217e 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -30,7 +30,6 @@ static struct signal_struct init_signals = {
> #ifdef CONFIG_CGROUPS
> .cgroup_threadgroup_rwsem = __RWSEM_INITIALIZER(init_signals.cgroup_threadgroup_rwsem),
> #endif
> - .cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
> .exec_update_lock = __RWSEM_INITIALIZER(init_signals.exec_update_lock),
> #ifdef CONFIG_POSIX_TIMERS
> .posix_timers = HLIST_HEAD_INIT,
> diff --git a/kernel/cred.c b/kernel/cred.c
> index dbf6b687dc5c..80e376ce005f 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -252,7 +252,7 @@ EXPORT_SYMBOL(prepare_creds);
>
> /*
> * Prepare credentials for current to perform an execve()
> - * - The caller must hold ->cred_guard_mutex
> + * - The caller must hold ->exec_update_lock
> */
> struct cred *prepare_exec_creds(void)
> {
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3da0f08615a9..996c649b9a4c 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1555,11 +1555,6 @@ static int copy_fs(u64 clone_flags, struct task_struct *tsk)
> if (clone_flags & CLONE_FS) {
> /* tsk->fs is already what we want */
> read_seqlock_excl(&fs->seq);
> - /* "users" and "in_exec" locked for check_unsafe_exec() */
> - if (fs->in_exec) {
> - read_sequnlock_excl(&fs->seq);
> - return -EAGAIN;
> - }
> fs->users++;
> read_sequnlock_excl(&fs->seq);
> return 0;
> @@ -1699,7 +1694,6 @@ static int copy_signal(u64 clone_flags, struct task_struct *tsk)
> sig->oom_score_adj = current->signal->oom_score_adj;
> sig->oom_score_adj_min = current->signal->oom_score_adj_min;
>
> - mutex_init(&sig->cred_guard_mutex);
> init_rwsem(&sig->exec_update_lock);
>
> return 0;
> @@ -1710,7 +1704,7 @@ static void copy_seccomp(struct task_struct *p)
> #ifdef CONFIG_SECCOMP
> /*
> * Must be called with sighand->lock held, which is common to
> - * all threads in the group. Holding cred_guard_mutex is not
> + * all threads in the group. Holding exec_update_lock is not
> * needed because this new task is not yet running and cannot
> * be racing exec.
> */
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 75a84efad40f..8140d4bfc279 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -444,8 +444,8 @@ static int ptrace_attach(struct task_struct *task, long request,
> * SUID, SGID and LSM creds get determined differently
> * under ptrace.
> */
> - scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> - &task->signal->cred_guard_mutex) {
> + scoped_cond_guard (rwsem_read_intr, return -ERESTARTNOINTR,
> + &task->signal->exec_update_lock) {
>
> scoped_guard (task_lock, task) {
> retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 25f62867a16d..87de8d47d876 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -479,7 +479,7 @@ static int is_ancestor(struct seccomp_filter *parent,
> /**
> * seccomp_can_sync_threads: checks if all threads can be synchronized
> *
> - * Expects sighand and cred_guard_mutex locks to be held.
> + * Expects sighand and exec_update_lock locks to be held.
> *
> * Returns 0 on success, -ve on error, or the pid of a thread which was
> * either not in the correct seccomp mode or did not have an ancestral
> @@ -489,7 +489,7 @@ static inline pid_t seccomp_can_sync_threads(void)
> {
> struct task_struct *thread, *caller;
>
> - BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
> + BUG_ON(!rwsem_is_locked(¤t->signal->exec_update_lock));
> assert_spin_locked(¤t->sighand->siglock);
>
> /* Validate all threads being eligible for synchronization. */
> @@ -590,7 +590,7 @@ void seccomp_filter_release(struct task_struct *tsk)
> *
> * @flags: SECCOMP_FILTER_FLAG_* flags to set during sync.
> *
> - * Expects sighand and cred_guard_mutex locks to be held, and for
> + * Expects sighand and exec_update_lock locks to be held, and for
> * seccomp_can_sync_threads() to have returned success already
> * without dropping the locks.
> *
> @@ -599,7 +599,7 @@ static inline void seccomp_sync_threads(unsigned long flags)
> {
> struct task_struct *thread, *caller;
>
> - BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
> + BUG_ON(!rwsem_is_locked(¤t->signal->exec_update_lock));
> assert_spin_locked(¤t->sighand->siglock);
>
> /*
> @@ -2011,7 +2011,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
> * while another thread is in the middle of calling exec.
> */
> if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
> - mutex_lock_killable(¤t->signal->cred_guard_mutex))
> + down_read_killable(¤t->signal->exec_update_lock))
> goto out_put_fd;
>
> spin_lock_irq(¤t->sighand->siglock);
> @@ -2034,7 +2034,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
> out:
> spin_unlock_irq(¤t->sighand->siglock);
> if (flags & SECCOMP_FILTER_FLAG_TSYNC)
> - mutex_unlock(¤t->signal->cred_guard_mutex);
> + up_read(¤t->signal->exec_update_lock);
> out_put_fd:
> if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> if (ret) {
^ permalink raw reply
* Re: [PATCH v2] lockdown: Only log restrictions once
From: Nicolas Bouchinet @ 2025-11-25 10:00 UTC (permalink / raw)
To: Xiujianfeng
Cc: Daniel Tang, Xiu Jianfeng, Paul Moore, linux-security-module,
linux-kernel, Nathan Lynch, Matthew Garrett, Kees Cook,
David Howells, James Morris
In-Reply-To: <2f4a1af8-adc6-4cbc-813f-4cc8e9bc75ae@huaweicloud.com>
Hi,
> Currently lockdown does not support the audit function, so I believe the
> logs here serve a purpose similar to auditing. Based on this, I think
> this change will meaningfully degrade the quality of the logs, making it
> hard for users to find out what happens when lockdown is active，
> especially after a long time running.
I agree with Xiu.
I'm not sure to understand how this is a kernel issue. I mean beside
that we do not support hibernation in Lockdown for now.
Can't you just disable hibernation with systemd-logind using someting like
'AllowHibernation=no' ?
Best regards,
Nicolas
^ permalink raw reply
* Re: [PATCH v5 07/10] selftests/landlock: add tests for quiet flag with fs rules
From: Tingmao Wang @ 2025-11-25 0:57 UTC (permalink / raw)
To: Justin Suess, Mickaël Salaün
Cc: gnoack, jack, xandfury, linux-security-module
In-Reply-To: <20251124143639.3321365-1-utilityemal77@gmail.com>
On 11/24/25 14:36, Justin Suess wrote:
> [...]
> Small suggestion on the tests and samples. I saw you
> added a bool quiet to some methods for the quiet flag.
>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index 943b6e2ac53d..6aa65d344c72 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -718,11 +718,15 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
>>
>> static void add_path_beneath(struct __test_metadata *const _metadata,
>> const int ruleset_fd, const __u64 allowed_access,
>> - const char *const path)
>> + const char *const path, bool quiet)
>> {
>> struct landlock_path_beneath_attr path_beneath = {
>> .allowed_access = allowed_access,
>> };
>> + __u32 flags = 0;
>> +
>> + if (quiet)
>> + flags |= LANDLOCK_ADD_RULE_QUIET;
>>
>> path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
>> ASSERT_LE(0, path_beneath.parent_fd)
>
>
> I think that the bool quiet could be replaced with a flags field
> so it can support other flags.
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 6aa65d344c72..5c38a11f1a05 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -717,16 +717,12 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
> }
>
> static void add_path_beneath(struct __test_metadata *const _metadata,
> - const int ruleset_fd, const __u64 allowed_access,
> - const char *const path, bool quiet)
> + const int ruleset_fd, const __u64 allowed_access,
> + const char *const path, __u32 flags)
> {
> struct landlock_path_beneath_attr path_beneath = {
> .allowed_access = allowed_access,
> };
> - __u32 flags = 0;
> -
> - if (quiet)
> - flags |= LANDLOCK_ADD_RULE_QUIET;
>
> path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
> ASSERT_LE(0, path_beneath.parent_fd)
>
> And then update the tests to account for the changed
> function signature.
>
> I think the bool quiet in the landlock-sandboxer methods
> populate_ruleset_fs and populate_ruleset_net (in
> samples/landlock/sandboxer.c) should be updated as well,
> replacing the bool quiet with a general flags field.
Good point - I think both suggestions makes sense for future-proofing.
Here are the proper changes, which I will apply to v6. For your
convenience, the new set of commits are available at
https://github.com/micromaomao/linux-dev/pull/13/commits
Mickaël - let me know if you have any other feedback on this series, and I
will send v6 afterwards.
squash! selftests/landlock: add tests for quiet flag with fs rules
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6aa65d344c72..c29ee72b2cc1 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -718,15 +718,11 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
static void add_path_beneath(struct __test_metadata *const _metadata,
const int ruleset_fd, const __u64 allowed_access,
- const char *const path, bool quiet)
+ const char *const path, __u32 flags)
{
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = allowed_access,
};
- __u32 flags = 0;
-
- if (quiet)
- flags |= LANDLOCK_ADD_RULE_QUIET;
path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
ASSERT_LE(0, path_beneath.parent_fd)
@@ -790,7 +786,7 @@ static int create_ruleset(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
- rules[i].path, false);
+ rules[i].path, 0);
}
return ruleset_fd;
}
@@ -1368,7 +1364,7 @@ TEST_F_FORK(layout1, inherit_subset)
* ANDed with the previous ones.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d2, false);
+ dir_s1d2, 0);
/*
* According to ruleset_fd, dir_s1d2 should now have the
* LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
@@ -1400,7 +1396,7 @@ TEST_F_FORK(layout1, inherit_subset)
* Try to get more privileges by adding new access rights to the parent
* directory: dir_s1d1.
*/
- add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, false);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, 0);
enforce_ruleset(_metadata, ruleset_fd);
/* Same tests and results as above. */
@@ -1423,7 +1419,7 @@ TEST_F_FORK(layout1, inherit_subset)
* that there was no rule tied to it before.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d3, false);
+ dir_s1d3, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1476,7 +1472,7 @@ TEST_F_FORK(layout1, inherit_superset)
add_path_beneath(_metadata, ruleset_fd,
LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_READ_DIR,
- dir_s1d2, false);
+ dir_s1d2, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -7647,7 +7643,7 @@ static int apply_a_layer(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, rs_fd, r->access, r->path,
- r->quiet);
+ r->quiet ? LANDLOCK_ADD_RULE_QUIET : 0);
}
ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
squash! samples/landlock: Add quiet flag support to sandboxer
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 2d8e3e94b77b..07dc0013ff19 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -121,7 +121,7 @@ static int parse_path(char *env_path, const char ***const path_list)
/* clang-format on */
static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
- const __u64 allowed_access, bool quiet)
+ const __u64 allowed_access, __u32 flags)
{
int num_paths, i, ret = 1;
char *env_path_name;
@@ -171,8 +171,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
if (!S_ISDIR(statbuf.st_mode))
path_beneath.allowed_access &= ACCESS_FILE;
if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
- &path_beneath,
- quiet ? LANDLOCK_ADD_RULE_QUIET : 0)) {
+ &path_beneath, flags)) {
fprintf(stderr,
"Failed to update the ruleset with \"%s\": %s\n",
path_list[i], strerror(errno));
@@ -190,7 +189,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
}
static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
- const __u64 allowed_access, bool quiet)
+ const __u64 allowed_access, __u32 flags)
{
int ret = 1;
char *env_port_name, *env_port_name_next, *strport;
@@ -218,8 +217,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
}
net_port.port = port;
if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
- &net_port,
- quiet ? LANDLOCK_ADD_RULE_QUIET : 0)) {
+ &net_port, flags)) {
fprintf(stderr,
"Failed to update the ruleset with port \"%llu\": %s\n",
net_port.port, strerror(errno));
@@ -595,35 +593,31 @@ int main(const int argc, char *const argv[], char *const *const envp)
return 1;
}
- if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro,
- false)) {
+ if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro, 0))
goto err_close_ruleset;
- }
- if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw,
- false)) {
+ if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw, 0))
goto err_close_ruleset;
- }
+
/* Don't require this env to be present. */
if (quiet_supported && getenv(ENV_FS_QUIET_NAME)) {
if (populate_ruleset_fs(ENV_FS_QUIET_NAME, ruleset_fd, 0,
- true)) {
+ LANDLOCK_ADD_RULE_QUIET))
goto err_close_ruleset;
- }
}
if (populate_ruleset_net(ENV_TCP_BIND_NAME, ruleset_fd,
- LANDLOCK_ACCESS_NET_BIND_TCP, false)) {
+ LANDLOCK_ACCESS_NET_BIND_TCP, 0)) {
goto err_close_ruleset;
}
if (populate_ruleset_net(ENV_TCP_CONNECT_NAME, ruleset_fd,
- LANDLOCK_ACCESS_NET_CONNECT_TCP, false)) {
+ LANDLOCK_ACCESS_NET_CONNECT_TCP, 0)) {
goto err_close_ruleset;
}
/* Don't require this env to be present. */
if (quiet_supported && getenv(ENV_NET_QUIET_NAME)) {
if (populate_ruleset_net(ENV_NET_QUIET_NAME, ruleset_fd, 0,
- true)) {
+ LANDLOCK_ADD_RULE_QUIET)) {
goto err_close_ruleset;
}
}
^ permalink raw reply related
* Re: [PATCH v5 07/10] selftests/landlock: add tests for quiet flag with fs rules
From: Justin Suess @ 2025-11-24 14:36 UTC (permalink / raw)
To: m; +Cc: gnoack, jack, linux-security-module, mic, utilityemal77, xandfury
In-Reply-To: <a839025f9ce9facee60ff8238ee350b5e780b777.1763931318.git.m@maowtm.org>
Good morning,
Great job on the patch.
Small suggestion on the tests and samples. I saw you
added a bool quiet to some methods for the quiet flag.
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 943b6e2ac53d..6aa65d344c72 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -718,11 +718,15 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
>
> static void add_path_beneath(struct __test_metadata *const _metadata,
> const int ruleset_fd, const __u64 allowed_access,
> - const char *const path)
> + const char *const path, bool quiet)
> {
> struct landlock_path_beneath_attr path_beneath = {
> .allowed_access = allowed_access,
> };
> + __u32 flags = 0;
> +
> + if (quiet)
> + flags |= LANDLOCK_ADD_RULE_QUIET;
>
> path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
> ASSERT_LE(0, path_beneath.parent_fd)
I think that the bool quiet could be replaced with a flags field
so it can support other flags.
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6aa65d344c72..5c38a11f1a05 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -717,16 +717,12 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
}
static void add_path_beneath(struct __test_metadata *const _metadata,
- const int ruleset_fd, const __u64 allowed_access,
- const char *const path, bool quiet)
+ const int ruleset_fd, const __u64 allowed_access,
+ const char *const path, __u32 flags)
{
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = allowed_access,
};
- __u32 flags = 0;
-
- if (quiet)
- flags |= LANDLOCK_ADD_RULE_QUIET;
path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
ASSERT_LE(0, path_beneath.parent_fd)
And then update the tests to account for the changed
function signature.
I think the bool quiet in the landlock-sandboxer methods
populate_ruleset_fs and populate_ruleset_net (in
samples/landlock/sandboxer.c) should be updated as well,
replacing the bool quiet with a general flags field.
I have change this in my patch but it might make more sense in
your patch since this is the first patch to add flags and would
make it easier if anyone else decides to add flags rebased on
your patch.
Great work and thank you for your help.
Kind Regards,
Justin Suess
^ permalink raw reply related
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Roberto Sassu @ 2025-11-24 10:01 UTC (permalink / raw)
To: Anirudh Venkataramanan, linux-integrity
Cc: Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <a77e9609-f6bd-4e6d-88be-5422f780b496@linux.microsoft.com>
On Fri, 2025-11-21 at 11:13 -0800, Anirudh Venkataramanan wrote:
> On 11/20/2025 3:02 AM, Roberto Sassu wrote:
> > On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
> > > ==========================================================================
> > > > A. Introduction |
> > > ==========================================================================
> > >
> > > IMA events are kept in kernel memory and preserved across kexec soft
> > > reboots. This can lead to increased kernel memory usage over time,
> > > especially with aggressive IMA policies that measure everything. To reduce
> > > memory pressure, it becomes necessary to discard IMA events but given that
> > > IMA events are extended into PCRs in the TPM, just discarding events will
> > > break the PCR extension chain, making future verification of the IMA event
> > > log impossible.
> > >
> > > This patch series proposes a method to discard IMA events while keeping the
> > > log verifiable. While reducing memory pressure is the primary objective,
> > > the second order benefit of trimming the IMA log is that IMA log verifiers
> > > (local userspace daemon or a remote cloud service) can process smaller IMA
> > > logs on a rolling basis, thus avoiding re-verification of previously
> > > verified events.
> >
> > Hi Anirudh
>
> Hi Roberto,
>
> Thanks for the feedback! Few questions below.
>
> >
> > I will rephrase this paragraph, to be sure that I understood it
> > correctly.
> >
> > You are proposing a method to trim the measurement list and, at the
> > same time, to keep the measurement list verifiable after trimming. The
> > way you would like to achieve that is to keep the verification state in
> > the kernel in the form of PCR values.
> >
> > Those values mean what verifiers have already verified. Thus, for the
> > next verification attempt, verifiers take the current PCR values as
> > starting values, replay the truncated IMA measurement list, and again
> > you match with the current PCR values and you trim until that point.
> >
> > So the benefit of this proposal is that you keep the verification of
> > the IMA measurement list self-contained by using the last verification
> > state (PCR starting value) and the truncated IMA measurement list as
> > the inputs of your verification.
>
> Your understanding as described above is correct.
>
> >
> > Let me reiterate on the trusted computing principles IMA relies on for
> > providing the evidence about a system's integrity.
> >
> > Unless you are at the beginning of the measurement chain, where the
> > Root of Trust for Measurement (RTM) is trusted by assumption, the
> > measurements done by a component can be trusted because that component
> > was already measured by the previous component in the boot chain,
> > before it had any chance to corrupt the system.
> >
> > In the context of IMA, IMA can be trusted to make new measurements
> > because it measures every file before those files could cause any harm
> > to the system. So, potentially IMA and the kernel can be corrupted by
> > any file.
> >
> > What you are proposing would not work, because you are placing trust in
> > an input (the PCR starting value) that can be manipulated at any time
> > by a corrupted kernel, before you had the chance to detect such
> > corruption.
>
> If starting PCR values can be corrupted, the IMA measurements list can
> also be corrupted, right?
Yes, my point was that there could be a malicious update of the PCR
starting value, so that the IMA measurements list with omitted entries
looks not corrupted.
> More generally, what integrity guarantees can be provided (if any) if
> the kernel itself is corrupted?
None. This is exactly the point: measuring the files before they get
accessed is the only way to provide reliable measurements. After that,
we assume that the operation can corrupt both the user space processes
and the kernel itself (the threat model takes into consideration only
regular files).
But if that happened, replaying the IMA measurements list will reveal
the attack at that point, and the remote verifier can conclude that any
measurement after that cannot be trusted.
Your solution is storing the verification state in the kernel, and make
remote verifiers rely on it for resuming their verification. But,
because the kernel can get potentially corrupted by every measurement,
the verification state can also get potentially corrupted by every
measurement, thus cannot be trusted.
The only way to make the verification of measurements list snapshots
work is that the verification state is stored outside the system to
evaluate (which can be assumed to be trusted), so that you are sure
that the system is not advancing the PCR starting value by itself.
> > Let me describe a scenario where I could take advantage of such
> > weakness. After the first measurement list trim, I perform an attack on
> > the system such that it corrupts the kernel. IMA added a new entry in
> > the measurement list, which would reveal the attack.
> >
> > But, since I have control of the kernel, I conveniently update the PCR
> > starting value to replay the new measurement entry, and remove the
> > measurement entry from the measurement list.
> >
> > Now, from the perspective of the user space verifiers everything is
> > fine, the truncated IMA measurement list is clean, no attack, and the
> > current PCR values match by replaying the new PCR starting value with
> > the remaining of the IMA measurement list.
>
> Wouldn't the verifier detect the attack when it sees that its
> recalculated PCR values don't match up to the PCR digest in the TPM quote?
I think not, because the system replayed the entries it wants to omit
by itself. If the remote verifier is trusting the PCR starting value
from the system, the remote verifier will replay the remaining
measurement entries and will obtain the PCR current values.
The same will happen if someone in the system just did a regular trim
without the remote attestation agent noticing it. Unless the agent
stored which was the last PCR starting value it took, it would not
notice.
But, again, if you rely on the remote attestation agent to maintain the
verification state locally in the system, you are assuming that the
agent will not be corrupted, which is a much stronger assumption than
just letting the agent pass data to the remote verifier (which instead
can be trusted to detect the PCR mismatch).
So, yes, the point of trimming is to just get the IMA measurements list
out of the kernel memory. I'm not opposing to trim N entries instead of
the entire IMA measurements list, as long as: (1) the PCR matching is
done in user space and is done only for convenience (can be totally
untrusted); (2) the verification state is stored in the remote verifier
(outside the system evaluated), and latter detects the PCR mismatch.
Roberto
> > So, in my opinion the kernel should just offer the ability to trim the
> > measurement list, and a remote verifier should be responsible to verify
> > the measurement list, without relying on anything from the system being
> > evaluated.
> >
> > Sure, the remote verifier can verify just the trimmed IMA measurement
> > list, but the remote verifier must solely rely on state information
> > maintained internally.
> >
> > Roberto
> >
> > > The method has other advantages too:
> > >
> > > 1. It provides a userspace interface that can be used to precisely control
> > > trim point, allowing for trim points to be optionally aligned with
> > > userspace IMA event log validation.
> > >
> > > 2. It ensures that all necessary information required for continued IMA
> > > log validation is made available via the userspace interface at all
> > > times.
> > >
> > > 3. It provides a simple mechanism for userspace applications to determine
> > > if the event log has been unexpectedly trimmed.
> > >
> > > 4. The duration for which the IMA Measurement list mutex must be held (for
> > > trimming) is minimal.
> > >
> > > ==========================================================================
> > > > B. Solution |
> > > ==========================================================================
> > >
> > > --------------------------------------------------------------------------
> > > > B.1 Overview |
> > > --------------------------------------------------------------------------
> > >
> > > The kernel trims the IMA event log based on PCR values supplied by userspace.
> > > The core principles leveraged are as follows:
> > >
> > > - Given an IMA event log, PCR values for each IMA event can be obtained by
> > > recalulating the PCR extension for each event. Thus processing N events
> > > from the start will yield PCR values as of event N. This is referred to
> > > as "IMA event log replay".
> > >
> > > - To get the PCR value for event N + 1, only the PCR value as of event N
> > > is needed. If this can be known, events till and including N can be
> > > safely purged.
> > >
> > > Putting it all together, we get the following userspace + kernel flow:
> > >
> > > 1. A userspace application replays the IMA event log to generate PCR
> > > values and then triggers a trim by providing these values to the kernel
> > > (by writing to a pseudo file).
> > >
> > > Optionally, the userspace application may verify these PCR values
> > > against the corresponding TPM quote, and trigger trimming only if
> > > the calculated PCR values match up to the expectations in the quote's
> > > PCR digest.
> > >
> > > 2. The kernel uses the userspace supplied PCR values to trim the IMA
> > > measurements list at a specific point, and so these are referred to as
> > > "trim-to PCR values" in this context.
> > >
> > > Note that the kernel doesn't really understand what these userspace
> > > provided PCR values mean or what IMA event they correspond to, and so
> > > it does its own IMA event replay till either the replayed PCR values
> > > match with the userspace provided ones, or it runs out of events.
> > >
> > > If a match is found, the kernel can proceed with trimming the IMA
> > > measurements list. This is done in two steps, to keep locking context
> > > minimal.
> > >
> > > step 1: Find and return the list entry (as a count from head) of exact
> > > match. This does not lock the measurements list mutex, ensuring
> > > new events can be appended to the log.
> > >
> > > step 2: Lock the measurements list mutex and trim the measurements list
> > > at the previously identified list entry.
> > >
> > > If the trim is successful, the trim-to PCR values are saved as "starting
> > > PCR values". The next time userspace wants to replay the IMA event log,
> > > it will use the starting PCR values as the base for the IMA event log
> > > replay.
> > >
> > > --------------------------------------------------------------------------
> > > > B.2 Kernel Interfaces |
> > > --------------------------------------------------------------------------
> > >
> > > A new configfs pseudo file /sys/kernel/config/ima/pcrs that supports the
> > > following operations is exposed.
> > >
> > > read: returns starting PCR values stored in the kernel (within IMA
> > > specifically).
> > >
> > > write: writes trim-to PCR values to trigger trimming. If trimming is
> > > successful, trim-to PCR values are stored as starting PCR values.
> > > requires root privileges.
> > >
> > > --------------------------------------------------------------------------
> > > > B.3 Walk-through with a real example |
> > > --------------------------------------------------------------------------
> > >
> > > This is a real example from a test run.
> > >
> > > Suppose this IMA policy is deployed:
> > >
> > > measure func=FILE_CHECK mask=MAY_READ pcr=10
> > > measure func=FILE_CHECK mask=MAY_WRITE pcr=11
> > >
> > > When the policy is deployed, a zero digest starting PCR value will be set
> > > for each PCR used. If the TPM supports multiple hashbanks, there will be
> > > one starting PCR value per PCR, per TPM hashbank. This can be seen in the
> > > following hexdump:
> > >
> > > $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
> > > 00000000 70 63 72 31 30 3a 73 68 61 31 3a 00 00 00 00 00 |pcr10:sha1:.....|
> > > 00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 |...............p|
> > > 00000020 63 72 31 31 3a 73 68 61 31 3a 00 00 00 00 00 00 |cr11:sha1:......|
> > > 00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 63 |..............pc|
> > > 00000040 72 31 30 3a 73 68 61 32 35 36 3a 00 00 00 00 00 |r10:sha256:.....|
> > > 00000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 00000060 00 00 00 00 00 00 00 00 00 00 00 70 63 72 31 31 |...........pcr11|
> > > 00000070 3a 73 68 61 32 35 36 3a 00 00 00 00 00 00 00 00 |:sha256:........|
> > > 00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 00000090 00 00 00 00 00 00 00 00 70 63 72 31 30 3a 73 68 |........pcr10:sh|
> > > 000000a0 61 33 38 34 3a 00 00 00 00 00 00 00 00 00 00 00 |a384:...........|
> > > 000000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 000000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 000000d0 00 00 00 00 00 70 63 72 31 31 3a 73 68 61 33 38 |.....pcr11:sha38|
> > > 000000e0 34 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |4:..............|
> > > 000000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 00000100 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> > > 00000110 00 00 |..|
> > > 00000112
> > >
> > > Let's say that a userspace utility replays the IMA event log, and triggers
> > > trimming by writing the following PCR values (i.e. trim-to PCR values) to the
> > > pseudo file:
> > >
> > > pcr10:sha256:8268782906555cf3aefc179f815c878527dd4e67eaa836572ebabab31977922c
> > > pcr11:sha256:4c7f31927183eacb53d51d95b0162916fd3fca51a8d1efc6dde3805eb891fe41
> > >
> > > The trim is successful,
> > >
> > > 1. Some number of entries from the measurements log will disappear. This
> > > can be verified by reading out the ASCII or binary IMA measurements
> > > file.
> > >
> > > 2. The trim-to PCR values are saved as starting PCR values. This can be
> > > verified by reading out the pseudo file again as shown below. Note that
> > > even through only sha256 PCR values were written, the kernel populated
> > > sha1 and sha384 starting values as well.
> > >
> > > $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
> > >
> > > 00000000 70 63 72 31 30 3a 73 68 61 31 3a c4 7f 9d 00 68 |pcr10:sha1:....h|
> > > 00000010 e4 86 71 bf bc ae f0 10 12 ff 68 e2 9e 74 e4 70 |..q.......h..t.p|
> > > 00000020 63 72 31 31 3a 73 68 61 31 3a 90 d7 17 ac 60 4d |cr11:sha1:....`M|
> > > 00000030 c8 25 ce 77 7d 9d 94 cf 44 7b b2 2e 2e e2 70 63 |.%.w}...D{....pc|
> > > 00000040 72 31 30 3a 73 68 61 32 35 36 3a 82 68 78 29 06 |r10:sha256:.hx).|
> > > 00000050 55 5c f3 ae fc 17 9f 81 5c 87 85 27 dd 4e 67 ea |U\......\..'.Ng.|
> > > 00000060 a8 36 57 2e ba ba b3 19 77 92 2c 70 63 72 31 31 |.6W.....w.,pcr11|
> > > 00000070 3a 73 68 61 32 35 36 3a 4c 7f 31 92 71 83 ea cb |:sha256:L.1.q...|
> > > 00000080 53 d5 1d 95 b0 16 29 16 fd 3f ca 51 a8 d1 ef c6 |S.....)..?.Q....|
> > > 00000090 dd e3 80 5e b8 91 fe 41 70 63 72 31 30 3a 73 68 |...^...Apcr10:sh|
> > > 000000a0 61 33 38 34 3a 8e d6 12 18 b1 d6 cd 95 16 98 33 |a384:..........3|
> > > 000000b0 2b 7d a2 d6 d9 05 c7 e8 5b 15 b0 91 c5 fc 23 d1 |+}......[.....#.|
> > > 000000c0 f9 a8 8d 60 50 5c e9 64 5f d7 b3 b2 f1 9c 90 0a |...`P\.d_.......|
> > > 000000d0 45 53 5d b2 57 70 63 72 31 31 3a 73 68 61 33 38 |ES].Wpcr11:sha38|
> > > 000000e0 34 3a 25 fc 21 28 31 5a f7 c6 fb 0f 40 c9 06 e6 |4:%.!(1Z....@...|
> > > 000000f0 c5 da ed 20 61 a1 03 54 4f 67 18 88 82 0f 48 d1 |... a..TOg....H.|
> > > 00000100 2f e0 3d 36 46 5e 94 a4 88 51 f8 91 39 7e e5 97 |/.=6F^...Q..9~..|
> > > 00000110 2c c5 |,.|
> > > 00000112
> > >
> > > --------------------------------------------------------------------------
> > > > C. Footnotes |
> > > --------------------------------------------------------------------------
> > >
> > > 1. The 'pcrs' pseudo file is currently part of configfs. This was due to
> > > some early internal feedback in a different context. This can as well be
> > > in securityfs with the rest of the IMA pseudo files.
> > >
> > > 2. PCR values are never read out of the TPM at any point. All PCR values
> > > used are derived from IMA event log replay.
> > >
> > > 3. Code is "RFC quality". Refinements can be made if the method is accepted.
> > >
> > > 4. For functional validation, base kernel version was 6.17 stable, with the
> > > most recent tested version being 6.17.8.
> > >
> > > 5. Code has been validated to some degree using a python-based internal test
> > > tool. This can be published if there is community interest.
> > >
> > > Steven Chen (1):
> > > ima: Implement IMA event log trimming
> > >
> > > drivers/Kconfig | 2 +
> > > drivers/Makefile | 1 +
> > > drivers/ima/Kconfig | 13 +
> > > drivers/ima/Makefile | 2 +
> > > drivers/ima/ima_config_pcrs.c | 291 ++++++++++++++++++
> > > include/linux/ima.h | 27 ++
> > > security/integrity/ima/Makefile | 4 +
> > > security/integrity/ima/ima.h | 8 +
> > > security/integrity/ima/ima_init.c | 44 +++
> > > security/integrity/ima/ima_log_trim.c | 421 ++++++++++++++++++++++++++
> > > security/integrity/ima/ima_policy.c | 7 +-
> > > security/integrity/ima/ima_queue.c | 5 +-
> > > 12 files changed, 821 insertions(+), 4 deletions(-)
> > > create mode 100644 drivers/ima/Kconfig
> > > create mode 100644 drivers/ima/Makefile
> > > create mode 100644 drivers/ima/ima_config_pcrs.c
> > > create mode 100644 security/integrity/ima/ima_log_trim.c
> > >
> >
>
^ permalink raw reply
* Re: [PATCH v3 0/9] module: Introduce hash-based integrity checking
From: Thomas Weißschuh @ 2025-11-24 9:41 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: James Bottomley, Masahiro Yamada, Nathan Chancellor,
Arnd Bergmann, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Paul Moore, James Morris, Serge E. Hallyn,
Jonathan Corbet, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Nicolas Schier,
Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi, linux-kbuild,
linux-kernel, linux-arch, linux-modules, linux-security-module,
linux-doc, linuxppc-dev, linux-integrity
In-Reply-To: <20251123170502.Ai5Ig66Z@breakpoint.cc>
Hi Sebastian,
On 2025-11-23 18:05:02+0100, Sebastian Andrzej Siewior wrote:
> On 2025-11-19 16:48:34 [+0100], Sebastian Andrzej Siewior wrote:
> > I fully agree with this approach. I don't like the big hash array but I
> > have an idea how to optimize that part. So I don't see a problem in the
> > long term.
>
> The following PoC creates a merkle tree from a set files ending with .ko
> within the specified directory. It will write a .hash files containing
> the required hash for each file for its validation. The root hash is
> saved as "hash_root" and "hash_root.h" in the directory.
Thanks a lot!
> The Debian kernel shipps 4256 modules:
>
> | $ time ./compute_hashes mods_deb
> | Files 4256 levels: 13 root hash: 97f8f439d63938ed74f48ec46dbd75c2b5e5b49f012a414e89b6f0e0f06efe84
> |
> | real 0m0,732s
> | user 0m0,304s
> | sys 0m0,427s
>
> This computes the hashes for all the modules it found in the mods_deb
> folder.
> The kernel needs the root hash (for sha256 32 bytes) and the depth of
> the tree (4 bytes). That are 36 bytes regardless of the number of
> modules that are built.
> In this case, the attached hash for each module is 420 bytes. This is 4
> bytes (position in the tree) + 13 (depth) * 32.
> The verification process requires 13 hash operation to hash through the
> tree and verify against the root hash.
We'll need to store the proof together with the modules somewhere.
Regular module signatures are stored as PKCS#7 and appended to the module
file. If we can also encode the merkle proof as PKCS#7, the integration
into the existing infrastructure should be much easier.
It will require some changes to this series, but honestly the Merkle
tree aproach looks like the clear winner here.
> For convience, the following PoC can also be found at
> https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/mtree-hashed-mods.git/
>
> which also includes a small testsuite.
(...)
Thomas
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-23 23:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <aSNX5B9a5iSjJcM1@redhat.com>
Oleg Nesterov <oleg@redhat.com> writes:
> Eric,
>
> sorry for delay, I am on PTO, didn't read emails this week...
>
> On 11/20, Eric W. Biederman wrote:
>>
>> Instead of computing the new cred before we pass the point of no
>> return compute the new cred just before we use it.
>>
>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>
>> I am not certain why we wanted to compute the cred for the new
>> executable so early. Perhaps I missed something but I did not see any
>> common errors being signaled. So I don't think we loose anything by
>> computing the new cred later.
>>
>> We gain a lot.
>
> Yes. I LIKE your approach after a quick glance. And I swear, I thought about
> it too ;)
>
> But is it correct? I don't know. I'll try to actually read your patch next
> week (I am on PTO untill the end of November), but I am not sure I can
> provide a valuable feedback.
>
> One "obvious" problem is that, after this patch, the execing process can crash
> in a case when currently exec() returns an error...
Yes.
I have been testing and looking at it, and I have found a few issues,
and I am trying to see if I can resolve them.
The good news is that with the advent of AT_EXECVE_CHECK we have a
really clear API boundary between errors that must be diagnosed
and errors of happenstance like running out of memory.
The bad news is that the implementation of AT_EXECVE_CHECK seems to been
rather hackish especially with respect to security_bprm_creds_for_exec.
What I am hoping for is to get the 3 causes of errors of brpm->unsafe
( LSM_UNSAFE_SHARE, LSM_UNSAFE_PTRACE, and LSM_UNSAFE_NO_NEW_PRIVS )
handled cleanly outside of the cred_guard_mutex, and simply
retested when it is time to build the credentials of the new process.
In practice that should get the same failures modes as we have now
but it would get SIGSEGV in rare instances where things changed
during exec. That feels acceptable.
I thought of one other approach that might be enough to put the issue to
bed if cleaning up exec is too much work. We could have ptrace_attach
use a trylock and fail when it doesn't succeed. That would solve the
worst of the symptoms.
I think this would be a complete patch:
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 75a84efad40f..5dd2144e5789 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -444,7 +444,7 @@ static int ptrace_attach(struct task_struct *task, long request,
* SUID, SGID and LSM creds get determined differently
* under ptrace.
*/
- scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
+ scoped_cond_guard (mutex_try, return -EAGAIN,
&task->signal->cred_guard_mutex) {
scoped_guard (task_lock, task) {
--
2.41.0
Eric
^ permalink raw reply related
* Re: [PATCH v5 02/10] landlock: Add API support and docs for the quiet flags
From: Tingmao Wang @ 2025-11-23 21:37 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <f78d2189e01229cc3b3f6138c19617b653ab9a19.1763931318.git.m@maowtm.org>
On 11/23/25 20:57, Tingmao Wang wrote:
> @@ -185,6 +188,8 @@ const int landlock_abi_version = 7;
> *
> * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
> + * - %EINVAL: quiet_access_fs or quiet_fs_net is not a subset of the
> + * corresponding handled_access_fs or handled_access_net;
Oops, not sure how I ended up with quiet_fs_net
Feel free to squash (or I will in v6)
(Thanks github)
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 93396bfc1500..5cf1183bb596 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -188,7 +188,7 @@ const int landlock_abi_version = 8;
*
* - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
* - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
- * - %EINVAL: quiet_access_fs or quiet_fs_net is not a subset of the
+ * - %EINVAL: quiet_access_fs or quiet_access_net is not a subset of the
* corresponding handled_access_fs or handled_access_net;
* - %E2BIG: @attr or @size inconsistencies;
* - %EFAULT: @attr or @size inconsistencies;
^ permalink raw reply related
* Re: [PATCH v4 00/10] Implement LANDLOCK_ADD_RULE_QUIET
From: Tingmao Wang @ 2025-11-23 21:03 UTC (permalink / raw)
To: Justin Suess; +Cc: gnoack, jack, linux-security-module, mic, xandfury
In-Reply-To: <20251123170103.2640561-1-utilityemal77@gmail.com>
On 11/23/25 17:01, Justin Suess wrote:
> I had a question in regards to the quiet flag in how it
> should interact with my proposed flag LANDLOCK_ADD_RULE_NO_INHERIT.
>
> Should this flag block inheritence of the LANDLOCK_ADD_RULE_QUIET flag?
> It seems to me it should block inheritence of this flag, so you can
> create more fine grained audit-suppression rules.
I think it probably should, given that inheriting the quiet flag is also a
form of "inheritance", right?
Also, if no_inherit inhibits all form of inheritance, then there is
opportunity for an optimization in which we stop the pathwalk altogether
if all layers has stopped inheritance (after verifying path_connected).
>
> So for example you could quiet logs on /a/b with the exception of /a/b/c
> by setting LANDLOCK_ADD_RULE_NO_INHERIT on /a/b/c.
>
> If so, as we add more flags, should this be a general policy that
> LANDLOCK_ADD_RULE_NO_INHERIT blocks access right inheritence AND flag
> inheritence? With the obvious exception of LANDLOCK_ADD_RULE_NO_INHERIT
> itself.
In fact, I don't see why LANDLOCK_ADD_RULE_NO_INHERIT itself would be an
"exception". It doesn't matter whether this flag inherits down, since it
is set on the rule that stops inheritance itself.
Kind regards,
Tingmao
^ permalink raw reply
* Re: [PATCH 2/6] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
From: Tingmao Wang @ 2025-11-23 21:03 UTC (permalink / raw)
To: Justin Suess
Cc: Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, linux-security-module
In-Reply-To: <20251120222346.1157004-3-utilityemal77@gmail.com>
On 11/20/25 22:23, Justin Suess wrote:
> Implements the syscall side flag handling and kernel api headers for the
> LANDLOCK_ADD_RULE_NO_INHERIT flag.
I guess you probably want to change the comment in add_rule_* as well:
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 200287a34895..650ffce6f92e 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -352,7 +352,7 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
/*
* Informs about useless rule: empty allowed_access (i.e. deny rules)
* are ignored in path walks. However, the rule is not useless if it
- * is there to hold a quiet flag
+ * is there to hold a quiet or no inherit flag.
*/
if (!flags && !path_beneath_attr.allowed_access)
return -ENOMSG;
@@ -393,7 +393,7 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
/*
* Informs about useless rule: empty allowed_access (i.e. deny rules)
* are ignored by network actions. However, the rule is not useless
- * if it is there to hold a quiet flag
+ * if it is there to hold a quiet or no inherit flag.
*/
if (!flags && !net_port_attr.allowed_access)
return -ENOMSG;
^ permalink raw reply related
* [PATCH v5 10/10] selftests/landlock: Add tests for invalid use of quiet flag
From: Tingmao Wang @ 2025-11-23 20:57 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, Justin Suess, Jan Kara,
Abhinav Saxena, linux-security-module
In-Reply-To: <cover.1763931318.git.m@maowtm.org>
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v4:
- New patch
tools/testing/selftests/landlock/base_test.c | 57 ++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index b34b340c52a5..055d416508a0 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -526,4 +526,61 @@ TEST(cred_transfer)
EXPECT_EQ(EACCES, errno);
}
+TEST(useless_quiet_rule)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
+ .quiet_access_fs = 0,
+ };
+ struct landlock_path_beneath_attr path_beneath_attr = {
+ .allowed_access = LANDLOCK_ACCESS_FS_READ_DIR,
+ };
+ int ruleset_fd, root_fd;
+
+ drop_caps(_metadata);
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ root_fd = open("/", O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, root_fd);
+ path_beneath_attr.parent_fd = root_fd;
+ ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &path_beneath_attr,
+ LANDLOCK_ADD_RULE_QUIET));
+ ASSERT_EQ(EINVAL, errno);
+
+ /* Check that the rule had not been added. */
+ ASSERT_EQ(0, close(root_fd));
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(-1, open("/", O_RDONLY | O_DIRECTORY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+}
+
+TEST(invalid_quiet_bits_1)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_WRITE_FILE,
+ };
+
+ ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr,
+ sizeof(ruleset_attr), 0));
+ ASSERT_EQ(EINVAL, errno);
+}
+
+TEST(invalid_quiet_bits_2)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
+ .quiet_access_fs = 1ULL << 63,
+ };
+
+ ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr,
+ sizeof(ruleset_attr), 0));
+ ASSERT_EQ(EINVAL, errno);
+}
+
TEST_HARNESS_MAIN
--
2.52.0
^ permalink raw reply related
* [PATCH v5 09/10] selftests/landlock: Add tests for quiet flag with scope
From: Tingmao Wang @ 2025-11-23 20:57 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, Justin Suess, Jan Kara,
Abhinav Saxena, linux-security-module
In-Reply-To: <cover.1763931318.git.m@maowtm.org>
Enhance scoped_audit.connect_to_child and audit_flags.signal to test
interaction with various quiet flag settings.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v4:
- New patch
tools/testing/selftests/landlock/audit_test.c | 25 ++++--
.../landlock/scoped_abstract_unix_test.c | 77 ++++++++++++++++---
2 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/landlock/audit_test.c b/tools/testing/selftests/landlock/audit_test.c
index 4417cdedeadd..818ce485ecd9 100644
--- a/tools/testing/selftests/landlock/audit_test.c
+++ b/tools/testing/selftests/landlock/audit_test.c
@@ -289,30 +289,42 @@ FIXTURE(audit_flags)
FIXTURE_VARIANT(audit_flags)
{
const int restrict_flags;
+ const __u64 quiet_scoped;
};
/* clang-format off */
FIXTURE_VARIANT_ADD(audit_flags, default) {
/* clang-format on */
.restrict_flags = 0,
+ .quiet_scoped = 0,
};
/* clang-format off */
FIXTURE_VARIANT_ADD(audit_flags, same_exec_off) {
/* clang-format on */
.restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF,
+ .quiet_scoped = 0,
};
/* clang-format off */
FIXTURE_VARIANT_ADD(audit_flags, subdomains_off) {
/* clang-format on */
.restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF,
+ .quiet_scoped = 0,
};
/* clang-format off */
FIXTURE_VARIANT_ADD(audit_flags, cross_exec_on) {
/* clang-format on */
.restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON,
+ .quiet_scoped = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(audit_flags, signal_quieted) {
+ /* clang-format on */
+ .restrict_flags = 0,
+ .quiet_scoped = LANDLOCK_SCOPE_SIGNAL,
};
FIXTURE_SETUP(audit_flags)
@@ -356,12 +368,16 @@ TEST_F(audit_flags, signal)
pid_t child;
struct audit_records records;
__u64 deallocated_dom = 2;
+ bool expect_audit = !(variant->restrict_flags &
+ LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF) &&
+ !(variant->quiet_scoped & LANDLOCK_SCOPE_SIGNAL);
child = fork();
ASSERT_LE(0, child);
if (child == 0) {
const struct landlock_ruleset_attr ruleset_attr = {
.scoped = LANDLOCK_SCOPE_SIGNAL,
+ .quiet_scoped = variant->quiet_scoped,
};
int ruleset_fd;
@@ -378,8 +394,7 @@ TEST_F(audit_flags, signal)
EXPECT_EQ(-1, kill(getppid(), 0));
EXPECT_EQ(EPERM, errno);
- if (variant->restrict_flags &
- LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF) {
+ if (!expect_audit) {
EXPECT_EQ(-EAGAIN, matches_log_signal(
_metadata, self->audit_fd,
getppid(), self->domain_id));
@@ -406,8 +421,7 @@ TEST_F(audit_flags, signal)
/* Makes sure there is no superfluous logged records. */
EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
- if (variant->restrict_flags &
- LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF) {
+ if (!expect_audit) {
EXPECT_EQ(0, records.access);
} else {
EXPECT_EQ(1, records.access);
@@ -431,8 +445,7 @@ TEST_F(audit_flags, signal)
WEXITSTATUS(status) != EXIT_SUCCESS)
_metadata->exit_code = KSFT_FAIL;
- if (variant->restrict_flags &
- LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF) {
+ if (!expect_audit) {
EXPECT_EQ(-EAGAIN,
matches_log_domain_deallocated(self->audit_fd, 0,
&deallocated_dom));
diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
index 6825082c079c..d6da9c20bde3 100644
--- a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
+++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
@@ -293,6 +293,45 @@ FIXTURE_TEARDOWN_PARENT(scoped_audit)
EXPECT_EQ(0, audit_cleanup(-1, NULL));
}
+FIXTURE_VARIANT(scoped_audit)
+{
+ const __u64 scoped;
+ const __u64 quiet_scoped;
+};
+
+// clang-format off
+FIXTURE_VARIANT_ADD(scoped_audit, no_quiet)
+{
+ // clang-format on
+ .scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET,
+ .quiet_scoped = 0,
+};
+
+// clang-format off
+FIXTURE_VARIANT_ADD(scoped_audit, quiet_abstract_socket)
+{
+ // clang-format on
+ .scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET,
+ .quiet_scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET,
+};
+
+// clang-format off
+FIXTURE_VARIANT_ADD(scoped_audit, quiet_abstract_socket_2)
+{
+ // clang-format on
+ .scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET | LANDLOCK_SCOPE_SIGNAL,
+ .quiet_scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET |
+ LANDLOCK_SCOPE_SIGNAL,
+};
+
+// clang-format off
+FIXTURE_VARIANT_ADD(scoped_audit, quiet_unrelated)
+{
+ // clang-format on
+ .scoped = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET | LANDLOCK_SCOPE_SIGNAL,
+ .quiet_scoped = LANDLOCK_SCOPE_SIGNAL,
+};
+
/* python -c 'print(b"\0selftests-landlock-abstract-unix-".hex().upper())' */
#define ABSTRACT_SOCKET_PATH_PREFIX \
"0073656C6674657374732D6C616E646C6F636B2D61627374726163742D756E69782D"
@@ -308,6 +347,13 @@ TEST_F(scoped_audit, connect_to_child)
char buf;
int dgram_client;
struct audit_records records;
+ int ruleset_fd;
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .scoped = variant->scoped,
+ .quiet_scoped = variant->quiet_scoped,
+ };
+ bool should_audit =
+ !(variant->quiet_scoped & LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
/* Makes sure there is no superfluous logged records. */
EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
@@ -345,7 +391,14 @@ TEST_F(scoped_audit, connect_to_child)
EXPECT_EQ(0, close(pipe_child[1]));
EXPECT_EQ(0, close(pipe_parent[0]));
- create_scoped_domain(_metadata, LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd)
+ {
+ TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+ }
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
/* Signals that the parent is in a domain, if any. */
ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
@@ -360,14 +413,20 @@ TEST_F(scoped_audit, connect_to_child)
EXPECT_EQ(-1, err_dgram);
EXPECT_EQ(EPERM, errno);
- EXPECT_EQ(
- 0,
- audit_match_record(
- self->audit_fd, AUDIT_LANDLOCK_ACCESS,
- REGEX_LANDLOCK_PREFIX
- " blockers=scope\\.abstract_unix_socket path=" ABSTRACT_SOCKET_PATH_PREFIX
- "[0-9A-F]\\+$",
- NULL));
+ if (should_audit) {
+ EXPECT_EQ(
+ 0,
+ audit_match_record(
+ self->audit_fd, AUDIT_LANDLOCK_ACCESS,
+ REGEX_LANDLOCK_PREFIX
+ " blockers=scope\\.abstract_unix_socket path=" ABSTRACT_SOCKET_PATH_PREFIX
+ "[0-9A-F]\\+$",
+ NULL));
+ }
+
+ /* No other logs */
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
EXPECT_EQ(0, close(dgram_client));
--
2.52.0
^ permalink raw reply related
* [PATCH v5 08/10] selftests/landlock: add tests for quiet flag with net rules
From: Tingmao Wang @ 2025-11-23 20:57 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, Justin Suess, Jan Kara,
Abhinav Saxena, linux-security-module
In-Reply-To: <cover.1763931318.git.m@maowtm.org>
Tests that:
- Quiet flag works on network rules
- Quiet flag applied to unrelated ports has no effect
- Denied access not in quiet_access_net is still logged
This is not as thorough as the fs tests, but given the shared logic it
should be sufficient. There is also no "optional" access for network
rules.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v3:
- New patch
tools/testing/selftests/landlock/net_test.c | 121 ++++++++++++++++++--
1 file changed, 111 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 2a45208551e6..0fd0ec85faff 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1871,21 +1871,22 @@ TEST_F(port_specific, bind_connect_1023)
static int matches_log_tcp(const int audit_fd, const char *const blockers,
const char *const dir_addr, const char *const addr,
- const char *const dir_port)
+ const char *const dir_port, const __u16 port)
{
static const char log_template[] = REGEX_LANDLOCK_PREFIX
- " blockers=%s %s=%s %s=1024$";
+ " blockers=%s %s=%s %s=%u$";
/*
* Max strlen(blockers): 16
* Max strlen(dir_addr): 5
* Max strlen(addr): 12
* Max strlen(dir_port): 4
+ * Max strlen(%d port): 5
*/
- char log_match[sizeof(log_template) + 37];
+ char log_match[sizeof(log_template) + 42];
int log_match_len;
log_match_len = snprintf(log_match, sizeof(log_match), log_template,
- blockers, dir_addr, addr, dir_port);
+ blockers, dir_addr, addr, dir_port, port);
if (log_match_len > sizeof(log_match))
return -E2BIG;
@@ -1895,7 +1896,8 @@ static int matches_log_tcp(const int audit_fd, const char *const blockers,
FIXTURE(audit)
{
- struct service_fixture srv0;
+ /* srv1 has a rule with no access but quiet bit set, srv0 does not. */
+ struct service_fixture srv0, srv1;
struct audit_filter audit_filter;
int audit_fd;
};
@@ -1929,6 +1931,7 @@ FIXTURE_VARIANT_ADD(audit, ipv6) {
FIXTURE_SETUP(audit)
{
ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
+ ASSERT_EQ(0, set_service(&self->srv1, variant->prot, 1));
setup_loopback(_metadata);
set_cap(_metadata, CAP_AUDIT_CONTROL);
@@ -1949,6 +1952,12 @@ TEST_F(audit, bind)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .quiet_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP |
+ LANDLOCK_ACCESS_NET_BIND_TCP,
+ };
+ const struct landlock_net_port_attr quiet_rule = {
+ .allowed_access = 0,
+ .port = self->srv1.port,
};
struct audit_records records;
int ruleset_fd, sock_fd;
@@ -1956,6 +1965,8 @@ TEST_F(audit, bind)
ruleset_fd =
landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
ASSERT_LE(0, ruleset_fd);
+ ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+ &quiet_rule, LANDLOCK_ADD_RULE_QUIET));
enforce_ruleset(_metadata, ruleset_fd);
EXPECT_EQ(0, close(ruleset_fd));
@@ -1963,11 +1974,21 @@ TEST_F(audit, bind)
ASSERT_LE(0, sock_fd);
EXPECT_EQ(-EACCES, bind_variant(sock_fd, &self->srv0));
EXPECT_EQ(0, matches_log_tcp(self->audit_fd, "net\\.bind_tcp", "saddr",
- variant->addr, "src"));
+ variant->addr, "src", self->srv0.port));
+ /* No other logs expected. */
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+
+ EXPECT_EQ(0, close(sock_fd));
+
+ sock_fd = socket_variant(&self->srv1);
+ ASSERT_LE(0, sock_fd);
+ EXPECT_EQ(-EACCES, bind_variant(sock_fd, &self->srv1));
+
+ /* No log expected due to quiet rule. */
EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
EXPECT_EQ(0, records.access);
- EXPECT_EQ(1, records.domain);
EXPECT_EQ(0, close(sock_fd));
}
@@ -1977,6 +1998,12 @@ TEST_F(audit, connect)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .quiet_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP |
+ LANDLOCK_ACCESS_NET_BIND_TCP,
+ };
+ const struct landlock_net_port_attr quiet_rule = {
+ .allowed_access = 0,
+ .port = self->srv1.port,
};
struct audit_records records;
int ruleset_fd, sock_fd;
@@ -1984,18 +2011,92 @@ TEST_F(audit, connect)
ruleset_fd =
landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
ASSERT_LE(0, ruleset_fd);
+ ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+ &quiet_rule, LANDLOCK_ADD_RULE_QUIET));
enforce_ruleset(_metadata, ruleset_fd);
EXPECT_EQ(0, close(ruleset_fd));
sock_fd = socket_variant(&self->srv0);
ASSERT_LE(0, sock_fd);
EXPECT_EQ(-EACCES, connect_variant(sock_fd, &self->srv0));
- EXPECT_EQ(0, matches_log_tcp(self->audit_fd, "net\\.connect_tcp",
- "daddr", variant->addr, "dest"));
+ EXPECT_EQ(0,
+ matches_log_tcp(self->audit_fd, "net\\.connect_tcp", "daddr",
+ variant->addr, "dest", self->srv0.port));
+
+ /* No other logs expected. */
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+
+ EXPECT_EQ(0, close(sock_fd));
+
+ sock_fd = socket_variant(&self->srv1);
+ ASSERT_LE(0, sock_fd);
+ EXPECT_EQ(-EACCES, connect_variant(sock_fd, &self->srv1));
+
+ /* Quieted - no logs expected. */
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+
+ EXPECT_EQ(0, close(sock_fd));
+}
+
+/* Quieting bind access has no effect on connect. */
+TEST_F(audit, connect_quiet_bind)
+{
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+ LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .quiet_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
+ };
+ const struct landlock_ruleset_attr ruleset_attr_2 = {
+ .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+ LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .quiet_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ };
+ const struct landlock_net_port_attr quiet_rule = {
+ .allowed_access = 0,
+ .port = self->srv1.port,
+ };
+ struct audit_records records;
+ int ruleset_fd, sock_fd;
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+ &quiet_rule, LANDLOCK_ADD_RULE_QUIET));
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ sock_fd = socket_variant(&self->srv1);
+ ASSERT_LE(0, sock_fd);
+ EXPECT_EQ(-EACCES, connect_variant(sock_fd, &self->srv1));
+ EXPECT_EQ(0,
+ matches_log_tcp(self->audit_fd, "net\\.connect_tcp", "daddr",
+ variant->addr, "dest", self->srv1.port));
+
+ /* No other logs expected. */
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+
+ EXPECT_EQ(0, close(sock_fd));
+
+ /* New layer that also denies connect but has the correct quiet bit. */
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr_2, sizeof(ruleset_attr_2), 0);
+ ASSERT_LE(0, ruleset_fd);
+ ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+ &quiet_rule, LANDLOCK_ADD_RULE_QUIET));
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ sock_fd = socket_variant(&self->srv1);
+ ASSERT_LE(0, sock_fd);
+ EXPECT_EQ(-EACCES, connect_variant(sock_fd, &self->srv1));
+ /* Quieted - no logs expected. */
EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
EXPECT_EQ(0, records.access);
- EXPECT_EQ(1, records.domain);
EXPECT_EQ(0, close(sock_fd));
}
--
2.52.0
^ permalink raw reply related
* [PATCH v5 07/10] selftests/landlock: add tests for quiet flag with fs rules
From: Tingmao Wang @ 2025-11-23 20:57 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, Justin Suess, Jan Kara,
Abhinav Saxena, linux-security-module
In-Reply-To: <cover.1763931318.git.m@maowtm.org>
Test various interactions of the quiet flag with filesystem rules:
- Non-optional access (tested with open and rename).
- Optional access (tested with truncate and ioctl).
- Behaviour around mounts matches with normal Landlock rules.
- Behaviour around disconnected directories matches with normal Landlock
rules (test expected behaviour of 9a868cdbe66a ("landlock: Fix handling of
disconnected directories") applied to the collected quiet flag).
- Multiple layers works as expected.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v5:
- Add quiet_two_layers_different_handled_{1,2,3} variants.
Changes in v3:
- New patch
tools/testing/selftests/landlock/fs_test.c | 2462 +++++++++++++++++++-
1 file changed, 2451 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 943b6e2ac53d..6aa65d344c72 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -718,11 +718,15 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
static void add_path_beneath(struct __test_metadata *const _metadata,
const int ruleset_fd, const __u64 allowed_access,
- const char *const path)
+ const char *const path, bool quiet)
{
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = allowed_access,
};
+ __u32 flags = 0;
+
+ if (quiet)
+ flags |= LANDLOCK_ADD_RULE_QUIET;
path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
ASSERT_LE(0, path_beneath.parent_fd)
@@ -731,7 +735,7 @@ static void add_path_beneath(struct __test_metadata *const _metadata,
strerror(errno));
}
ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
- &path_beneath, 0))
+ &path_beneath, flags))
{
TH_LOG("Failed to update the ruleset with \"%s\": %s", path,
strerror(errno));
@@ -786,7 +790,7 @@ static int create_ruleset(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
- rules[i].path);
+ rules[i].path, false);
}
return ruleset_fd;
}
@@ -1364,7 +1368,7 @@ TEST_F_FORK(layout1, inherit_subset)
* ANDed with the previous ones.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d2);
+ dir_s1d2, false);
/*
* According to ruleset_fd, dir_s1d2 should now have the
* LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
@@ -1396,7 +1400,7 @@ TEST_F_FORK(layout1, inherit_subset)
* Try to get more privileges by adding new access rights to the parent
* directory: dir_s1d1.
*/
- add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, false);
enforce_ruleset(_metadata, ruleset_fd);
/* Same tests and results as above. */
@@ -1419,7 +1423,7 @@ TEST_F_FORK(layout1, inherit_subset)
* that there was no rule tied to it before.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d3);
+ dir_s1d3, false);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1472,7 +1476,7 @@ TEST_F_FORK(layout1, inherit_superset)
add_path_beneath(_metadata, ruleset_fd,
LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_READ_DIR,
- dir_s1d2);
+ dir_s1d2, false);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -4211,7 +4215,7 @@ static int ioctl_error(struct __test_metadata *const _metadata, int fd,
unsigned int cmd)
{
char buf[128]; /* sufficiently large */
- int res, stdinbak_fd;
+ int res, stdinbak_fd, err;
/*
* Depending on the IOCTL command, parts of the zeroed-out buffer might
@@ -4226,13 +4230,14 @@ static int ioctl_error(struct __test_metadata *const _metadata, int fd,
/* Invokes the IOCTL with a zeroed-out buffer. */
bzero(&buf, sizeof(buf));
res = ioctl(fd, cmd, &buf);
+ err = errno;
/* Restores the old FD 0 and closes the backup FD. */
ASSERT_EQ(0, dup2(stdinbak_fd, 0));
ASSERT_EQ(0, close(stdinbak_fd));
if (res < 0)
- return errno;
+ return err;
return 0;
}
@@ -4579,6 +4584,7 @@ FIXTURE(layout1_bind) {};
static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+static const char bind_file2_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f2";
/* Move targets for disconnected path tests. */
static const char dir_s4d1[] = TMP_DIR "/s4d1";
@@ -6927,8 +6933,8 @@ static int matches_log_fs_extra(struct __test_metadata *const _metadata,
return -E2BIG;
/*
- * It is assume that absolute_path does not contain control characters nor
- * spaces, see audit_string_contains_control().
+ * It is assumed that absolute_path does not contain control
+ * characters nor spaces, see audit_string_contains_control().
*/
absolute_path = realpath(path, NULL);
if (!absolute_path)
@@ -7500,4 +7506,2438 @@ TEST_F(audit_layout1, mount)
EXPECT_EQ(1, records.domain);
}
+static bool debug_quiet_tests;
+
+FIXTURE(audit_quiet_layout1)
+{
+ struct audit_filter audit_filter;
+ int audit_fd;
+};
+
+FIXTURE_SETUP(audit_quiet_layout1)
+{
+ prepare_layout(_metadata);
+ create_layout1(_metadata);
+
+ set_cap(_metadata, CAP_AUDIT_CONTROL);
+ self->audit_fd = audit_init_with_exe_filter(&self->audit_filter);
+ EXPECT_LE(0, self->audit_fd);
+ clear_cap(_metadata, CAP_AUDIT_CONTROL);
+
+ if (getenv("DEBUG_QUIET_TESTS"))
+ debug_quiet_tests = true;
+}
+
+FIXTURE_TEARDOWN_PARENT(audit_quiet_layout1)
+{
+ remove_layout1(_metadata);
+ cleanup_layout(_metadata);
+
+ set_cap(_metadata, CAP_AUDIT_CONTROL);
+ EXPECT_EQ(0, audit_cleanup(-1, NULL));
+ clear_cap(_metadata, CAP_AUDIT_CONTROL);
+}
+
+struct a_rule {
+ const char *path;
+ __u64 access;
+ bool quiet;
+};
+
+struct a_layer {
+ __u64 handled_access_fs;
+ __u64 quiet_access_fs;
+ struct a_rule rules[6];
+ __u64 restrict_flags;
+};
+
+struct a_target {
+ /* File/dir to try open. */
+ const char *target;
+ /* Open mode (one of O_RDONLY, O_WRONLY, or O_RDWR). */
+ int open_mode;
+ /* Should open succeed? */
+ bool expect_open_success;
+ /* If open fails, whether to expect an audit log for read. */
+ bool audit_read_blocked;
+ /* If open fails, whether to expect an audit log for write. */
+ bool audit_write_blocked;
+ /* If ftruncate() is expected to be allowed. */
+ bool expect_truncate_success;
+ /* If ftruncate fails, whether to expect an audit log. */
+ bool audit_truncate;
+ /*
+ * If ioctl() is expected to be allowed (ioctl not attempted if
+ * neither this nor expect_ioctl_denied is set).
+ */
+ bool expect_ioctl_allowed;
+ /* If ioctl() is expected to be denied. */
+ bool expect_ioctl_denied;
+ /* If ioctl fails, whether to expect an audit log. */
+ bool audit_ioctl;
+};
+
+#define AUDIT_QUIET_MAX_TARGETS 10
+
+FIXTURE_VARIANT(audit_quiet_layout1)
+{
+ struct a_layer layers[3];
+ struct a_target targets[AUDIT_QUIET_MAX_TARGETS];
+};
+
+#define FS_R LANDLOCK_ACCESS_FS_READ_FILE
+#define FS_W LANDLOCK_ACCESS_FS_WRITE_FILE
+#define FS_TRUNC LANDLOCK_ACCESS_FS_TRUNCATE
+#define FS_IOCTL LANDLOCK_ACCESS_FS_IOCTL_DEV
+
+static int sprint_access_bits(char *buf, size_t buflen, __u64 access)
+{
+ size_t offset = 0;
+
+ if (buflen < strlen("rwti make_reg remove_file refer") + 1)
+ abort();
+
+ buf[0] = '\0';
+ if (access & FS_R)
+ offset += snprintf(buf + offset, buflen - offset, "r");
+ if (access & FS_W)
+ offset += snprintf(buf + offset, buflen - offset, "w");
+ if (access & FS_TRUNC)
+ offset += snprintf(buf + offset, buflen - offset, "t");
+ if (access & FS_IOCTL)
+ offset += snprintf(buf + offset, buflen - offset, "i");
+ if (access & LANDLOCK_ACCESS_FS_MAKE_REG)
+ offset += snprintf(buf + offset, buflen - offset, ",make_reg");
+ if (access & LANDLOCK_ACCESS_FS_REMOVE_FILE)
+ offset +=
+ snprintf(buf + offset, buflen - offset, ",remove_file");
+ if (access & LANDLOCK_ACCESS_FS_REFER)
+ offset += snprintf(buf + offset, buflen - offset, ",refer");
+
+ if (buf[0] == ',') {
+ offset--;
+ memmove(buf, buf + 1, offset);
+ buf[offset] = '\0';
+ }
+
+ return offset;
+}
+
+static int apply_a_layer(struct __test_metadata *const _metadata,
+ const struct a_layer *l)
+{
+ struct landlock_ruleset_attr rs_attr = {
+ .handled_access_fs = l->handled_access_fs,
+ .quiet_access_fs = l->quiet_access_fs,
+ };
+ int rs_fd;
+ int i;
+ const struct a_rule *r;
+ char handled_access_s[33], quiet_access_s[33], rule_access_s[33];
+
+ if (!l->handled_access_fs)
+ return 0;
+
+ rs_fd = landlock_create_ruleset(&rs_attr, sizeof(rs_attr), 0);
+ ASSERT_LE(0, rs_fd);
+
+ for (i = 0; i < ARRAY_SIZE(l->rules); i++) {
+ r = &l->rules[i];
+ if (!r->path)
+ continue;
+
+ add_path_beneath(_metadata, rs_fd, r->access, r->path,
+ r->quiet);
+ }
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+ ASSERT_EQ(0, landlock_restrict_self(rs_fd, l->restrict_flags))
+ {
+ TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
+ }
+ ASSERT_EQ(0, close(rs_fd));
+
+ if (debug_quiet_tests) {
+ sprint_access_bits(handled_access_s, sizeof(handled_access_s),
+ l->handled_access_fs);
+ sprint_access_bits(quiet_access_s, sizeof(quiet_access_s),
+ l->quiet_access_fs);
+ TH_LOG("applied layer: handled=%s quiet=%s restrict_flags=0x%llx",
+ handled_access_s, quiet_access_s,
+ (unsigned long long)l->restrict_flags);
+ for (i = 0; i < ARRAY_SIZE(l->rules); i++) {
+ r = &l->rules[i];
+ if (!r->path)
+ continue;
+
+ sprint_access_bits(rule_access_s, sizeof(rule_access_s),
+ r->access);
+ TH_LOG(" rule[%d]: path=%s access=%s quiet=%d", i,
+ r->path, rule_access_s, r->quiet);
+ }
+ }
+ return 0;
+}
+
+void audit_quiet_layout1_test_body(struct __test_metadata *const _metadata,
+ FIXTURE_DATA(audit_quiet_layout1) * self,
+ const struct a_target *targets)
+{
+ struct audit_records records = {};
+ int i;
+ const struct a_target *target;
+ int fd = -1;
+ int open_mode;
+ int ret;
+ bool expect_audit;
+ const char *blocker;
+
+ for (i = 0; i < AUDIT_QUIET_MAX_TARGETS; i++) {
+ target = &targets[i];
+ if (!target->target)
+ continue;
+
+ open_mode = target->open_mode & (O_RDONLY | O_WRONLY | O_RDWR);
+
+ EXPECT_TRUE(open_mode == O_RDONLY || open_mode == O_WRONLY ||
+ open_mode == O_RDWR);
+
+ if (target->expect_open_success) {
+ EXPECT_FALSE(target->audit_read_blocked);
+ EXPECT_FALSE(target->audit_write_blocked);
+ }
+ if (target->expect_truncate_success)
+ EXPECT_TRUE(target->expect_open_success &&
+ !target->audit_truncate);
+
+ if (debug_quiet_tests)
+ TH_LOG("Try open \"%s\" with %s%s", target->target,
+ open_mode != O_WRONLY ? "r" : "",
+ open_mode != O_RDONLY ? "w" : "");
+
+ fd = openat(AT_FDCWD, target->target, open_mode | O_CLOEXEC);
+ if (target->expect_open_success) {
+ ASSERT_LE(0, fd)
+ {
+ TH_LOG("Failed to open \"%s\": %s",
+ target->target, strerror(errno));
+ };
+ } else {
+ ASSERT_EQ(-1, fd);
+ ASSERT_EQ(EACCES, errno);
+ }
+
+ expect_audit = true;
+
+ if (target->audit_read_blocked && target->audit_write_blocked)
+ blocker = "fs\\.write_file,fs\\.read_file";
+ else if (target->audit_read_blocked)
+ blocker = "fs\\.read_file";
+ else if (target->audit_write_blocked)
+ blocker = "fs\\.write_file";
+ else
+ expect_audit = false;
+
+ if (expect_audit)
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ blocker, target->target));
+
+ /*
+ * Check that we see no (other) logs.
+ *
+ * We explicitly do not check records.domain here because sometimes, a
+ * domain deallocation log from a previous test (or even a previous
+ * run of the test binary when running in a loop) might run over and
+ * show up here. Since this is not a test about domain alloc/dealloc
+ * messages, we ignore them.
+ */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+
+ if (target->expect_open_success && fd >= 0) {
+ if (debug_quiet_tests)
+ TH_LOG("Try ftruncate \"%s\"", target->target);
+
+ ret = ftruncate(fd, 0);
+ if (target->expect_truncate_success) {
+ ASSERT_EQ(0, ret);
+ } else {
+ ASSERT_EQ(-1, ret);
+ if (open_mode != O_RDONLY)
+ ASSERT_EQ(EACCES, errno);
+ }
+
+ if (target->audit_truncate)
+ ASSERT_EQ(0, matches_log_fs(_metadata,
+ self->audit_fd,
+ "fs\\.truncate",
+ target->target));
+
+ if (target->expect_ioctl_allowed || target->expect_ioctl_denied) {
+ if (debug_quiet_tests)
+ TH_LOG("Try ioctl FIONREAD on \"%s\"",
+ target->target);
+
+ ret = ioctl_error(_metadata, fd, FIONREAD);
+ if (target->expect_ioctl_allowed) {
+ ASSERT_NE(EACCES, ret);
+ } else {
+ ASSERT_EQ(EACCES, ret);
+ }
+ }
+
+ if (target->audit_ioctl)
+ ASSERT_EQ(0,
+ matches_log_fs_extra(
+ _metadata, self->audit_fd,
+ "fs\\.ioctl_dev",
+ target->target,
+ " ioctlcmd=0x541b\\+"));
+
+ /* No other logs. records.domain not checked per reasoning above. */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+ ASSERT_EQ(0, close(fd));
+ }
+ }
+}
+
+TEST_F(audit_quiet_layout1, base)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &variant->layers[i]));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant->targets);
+}
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_simple) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ { .path = dir_s1d1, .access = 0, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ /* Access not quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .audit_write_blocked = true,
+ },
+ /*
+ * Quiet flag only takes effect if all blocked access bits are
+ * quieted, otherwise audit log emitted as normal (with all blockers)
+ */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_allow_read) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_W,
+ .rules = {
+ { .path = dir_s1d1, .access = FS_R, .quiet = true },
+ /* Quiet flags inherit down and is not overridden */
+ { .path = file1_s1d1, .access = FS_R, .quiet = false },
+ { .path = file1_s2d3, .access = 0, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ /* Read ok */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ },
+ /* Write quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ },
+ /* Read allowed, write quieted so no audit */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s2d2,
+ .open_mode = O_WRONLY,
+ .audit_write_blocked = true,
+ },
+ {
+ .target = file1_s2d2,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ /* Single file quiet */
+ {
+ .target = file1_s2d3,
+ .open_mode = O_WRONLY,
+ },
+ /* Wrong file */
+ {
+ .target = file2_s2d3,
+ .open_mode = O_WRONLY,
+ .audit_write_blocked = true,
+ },
+ /* Access not quieted */
+ {
+ .target = file1_s2d3,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ /* Some access not quieted */
+ {
+ .target = file1_s2d3,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_allow_write) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ { .path = dir_s1d1, .access = FS_W, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ /* Read quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ /* Truncate not quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ /* Write allowed, read quieted so no audit */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, allow_write_quiet_trunc) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_TRUNC,
+ .rules = {
+ { .path = dir_s1d1, .access = FS_W, .quiet = true },
+ { .path = dir_s2d1, .access = FS_W, .quiet = false },
+ },
+ },
+ },
+ .targets = {
+ /* Read not allowed and not quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ /* Truncate quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ },
+ /* Not covered by quiet (truncate) */
+ {
+ .target = file1_s2d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ /* Not covered by quiet (read/write) */
+ {
+ .target = file1_s3d1,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, allow_rw_quiet_trunc) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_TRUNC,
+ .rules = {
+ { .path = dir_s1d1, .access = FS_R | FS_W, .quiet = true },
+ { .path = dir_s2d1, .access = FS_R | FS_W, .quiet = false },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_all) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ { .path = dir_s1d1, .access = 0, .quiet = true },
+ { .path = file1_s2d1, .access = FS_R | FS_W, .quiet = true },
+ { .path = file1_s2d3, .access = 0, .quiet = true },
+ { .path = dir_s3d1, .access = FS_W, .quiet = false },
+ { .path = "/dev/zero", .access = FS_R, .quiet = false },
+ { .path = "/dev/null", .access = FS_R, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ /* No logs */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ /* Truncate quieted - no log */
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ },
+ /* Truncate not covered by quiet */
+ {
+ .target = file1_s3d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s3d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ /* Single file quiet */
+ {
+ .target = file1_s2d3,
+ .open_mode = O_RDWR,
+ },
+ /* Wrong file */
+ {
+ .target = file2_s2d3,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ /* Ioctl quieted */
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ /* Ioctl not quieted */
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ .audit_ioctl = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_across_mountpoint) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ { .path = dir_s3d1, .access = 0, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s3d3,
+ .open_mode = O_RDONLY,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ .audit_write_blocked = true,
+ },
+ /* Access not quieted */
+ {
+ .target = file1_s3d3,
+ .open_mode = O_WRONLY,
+ .audit_write_blocked = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, allow_all_quiet) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = true
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = true
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ .expect_truncate_success = true,
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_allowed = true,
+ },
+ },
+};
+
+/*
+ * With LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF, it doesn't matter what
+ * the quiet flags below the layer says
+ */
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, subdomains_off) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R,
+ .restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF,
+ .rules = {
+ { .path = "/", .access = FS_R, .quiet = false },
+ }
+ },
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ { .path = dir_s1d1, .access = 0, .quiet = true },
+ { .path = file1_s2d2, .access = FS_R | FS_W, .quiet = true },
+ { .path = file1_s2d3, .access = FS_R | FS_W, .quiet = false },
+ { .path = "/dev/null", .access = FS_R | FS_W, .quiet = true },
+ { .path = "/dev/zero", .access = FS_R | FS_W, .quiet = false },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file1_s2d2,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ /* No audit_truncate */
+ },
+ {
+ .target = file1_s2d3,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ /* No audit_truncate */
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ /* No audit_ioctl */
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ /* No audit_ioctl */
+ },
+ },
+};
+
+/*
+ * With LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF, it doesn't matter what
+ * the quiet flags on the layer says
+ */
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, same_exec_off) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R,
+ .restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF,
+ .rules = {
+ { .path = dir_s1d1, .access = 0, .quiet = true },
+ { .path = file1_s2d2, .access = FS_R | FS_W, .quiet = true },
+ { .path = file1_s2d3, .access = FS_R | FS_W, .quiet = false },
+ { .path = "/dev/null", .access = FS_R | FS_W, .quiet = true },
+ { .path = "/dev/zero", .access = FS_R | FS_W, .quiet = false },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file1_s2d2,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ /* No audit_truncate */
+ },
+ {
+ .target = file1_s2d3,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ /* No audit_truncate */
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ /* No audit_ioctl */
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ /* No audit_ioctl */
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_1) {
+ /* Here, rules that deny access is always quiet. */
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R,
+ .quiet = true,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R,
+ .quiet = true,
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_2) {
+ /* Here, rules that deny access is never quiet. */
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_W,
+ .quiet = false
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = true
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R,
+ .quiet = false
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = true
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = true
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_W,
+ .quiet = false
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = true
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R,
+ .quiet = false
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ .audit_ioctl = true,
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ .audit_ioctl = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_3) {
+ /* This time only the second layer quiets things. */
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_W,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R,
+ .quiet = true,
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ .audit_ioctl = true,
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_different_quiet_access) {
+ /* Here, rules that deny access is always quiet. */
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R,
+ .quiet = true,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_IOCTL,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = FS_R | FS_W | FS_TRUNC,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = "/dev/null",
+ .access = FS_R | FS_W | FS_IOCTL,
+ .quiet = false,
+ },
+ {
+ .path = "/dev/zero",
+ .access = FS_R,
+ .quiet = true,
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ {
+ .target = file1_s2d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .audit_truncate = true,
+ },
+ {
+ .target = "/dev/null",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ {
+ .target = "/dev/zero",
+ .open_mode = O_RDONLY,
+ .expect_open_success = true,
+ .expect_ioctl_denied = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_different_handled_1) {
+ /* Quiet from layer 1 */
+ .layers = {
+ {
+ .handled_access_fs = FS_R,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_R,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file1_s1d2,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d2,
+ .access = FS_R,
+ .quiet = true,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_W,
+ .quiet_access_fs = FS_W,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_W,
+ .quiet = false,
+ },
+ /* Nothing for file2_s1d1 */
+ {
+ .path = file1_s1d2,
+ .access = FS_W,
+ .quiet = false,
+ },
+ /* Nothing for file2_s1d2 */
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ .expect_truncate_success = true,
+ },
+ /* Missing both, youngest layer denies write, not quiet */
+ {
+ .target = file2_s1d1,
+ .open_mode = O_RDWR,
+ .audit_write_blocked = true,
+ },
+ /* Missing read, denied and quieted by layer 1 */
+ {
+ .target = file1_s1d2,
+ .open_mode = O_RDWR,
+ },
+ /* Missing write, denied and not quieted by layer 2 */
+ {
+ .target = file2_s1d2,
+ .open_mode = O_RDWR,
+ .audit_write_blocked = true,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_different_handled_2) {
+ /* Quiet from layer 2 */
+ .layers = {
+ {
+ .handled_access_fs = FS_R,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_R,
+ .quiet = false,
+ },
+ /* Nothing for file2_s1d1 and file1_s1d2 */
+ {
+ .path = file2_s1d2,
+ .access = FS_R,
+ .quiet = false,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_W,
+ .quiet_access_fs = FS_W,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file1_s1d2,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d2,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ .expect_truncate_success = true,
+ },
+ /* Missing both, youngest layer denies write, quiet */
+ {
+ .target = file2_s1d1,
+ .open_mode = O_RDWR,
+ },
+ /* Missing read, denied and not quieted by layer 1 */
+ {
+ .target = file1_s1d2,
+ .open_mode = O_RDWR,
+ .audit_read_blocked = true,
+ },
+ /* Missing write, denied and quieted by layer 2 */
+ {
+ .target = file2_s1d2,
+ .open_mode = O_RDWR,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, quiet_two_layers_different_handled_3) {
+ /* Quiet from both layers */
+ .layers = {
+ {
+ .handled_access_fs = FS_R,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_R,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file1_s1d2,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d2,
+ .access = FS_R,
+ .quiet = true,
+ },
+ },
+ },
+ {
+ .handled_access_fs = FS_W,
+ .quiet_access_fs = FS_W,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file1_s1d2,
+ .access = FS_W,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d2,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ },
+ },
+ .targets = {
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ .expect_open_success = true,
+ .expect_truncate_success = true,
+ },
+ {
+ .target = file2_s1d1,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file1_s1d2,
+ .open_mode = O_RDWR,
+ },
+ {
+ .target = file2_s1d2,
+ .open_mode = O_RDWR,
+ },
+ },
+};
+
+FIXTURE_VARIANT_ADD(audit_quiet_layout1, without_quiet_then_with_quiet) {
+ .layers = {
+ {
+ .handled_access_fs = FS_R | FS_W,
+ .quiet_access_fs = FS_R,
+ .rules = {
+ { .path = dir_s1d1, .access = FS_W, .quiet = false },
+ { .path = dir_s1d1, .access = 0, .quiet = true },
+ },
+ },
+ },
+ .targets = {
+ /* Read denied and quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDONLY,
+ },
+ /* Write ok */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_WRONLY,
+ .expect_open_success = true,
+ .expect_truncate_success = true,
+ },
+ /* Write ok, read denied and quieted */
+ {
+ .target = file1_s1d1,
+ .open_mode = O_RDWR,
+ },
+ /* Not covered by quiet */
+ {
+ .target = file1_s2d1,
+ .open_mode = O_RDONLY,
+ .audit_read_blocked = true,
+ },
+ },
+};
+
+/*
+ * The following TEST_F extend the above test cases to test more layers,
+ * with the inserted layers having varying configurations.
+ */
+
+/* Extra allow all layers, quiet or not, does not change any behaviour. */
+TEST_F(audit_quiet_layout1, allow_all_layer)
+{
+ struct a_layer allow_all_layer = {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = 0,
+ .rules = {
+ {
+ .path = "/",
+ .access = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet = false,
+ },
+ },
+ };
+ int i;
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &allow_all_layer));
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &variant->layers[i]));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant->targets);
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &allow_all_layer));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant->targets);
+
+ /*
+ * SELF_LOG flags or quiet bits from inner allowing layers should not
+ * affect behaviour.
+ */
+ allow_all_layer.quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL;
+ allow_all_layer.rules[0].quiet = true;
+ /*
+ * Note: this only works because we're not checking counts of domain
+ * alloc/dealloc logs
+ */
+ allow_all_layer.restrict_flags =
+ LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF |
+ LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF;
+ ASSERT_EQ(0, apply_a_layer(_metadata, &allow_all_layer));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant->targets);
+}
+
+/*
+ * Add useless outer layers until we reach the layer limit. Should not
+ * change anything.
+ */
+TEST_F(audit_quiet_layout1, many_outer_layers)
+{
+ struct a_layer useless_layer = {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC,
+ .rules = {
+ { .path = "/", .access = FS_R | FS_W | FS_TRUNC, .quiet = true },
+ },
+ };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++) {
+ if (variant->layers[i].handled_access_fs == 0)
+ break;
+ }
+
+ for (; i < LANDLOCK_MAX_NUM_LAYERS; i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &useless_layer));
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &variant->layers[i]));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant->targets);
+}
+
+/*
+ * An inner layer that denies and quiets everything should result in no
+ * logs.
+ */
+TEST_F(audit_quiet_layout1, deny_all_quiet_layer)
+{
+ struct a_layer deny_all_layer = {
+ .handled_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .quiet_access_fs = FS_R | FS_W | FS_TRUNC | FS_IOCTL,
+ .rules = {
+ { .path = "/", .access = 0, .quiet = true },
+ },
+ };
+ int i;
+ FIXTURE_VARIANT(audit_quiet_layout1) variant_2 = {};
+
+ /* Any open should fail with no logs. */
+ for (i = 0; i < ARRAY_SIZE(variant->targets); i++) {
+ const struct a_target *target = &variant->targets[i];
+
+ variant_2.targets[i] = (struct a_target){
+ .target = target->target,
+ .open_mode = target->open_mode,
+ /* We denied everything, open should always fail. */
+ .expect_open_success = false,
+ };
+ }
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &variant->layers[i]));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &deny_all_layer));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant_2.targets);
+}
+
+/*
+ * An inner layer that denies everything without quiet should produce logs
+ * for all access.
+ */
+TEST_F(audit_quiet_layout1, deny_all_layer)
+{
+ struct a_layer deny_all_layer = {
+ .handled_access_fs = FS_R | FS_W,
+ .quiet_access_fs = FS_R | FS_W,
+ };
+ int i;
+ FIXTURE_VARIANT(audit_quiet_layout1) variant_2 = {};
+ bool test_has_subdomains_off = false;
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++) {
+ if (variant->layers[i].restrict_flags &
+ LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF) {
+ test_has_subdomains_off = true;
+ break;
+ }
+ }
+
+ for (i = 0; i < ARRAY_SIZE(variant->targets); i++) {
+ const struct a_target *target = &variant->targets[i];
+
+ variant_2.targets[i] = (struct a_target){
+ .target = target->target,
+ .open_mode = target->open_mode,
+
+ /* We denied everything, open should always fail. */
+ .expect_open_success = false,
+ /* Audit should always happen as long as open request contains read. */
+ .audit_read_blocked = !test_has_subdomains_off &&
+ target->open_mode != O_WRONLY,
+ /* Audit should always happen as long as open request contains write. */
+ .audit_write_blocked = !test_has_subdomains_off &&
+ target->open_mode != O_RDONLY,
+ };
+ }
+
+ for (i = 0; i < ARRAY_SIZE(variant->layers); i++)
+ ASSERT_EQ(0, apply_a_layer(_metadata, &variant->layers[i]));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &deny_all_layer));
+
+ audit_quiet_layout1_test_body(_metadata, self, variant_2.targets);
+}
+
+/* Uses layout1_bind hierarchy */
+FIXTURE(audit_quiet_rename)
+{
+ struct audit_filter audit_filter;
+ int audit_fd;
+};
+
+FIXTURE_SETUP(audit_quiet_rename)
+{
+ prepare_layout(_metadata);
+ create_layout1(_metadata);
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0, mount(dir_s1d2, dir_s2d2, NULL, MS_BIND, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+
+ set_cap(_metadata, CAP_AUDIT_CONTROL);
+ self->audit_fd = audit_init_with_exe_filter(&self->audit_filter);
+ EXPECT_LE(0, self->audit_fd);
+ clear_cap(_metadata, CAP_AUDIT_CONTROL);
+
+ if (getenv("DEBUG_QUIET_TESTS"))
+ debug_quiet_tests = true;
+}
+
+FIXTURE_TEARDOWN_PARENT(audit_quiet_rename)
+{
+ remove_layout1(_metadata);
+ cleanup_layout(_metadata);
+
+ /* umount(dir_s2d2)) is handled by namespace lifetime. */
+
+ remove_path(file1_s4d1);
+ remove_path(file2_s4d1);
+
+ set_cap(_metadata, CAP_AUDIT_CONTROL);
+ EXPECT_EQ(0, audit_cleanup(-1, NULL));
+ clear_cap(_metadata, CAP_AUDIT_CONTROL);
+}
+
+static void simple_quiet_rename(struct __test_metadata *const _metadata,
+ FIXTURE_DATA(audit_quiet_rename) *const self,
+ __u64 handled_access, __u64 quiet_access,
+ bool source_allow, bool dest_allow,
+ bool source_quiet, bool dest_quiet,
+ const char *source_blockers,
+ const char *dest_blockers)
+{
+ /* We will move file1_s1d1 to file1_s2d1 */
+ struct a_layer layer = {
+ .handled_access_fs = handled_access,
+ .quiet_access_fs = quiet_access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = source_allow ? handled_access : 0,
+ .quiet = source_quiet,
+ },
+ {
+ .path = dir_s2d1,
+ .access = dest_allow ? handled_access : 0,
+ .quiet = dest_quiet,
+ },
+ },
+ };
+ struct audit_records records = {};
+ int ret, err;
+
+ /* Skip landlock_add_rule for useless rules. */
+ if (!source_allow && !source_quiet)
+ layer.rules[0].path = NULL;
+ if (!dest_allow && !dest_quiet)
+ layer.rules[1].path = NULL;
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+ EXPECT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ if (debug_quiet_tests)
+ TH_LOG("Try renameat \"%s\" to \"%s\"", file1_s1d1, file1_s2d1);
+ ret = renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1);
+ err = errno;
+ if (ret != 0 && debug_quiet_tests) {
+ TH_LOG("renameat error: %s", err == EXDEV ? "EXDEV" :
+ err == EACCES ? "EACCES" :
+ strerror(err));
+ }
+ if (source_allow && dest_allow) {
+ ASSERT_EQ(0, ret);
+ } else {
+ ASSERT_EQ(-1, ret);
+ if (handled_access & (LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE)) {
+ ASSERT_EQ(EACCES, err);
+ } else {
+ ASSERT_EQ(EXDEV, err);
+ }
+
+ if (source_blockers)
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ source_blockers, dir_s1d1));
+ if (dest_blockers)
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ dest_blockers, dir_s2d1));
+ }
+ /*
+ * No other logs. records.domain not checked per reasoning in
+ * audit_quiet_layout1_test_body.
+ */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, rename_ok)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, true, true, false,
+ false, NULL, NULL);
+}
+
+TEST_F(audit_quiet_rename, no_quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, false, false,
+ false, false, "fs\\.remove_file,fs\\.refer",
+ "fs\\.make_reg,fs\\.refer");
+}
+
+TEST_F(audit_quiet_rename, quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, false, false, true,
+ true, NULL, NULL);
+}
+
+TEST_F(audit_quiet_rename, source_no_quiet_dest_quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, false, false,
+ false, true, "fs\\.remove_file,fs\\.refer", NULL);
+}
+
+TEST_F(audit_quiet_rename, source_quiet_dest_no_quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, false, false, true,
+ false, NULL, "fs\\.make_reg,fs\\.refer");
+}
+
+TEST_F(audit_quiet_rename, only_quiet_refer)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, LANDLOCK_ACCESS_FS_REFER,
+ false, false, true, true,
+ "fs\\.remove_file,fs\\.refer",
+ "fs\\.make_reg,fs\\.refer");
+}
+
+TEST_F(audit_quiet_rename, source_allow_dest_quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, true, false, false,
+ true, NULL, NULL);
+}
+
+TEST_F(audit_quiet_rename, source_quiet_dest_allow)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+
+ simple_quiet_rename(_metadata, self, access, access, false, true, true,
+ false, NULL, NULL);
+}
+
+TEST_F(audit_quiet_rename, handle_all_deny_quiet_refer)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_REFER,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EXDEV, errno);
+
+ /* No logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, handle_all_deny_not_quiet_refer)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = 0,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = false,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EXDEV, errno);
+
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd, "fs\\.refer",
+ dir_s1d1));
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd, "fs\\.refer",
+ dir_s2d1));
+
+ /* No other logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, handle_all_deny_refer_quiet_source_not_quiet_dest)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_REFER,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ .quiet = false,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EXDEV, errno);
+
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd, "fs\\.refer",
+ dir_s2d1));
+
+ /* No other logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_same_dir)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file2_s1d1));
+ ASSERT_EQ(EACCES, errno);
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_flag_on_file_ignored)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file1_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.refer", dir_s1d1));
+ /* We didn't unlink destination file */
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.make_reg,fs\\.refer", dir_s2d1));
+
+ /* No other logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_flag_on_file_ignored_same_dir)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = file1_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = file2_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file2_s1d1));
+ ASSERT_EQ(EACCES, errno);
+
+ ASSERT_EQ(0,
+ matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.make_reg", dir_s1d1));
+
+ /* No other logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, two_layers_different_quiet1)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer1 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = access,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct a_layer layer2 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_REFER,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = access,
+ .quiet = false,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer2));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * The youngest denial will be layer 2. Refer is quieted but we are
+ * also missing remove_file on source.
+ */
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.refer", dir_s1d1));
+ /* No other logs */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, two_layers_different_quiet2)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer1 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = access,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct a_layer layer2 = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_REFER,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_REFER,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = LANDLOCK_ACCESS_FS_REFER,
+ .quiet = false,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer2));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * The youngest denial will be layer 2, but refer is quieted (and that
+ * layer does not handle any other accesses).
+ */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, two_layers_different_quiet3)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer1 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = access,
+ .quiet = false,
+ },
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct a_layer layer2 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = access,
+ .quiet = false,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer2));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * The youngest denial will be layer 2, in which everything is
+ * quieted.
+ */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, first_layer_quiet_deny_all_second_layer_not_quiet_deny_all)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer1 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct a_layer layer2 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {},
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer2));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.refer", dir_s1d1));
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.make_reg,fs\\.refer", dir_s2d1));
+ /* No other logs. */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, first_layer_quiet_deny_all_second_layer_dest_not_quiet)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer1 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct a_layer layer2 = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file1_s2d1));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer1));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer2));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * Source is quieted but destination is not.
+ */
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.make_reg,fs\\.refer", dir_s2d1));
+ /* No other logs. */
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, rename_xchg)
+{
+ struct a_layer layer = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER,
+ .quiet_access_fs = LANDLOCK_ACCESS_FS_MAKE_REG,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER,
+ .quiet = true,
+ },
+ {
+ .path = dir_s2d1,
+ .access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER,
+ .quiet = false,
+ }
+ },
+ };
+ struct audit_records records = {};
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d1,
+ RENAME_EXCHANGE));
+ ASSERT_EQ(EACCES, errno);
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_on_parent_mount)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file2_s1d3));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, bind_file1_s1d3, AT_FDCWD, bind_file2_s1d3));
+ ASSERT_EQ(EACCES, errno);
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_behind_mountpoint_ignored)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s1d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+
+ EXPECT_EQ(0, unlink(file2_s1d3));
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(AT_FDCWD, bind_file1_s1d3, AT_FDCWD, bind_file2_s1d3));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(0,
+ matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.make_reg", bind_dir_s1d3));
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_on_parent_mount_disconnected)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s2d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+ int bind_s1d3_fd;
+
+ EXPECT_EQ(0, unlink(file2_s1d3));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_DIRECTORY);
+ ASSERT_GE(bind_s1d3_fd, 0);
+
+ /* Make s1d3 disconnected. */
+ create_directory(_metadata, dir_s4d1);
+ ASSERT_EQ(0, renameat(AT_FDCWD, dir_s1d3, AT_FDCWD, dir_s4d2));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name));
+ ASSERT_EQ(EACCES, errno);
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
+TEST_F(audit_quiet_rename, quiet_behind_mountpoint_ignored_disconnected)
+{
+ __u64 access = LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REFER;
+ struct a_layer layer = {
+ .handled_access_fs = access,
+ .quiet_access_fs = access,
+ .rules = {
+ {
+ .path = dir_s4d1,
+ .access = 0,
+ .quiet = true,
+ },
+ },
+ };
+ struct audit_records records = {};
+ int bind_s1d3_fd;
+
+ EXPECT_EQ(0, unlink(file2_s1d3));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_DIRECTORY);
+ ASSERT_GE(bind_s1d3_fd, 0);
+
+ /* Make s1d3 disconnected. */
+ create_directory(_metadata, dir_s4d1);
+ ASSERT_EQ(0, renameat(AT_FDCWD, dir_s1d3, AT_FDCWD, dir_s4d2));
+
+ ASSERT_EQ(0, apply_a_layer(_metadata, &layer));
+
+ ASSERT_EQ(-1, renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Disconnected paths are logged as "/". */
+ ASSERT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.remove_file,fs\\.make_reg", "/"));
+
+ audit_count_records(self->audit_fd, &records);
+ ASSERT_EQ(0, records.access);
+}
+
TEST_HARNESS_MAIN
--
2.52.0
^ permalink raw reply related
* [PATCH v5 06/10] selftests/landlock: Replace hard-coded 16 with a constant
From: Tingmao Wang @ 2025-11-23 20:57 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, Justin Suess, Jan Kara,
Abhinav Saxena, linux-security-module
In-Reply-To: <cover.1763931318.git.m@maowtm.org>
The next commit will reuse this number. Make it a shared constant to
future-proof changes.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v3:
- New patch
tools/testing/selftests/landlock/audit_test.c | 2 +-
tools/testing/selftests/landlock/common.h | 2 ++
tools/testing/selftests/landlock/fs_test.c | 2 +-
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/landlock/audit_test.c b/tools/testing/selftests/landlock/audit_test.c
index 46d02d49835a..4417cdedeadd 100644
--- a/tools/testing/selftests/landlock/audit_test.c
+++ b/tools/testing/selftests/landlock/audit_test.c
@@ -76,7 +76,7 @@ TEST_F(audit, layers)
.scoped = LANDLOCK_SCOPE_SIGNAL,
};
int status, ruleset_fd, i;
- __u64(*domain_stack)[16];
+ __u64(*domain_stack)[LANDLOCK_MAX_NUM_LAYERS];
__u64 prev_dom = 3;
pid_t child;
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 230b75f6015b..719326f9e8f0 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -25,6 +25,8 @@
/* TEST_F_FORK() should not be used for new tests. */
#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)
+#define LANDLOCK_MAX_NUM_LAYERS 16
+
static const char bin_sandbox_and_launch[] = "./sandbox-and-launch";
static const char bin_wait_pipe[] = "./wait-pipe";
static const char bin_wait_pipe_sandbox[] = "./wait-pipe-sandbox";
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21dd95aaf5e4..943b6e2ac53d 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -1497,7 +1497,7 @@ TEST_F_FORK(layout0, max_layers)
const int ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
ASSERT_LE(0, ruleset_fd);
- for (i = 0; i < 16; i++)
+ for (i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++)
enforce_ruleset(_metadata, ruleset_fd);
for (i = 0; i < 2; i++) {
--
2.52.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox