* Re: [PATCH] KEYS: encrypted: Use pr_fmt()
From: Jarkko Sakkinen @ 2025-11-21 20:10 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <83C83079-0354-4642-A980-DBC7AE572A53@linux.dev>
On Wed, Nov 19, 2025 at 03:45:02PM +0100, Thorsten Blum wrote:
> On 19. Nov 2025, at 03:48, Jarkko Sakkinen wrote:
> > On Thu, Nov 13, 2025 at 01:35:44PM +0100, Thorsten Blum wrote:
> >> Use pr_fmt() to automatically prefix all pr_<level>() log messages with
> >
> > This fails to describe what "use" means.
>
> I don't understand what you mean. What's wrong with "use ... to ..."?
I think e.g., "Rewrite the definition of ..." describes better what
you're doing.
>
> >> "encrypted_key: " and remove all manually added prefixes.
> >>
> >> Reformat the code accordingly and avoid line breaks in log messages.
> >>
> >> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >> ---
> >> security/keys/encrypted-keys/encrypted.c | 74 +++++++++++-------------
> >> security/keys/encrypted-keys/encrypted.h | 2 +-
> >> 2 files changed, 35 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> >> index 513c09e2b01c..a8e8bf949b4b 100644
> >> --- a/security/keys/encrypted-keys/encrypted.c
> >> +++ b/security/keys/encrypted-keys/encrypted.c
> >> @@ -11,6 +11,8 @@
> >> * See Documentation/security/keys/trusted-encrypted.rst
> >> */
> >>
> >
> > Should have undef prepending.
>
> Why is this necessary when the #define is at the top of a source file?
> The kernel documentation [1] doesn't mention this anywhere. Isn't #undef
> only needed when redefining 'pr_fmt' in the middle of a file to avoid a
> compiler warning/error?
>
> >> +#define pr_fmt(fmt) "encrypted_key: " fmt
> >> +
> >> [...]
>
> Thanks,
> Thorsten
>
> [1] https://docs.kernel.org/core-api/printk-basics.html
>
BR, Jarkko
^ permalink raw reply
* [GIT PULL] selinux/selinux-pr-20251121
From: Paul Moore @ 2025-11-21 20:57 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Three SELinux patches for v6.18 to fix issues around accessing the
per-task decision cache that we introduced in v6.16 to help reduce
SELinux overhead on path walks. The problem was that despite the
cache being located in the SELinux "task_security_struct", the
parent struct wasn't actually tied to the task, it was tied to a cred.
Historically SELinux did locate the task_security_struct in the
task_struct's security blob, but it was later relocated to the cred
struct when the cred work happened, as it made the most sense at the
time. Unfortunately we never did the task_security_struct to
cred_security_struct rename work (avoid code churn maybe? who knows)
because it didn't really matter at the time. However, it suddenly
became a problem when we added a per-task cache to a per-cred object
and didn't notice because of the old, no-longer-correct struct naming.
Thanks to KCSAN for flagging this, as the silly humans running
things forgot that the task_security_struct was a big lie.
This pull request contains three patches, only one of which actually
fixes the problem described above and moves the SELinux decision cache
from the per-cred struct to a newly (re)created per-task struct. The
other two patches, which form the bulk of the diffstat, take care of
the associated renaming tasks so we can hopefully avoid making the
same stupid mistake in the future. For the record, I did contemplate
sending just a fix for the cache, leaving the renaming patches for the
upcoming merge window, but the type/variable naming ended up being
pretty awful and would have made v6.18 an outlier stuck between the
"old" names and the "new" names in v6.19. The renaming patches are
also fairly mechanical/trivial and shouldn't pose much risk despite
their size.
TLDR; naming things may be hard, but if you mess it up bad things
happen.
Paul
--
The following changes since commit 211ddde0823f1442e4ad052a2f30f050145ccada:
Linux 6.18-rc2 (2025-10-19 15:19:16 -1000)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20251121
for you to fetch changes up to 3ded250b97c3ae94a642bc2e710a95700e72dfb0:
selinux: rename the cred_security_struct variables to "crsec"
(2025-11-20 16:47:50 -0500)
----------------------------------------------------------------
selinux/stable-6.18 PR 20251121
----------------------------------------------------------------
Paul Moore (1):
selinux: rename the cred_security_struct variables to "crsec"
Stephen Smalley (2):
selinux: rename task_security_struct to cred_security_struct
selinux: move avdcache to per-task security struct
security/selinux/hooks.c | 251 +++++++++++++++---------------
security/selinux/include/objsec.h | 22 +-
2 files changed, 144 insertions(+), 129 deletions(-)
--
paul-moore.com
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Ryan Lee @ 2025-11-21 23:06 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Bernd Edlinger, 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, apparmor
In-Reply-To: <87ikf3w5us.fsf@email.froward.int.ebiederm.org>
On Fri, Nov 21, 2025 at 11:20 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>
> > On 11/21/25 10:35, Bernd Edlinger wrote:
> >> On 11/21/25 08:18, Eric W. Biederman wrote:
> >>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>>
> >>>> Hi Eric,
> >>>>
> >>>> thanks for you valuable input on the topic.
> >>>>
> >>>> On 11/21/25 00:50, Eric W. Biederman wrote:
> >>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> >>>>>
> >>>>>> 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.
> >>>>>
> >>>>> I should add that the permission checks happen in open_exec,
> >>>>> everything that follows credential wise is just about representing in
> >>>>> struct cred the credentials the new executable will have.
> >>>>>
> >>>>> So I am really at a loss why we have had this complicated way of
> >>>>> computing of computed the credentials all of these years full of
> >>>>> time of check to time of use problems.
> >>>>>
> >>>>
> >>>> Well, I think I see a problem with your patch:
> >>>>
> >>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
> >>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
> >>>> or the smack security engines at least. Previously that callback
> >>>> was called before the point of no return, and the return code should
> >>>> be returned as a return code the the caller of execve. But if we move
> >>>> that check after the point of no return, the caller will get killed
> >>>> due to the failed security check.
> >>>>
> >>>> Or did I miss something?
> >>>
> >>> I think we definitely need to document this change in behavior. I would
> >>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
> >>> issue. The exec is failing one way or the other so I don't see it as a
> >>> correctness issue.
> >>>
> >>> In the case of ptrace in general I think it is a bug if the mere act of
> >>> debugging a program changes it's behavior. So which buggy behavior
> >>> should we prefer? SIGSEGV where it is totally clear that the behavior
> >>> has changed or -EPERM and ask the debugged program to handle it.
> >>> I lean towards SIGSEGV because then it is clear the code should not
> >>> handle it.
> >>>
> >>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
> >>> handle unexpected things happening is to terminate the application.
> >>>
> >>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
> >>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
> >>> of CLONE_THREAD.
> >>>
> >>>
> >>> Plus all of these things are only considerations if we are exec'ing a
> >>> program that transitions to a different set of credentials. Something
> >>> that happens but is quite rare itself.
AppArmor's exec rules rely heavily on transitioning to different creds
on exec. For example, an AppArmor policy like
profile example_1 /usr/bin/example_1 {
/usr/bin/example_2 Px -> example_2_profile,
/usr/bin/example_3 Px,
}
will allow binary example_1 to execute binaries example_2 and
example_3, launching those processes under a different confinement
(example_2_profile and a profile that attaches to /usr/bin/example_3,
respectively). We will need to look into how much this patch (or a
corresponding change in behavior) would affect our use case, but
confinement transitions (where the confinement information is stored
as an LSM blob on the cred struct) are extremely common in a system
that uses AppArmor as an LSM.
> >>>
> >>> In practice I don't expect there is anything that depends on the exact
> >>> behavior of what happens when exec'ing a suid executable to gain
> >>> privileges when ptraced. The closes I can imagine is upstart and
> >>> I think upstart ran as root when ptracing other programs so there is no
> >>> gaining of privilege and thus no reason for a security module to
> >>> complain.
> >>>
> >>> Who knows I could be wrong, and someone could actually care. Which is
> >>> hy I think we should document it.>>
> >>
> >>
> >> Well, I dont know for sure, but the security engine could deny the execution
> >> for any reason, not only because of being ptraced.
> >> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
> >>
> >>
> >> Bernd.
> >>
> >
> > Hmm, funny..
> >
> > I installed this patch on top of
> >
> > commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
> > Merge: c966813ea120 7b6216baae75
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date: Thu Nov 20 11:04:37 2025 -0800
> >
> > but it does panic when I try to boot:
> >
> > [ 0.870539] TERM=1inux
> > [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
> > [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
> > [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
> > [ 0.870580] Call Trace:
> > [ 0.870590] <TASK>
> > [ 0.870592] vpanic+0x36d/0x380
> > [ 0.870607] ? __pfx_kernel_init+0x10/0x10
> > [ 0.870615] panic+0x5b/0x60
> > [ 0.870617] kernel_init+0x17d/0x1c0
> > [ 0.870623] ret_from_fork+0x124/0x150
> > [ 0.870625} ? __pfx_kernel_init+0x10/0x10
> > [ 0.870627] ret_from_fork_asm+0x1a/0x30
> > [ 0.870632] </TASK>
> > [ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
> > [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
> >
> >
> > Is that a known problem?
>
> Nope. It looks like the code needs a little bit bug fixing testing.
>
> I will take see about taking a look.
>
> Eric
>
I've also CC'ed the AppArmor mailing list on this patch to facilitate
discussion if, upon further investigation, this patch would require
changes or cause other problems on the AppArmor side.
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Günther Noack @ 2025-11-22 10:16 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-13-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index e22e10edb103..d1a004c2e0f5 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
> }
> }
>
> +static int test_socketpair(int family, int type, int protocol)
> +{
> + int fds[2];
> + int err;
> +
> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
> + if (err)
> + return errno;
> + /*
> + * Mixing error codes from close(2) and socketpair(2) should not lead to
> + * any (access type) confusion for this test.
> + */
> + if (close(fds[0]) != 0)
> + return errno;
> + if (close(fds[1]) != 0)
> + return errno;
Very minor nit: the function leaks an FD if it returns early after the
first close() call failed. (Highly unlikely to happen though.)
> + return 0;
> +}
> +
> +TEST_F(mini, socketpair)
> +{
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_socket_attr unix_socket_create = {
> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> + .family = AF_UNIX,
> + .type = SOCK_STREAM,
> + .protocol = 0,
> + };
> + int ruleset_fd;
> +
> + /* Tries to create socket when ruleset is not established. */
> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> + 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_SOCKET,
> + &unix_socket_create, 0));
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is allowed */
> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
You may want to check that landlock_create_ruleset() succeeded here:
ASSERT_LE(0, ruleset_fd)
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is restricted. */
> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +}
> +
> TEST_HARNESS_MAIN
> --
> 2.34.1
>
Otherwise, looks good.
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Mikhail Ivanov @ 2025-11-22 10:21 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.4795c4c3bb03@gnoack.org>
On 11/22/2025 1:16 PM, Günther Noack wrote:
> On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index e22e10edb103..d1a004c2e0f5 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
>> }
>> }
>>
>> +static int test_socketpair(int family, int type, int protocol)
>> +{
>> + int fds[2];
>> + int err;
>> +
>> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
>> + if (err)
>> + return errno;
>> + /*
>> + * Mixing error codes from close(2) and socketpair(2) should not lead to
>> + * any (access type) confusion for this test.
>> + */
>> + if (close(fds[0]) != 0)
>> + return errno;
>> + if (close(fds[1]) != 0)
>> + return errno;
>
> Very minor nit: the function leaks an FD if it returns early after the
> first close() call failed. (Highly unlikely to happen though.)
Yeah, but AFAIK fd[0] may be leaked anyway if close() fails. Anyway
this shouldn't be an issue for tests.
>
>> + return 0;
>> +}
>> +
>> +TEST_F(mini, socketpair)
>> +{
>> + const struct landlock_ruleset_attr ruleset_attr = {
>> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + };
>> + const struct landlock_socket_attr unix_socket_create = {
>> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + .family = AF_UNIX,
>> + .type = SOCK_STREAM,
>> + .protocol = 0,
>> + };
>> + int ruleset_fd;
>> +
>> + /* Tries to create socket when ruleset is not established. */
>> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> + 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_SOCKET,
>> + &unix_socket_create, 0));
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is allowed */
>> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +
>> + ruleset_fd =
>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>
> You may want to check that landlock_create_ruleset() succeeded here:
>
> ASSERT_LE(0, ruleset_fd)
thanks, I'll fix it.
>
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is restricted. */
>> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +}
>> +
>> TEST_HARNESS_MAIN
>> --
>> 2.34.1
>>
>
> Otherwise, looks good.
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Günther Noack @ 2025-11-22 10:49 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-2-ivanov.mikhail1@huawei-partners.com>
Hello!
On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
> It is possible to create sockets of the same protocol with different
> protocol number values. For example, TCP sockets can be created using one
> of the following commands:
> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
> protocol of the given protocol family and can be mapped to another
> value.
>
> Socket rules do not perform such mappings to not increase complexity
> of rules definition and their maintenance.
Minor phrasing nit: Maybe we can phrase this constructively, like
"rules operate on the socket(2) parameters as they are passed by the
user, before this mapping happens"?
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f030adc462ee..030c96cb5d25 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
> * flags`_).
> */
> __u64 handled_access_net;
> + /**
> + * @handled_access_socket: Bitmask of handled actions performed on sockets
> + * (cf. `Socket flags`).
> + */
> + __u64 handled_access_socket;
This struct can only be extended at the end, for ABI compatibility reasons.
In the call to landlock_create_ruleset(2), the user passes the __user
pointer to this struct along with its size (as known to the user at
compile time). When we copy this into the kernel, we blank out the
struct and only copy the prefix of the caller-supplied size. The
implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
When you rearrange the order, please also update it in other places
where these fields are mentioned next to each other, for
consistency. I'll try to point it out where I see it in the review,
but I might miss some places.
> /**
> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> * restricting a Landlock domain from accessing outside
> @@ -140,6 +145,11 @@ enum landlock_rule_type {
> * landlock_net_port_attr .
> */
> LANDLOCK_RULE_NET_PORT,
> + /**
> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
> + * landlock_socket_attr.
^
Nit: Adjacent documentation has a space before the dot.
I assume this is needed for kernel doc formatting?
> + */
> + LANDLOCK_RULE_SOCKET,
> };
>
> /**
> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
> __u64 port;
> };
>
> +/**
> + * struct landlock_socket_attr - Socket protocol definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_socket_attr {
> + /**
> + * @allowed_access: Bitmask of allowed access for a socket protocol
> + * (cf. `Socket flags`_).
> + */
> + __u64 allowed_access;
> + /**
> + * @family: Protocol family used for communication
> + * (cf. include/linux/socket.h).
> + */
> + __s32 family;
> + /**
> + * @type: Socket type (cf. include/linux/net.h)
> + */
> + __s32 type;
> + /**
> + * @protocol: Communication protocol specific to protocol family set in
> + * @family field.
This is specific to both the @family and the @type, not just the @family.
From socket(2):
Normally only a single protocol exists to support a particular
socket type within a given protocol family.
For instance, in your commit message above the protocol in the example
is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
with SOCK_DGRAM.
> + */
> + __s32 protocol;
> +} __attribute__((packed));
Since we are in the UAPI header, please also document the wildcard
values for @type and @protocol.
(Remark, should those be exposed as constants?)
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d..03ccd6fbfe83 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> /* Makes sure all network access rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all socket access rights can be stored. */
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
> /* Makes sure all scoped rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> struct access_masks {
> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
> access_mask_t scope : LANDLOCK_NUM_SCOPE;
(Please re-adjust field order for consistency with UAPI)
> };
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index dfcdc19ea268..a34d2dbe3954 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> return new_ruleset;
> }
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t fs_access_mask,
> - const access_mask_t net_access_mask,
> - const access_mask_t scope_mask)
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
(Please re-adjust field order for consistency with UAPI)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask && !net_access_mask && !scope_mask)
> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
> + !scope_mask)
(Please re-adjust field order for consistency with UAPI)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (IS_ERR(new_ruleset))
> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> if (net_access_mask)
> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> + if (socket_access_mask)
> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
> + 0);
(Please re-adjust order of these "if"s for consistency with UAPI)
> if (scope_mask)
> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
> return new_ruleset;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 1a78cba662b2..a60ede2fc2a5 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -189,10 +204,9 @@ struct landlock_ruleset {
> };
> };
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t access_mask_fs,
> - const access_mask_t access_mask_net,
> - const access_mask_t scope_mask);
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
(Please re-adjust field order for consistency with UAPI)
> index 000000000000..28a80dcad629
> --- /dev/null
> +++ b/security/landlock/socket.c
> @@ -0,0 +1,105 @@
> [...]
> +#define TYPE_ALL (-1)
> +#define PROTOCOL_ALL (-1)
Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 33eafb71e4f3..e9f500f97c86 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -101,9 +104,10 @@ static void build_check_abi(void)
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> ruleset_size += sizeof(ruleset_attr.handled_access_net);
> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
> ruleset_size += sizeof(ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
> [...]
> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> LANDLOCK_MASK_ACCESS_NET)
> return -EINVAL;
>
> + /* Checks socket content (and 32-bits cast). */
> + if ((ruleset_attr.handled_access_socket |
> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
> + return -EINVAL;
> +
> /* Checks IPC scoping content (and 32-bits cast). */
> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> return -EINVAL;
> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> /* Checks arguments and transforms to kernel struct. */
> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> ruleset_attr.handled_access_net,
> + ruleset_attr.handled_access_socket,
> ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
> [...]
> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
> * handled accesses)
> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
> + * &landlock_socket_attr.protocol is greater than 65534;
Hmm, this is a bit annoying that these values have such unusual
bounds, even though the input parameters are 32 bit. We are exposing
a little bit that we are internally storing this with only 8 and 16
bits... (I don't know a better solution immediately either, though. I
think we discussed this on a previous version of the patch set as well
and ended up with permitting larger values than the narrower SOCK_MAX
etc bounds.)
> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
> * 0);
> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
> return add_rule_path_beneath(ruleset, rule_attr);
> case LANDLOCK_RULE_NET_PORT:
> return add_rule_net_port(ruleset, rule_attr);
> + case LANDLOCK_RULE_SOCKET:
> + return add_rule_socket(ruleset, rule_attr);
> default:
> return -EINVAL;
> }
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Mikhail Ivanov @ 2025-11-22 11:13 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.e645d2f1b8a1@gnoack.org>
On 11/22/2025 1:49 PM, Günther Noack wrote:
> Hello!
>
> On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
>> It is possible to create sockets of the same protocol with different
>> protocol number values. For example, TCP sockets can be created using one
>> of the following commands:
>> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
>> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
>> protocol of the given protocol family and can be mapped to another
>> value.
>>
>> Socket rules do not perform such mappings to not increase complexity
>> of rules definition and their maintenance.
>
> Minor phrasing nit: Maybe we can phrase this constructively, like
> "rules operate on the socket(2) parameters as they are passed by the
> user, before this mapping happens"?
OK, thats sounds good.
>
>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f030adc462ee..030c96cb5d25 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
>> * flags`_).
>> */
>> __u64 handled_access_net;
>> + /**
>> + * @handled_access_socket: Bitmask of handled actions performed on sockets
>> + * (cf. `Socket flags`).
>> + */
>> + __u64 handled_access_socket;
>
> This struct can only be extended at the end, for ABI compatibility reasons.
>
> In the call to landlock_create_ruleset(2), the user passes the __user
> pointer to this struct along with its size (as known to the user at
> compile time). When we copy this into the kernel, we blank out the
> struct and only copy the prefix of the caller-supplied size. The
> implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
Indeed... Thanks for pointing on this, I'll move this field in the end
of the structure.
>
> When you rearrange the order, please also update it in other places
> where these fields are mentioned next to each other, for
> consistency. I'll try to point it out where I see it in the review,
> but I might miss some places.
ok
>
>> /**
>> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
>> * restricting a Landlock domain from accessing outside
>> @@ -140,6 +145,11 @@ enum landlock_rule_type {
>> * landlock_net_port_attr .
>> */
>> LANDLOCK_RULE_NET_PORT,
>> + /**
>> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
>> + * landlock_socket_attr.
> ^
>
> Nit: Adjacent documentation has a space before the dot.
> I assume this is needed for kernel doc formatting?
Probably, I'll fix this anyway.
>
>> + */
>> + LANDLOCK_RULE_SOCKET,
>> };
>>
>> /**
>> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
>> __u64 port;
>> };
>>
>> +/**
>> + * struct landlock_socket_attr - Socket protocol definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_socket_attr {
>> + /**
>> + * @allowed_access: Bitmask of allowed access for a socket protocol
>> + * (cf. `Socket flags`_).
>> + */
>> + __u64 allowed_access;
>> + /**
>> + * @family: Protocol family used for communication
>> + * (cf. include/linux/socket.h).
>> + */
>> + __s32 family;
>> + /**
>> + * @type: Socket type (cf. include/linux/net.h)
>> + */
>> + __s32 type;
>> + /**
>> + * @protocol: Communication protocol specific to protocol family set in
>> + * @family field.
>
> This is specific to both the @family and the @type, not just the @family.
>
>>From socket(2):
>
> Normally only a single protocol exists to support a particular
> socket type within a given protocol family.
>
> For instance, in your commit message above the protocol in the example
> is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
> with SOCK_DGRAM.
You're right.
>
>> + */
>> + __s32 protocol;
>> +} __attribute__((packed));
>
> Since we are in the UAPI header, please also document the wildcard
> values for @type and @protocol.
I'll add the description, thanks!
>
> (Remark, should those be exposed as constants?)
I thought it could overcomplicate socket rules definition and Landlock
API. Do you think introducing such constants will be better decision?
>
>
>> diff --git a/security/landlock/access.h b/security/landlock/access.h
>> index 7961c6630a2d..03ccd6fbfe83 100644
>> --- a/security/landlock/access.h
>> +++ b/security/landlock/access.h
>> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> /* Makes sure all network access rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>> +/* Makes sure all socket access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
>> /* Makes sure all scoped rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
>> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> struct access_masks {
>> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
>> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
>> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
>> access_mask_t scope : LANDLOCK_NUM_SCOPE;
>
> (Please re-adjust field order for consistency with UAPI)
ok, will be fixed in all such places.
>
>> };
>
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index dfcdc19ea268..a34d2dbe3954 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> return new_ruleset;
>> }
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask,
>> - const access_mask_t net_access_mask,
>> - const access_mask_t scope_mask)
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
>> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> {
>> struct landlock_ruleset *new_ruleset;
>>
>> /* Informs about useless ruleset. */
>> - if (!fs_access_mask && !net_access_mask && !scope_mask)
>> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
>> + !scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> return ERR_PTR(-ENOMSG);
>> new_ruleset = create_ruleset(1);
>> if (IS_ERR(new_ruleset))
>> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>> if (net_access_mask)
>> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> + if (socket_access_mask)
>> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
>> + 0);
>
> (Please re-adjust order of these "if"s for consistency with UAPI)
>
>> if (scope_mask)
>> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
>> return new_ruleset;
>
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 1a78cba662b2..a60ede2fc2a5 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -189,10 +204,9 @@ struct landlock_ruleset {
>> };
>> };
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t access_mask_fs,
>> - const access_mask_t access_mask_net,
>> - const access_mask_t scope_mask);
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
>> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> index 000000000000..28a80dcad629
>> --- /dev/null
>> +++ b/security/landlock/socket.c
>> @@ -0,0 +1,105 @@
>> [...]
>> +#define TYPE_ALL (-1)
>> +#define PROTOCOL_ALL (-1)
>
> Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
answered above.
>
>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 33eafb71e4f3..e9f500f97c86 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -101,9 +104,10 @@ static void build_check_abi(void)
>> */
>> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>> ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>> ruleset_size += sizeof(ruleset_attr.scoped);
> (Please re-adjust field order for consistency with UAPI)
>
>> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
>> [...]
>
>> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> LANDLOCK_MASK_ACCESS_NET)
>> return -EINVAL;
>>
>> + /* Checks socket content (and 32-bits cast). */
>> + if ((ruleset_attr.handled_access_socket |
>> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
>> + return -EINVAL;
>> +
>> /* Checks IPC scoping content (and 32-bits cast). */
>> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
>> return -EINVAL;
>> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> /* Checks arguments and transforms to kernel struct. */
>> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>> ruleset_attr.handled_access_net,
>> + ruleset_attr.handled_access_socket,
>> ruleset_attr.scoped);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> if (IS_ERR(ruleset))
>> return PTR_ERR(ruleset);
>> [...]
>
>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>> * handled accesses)
>> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>> + * &landlock_socket_attr.protocol is greater than 65534;
>
> Hmm, this is a bit annoying that these values have such unusual
> bounds, even though the input parameters are 32 bit. We are exposing
> a little bit that we are internally storing this with only 8 and 16
> bits... (I don't know a better solution immediately either, though. I
> think we discussed this on a previous version of the patch set as well
> and ended up with permitting larger values than the narrower SOCK_MAX
> etc bounds.)
I agree, one of the possible solutions may be to store larger values in
socket keys (eg. s32), but this would require to make a separate
interface for storing socket rules (in order to not change key size for
other type of rules which is currently 32-64 bit depending on virtual
address size).
>
>> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
>> * 0);
>> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
>> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>> return add_rule_path_beneath(ruleset, rule_attr);
>> case LANDLOCK_RULE_NET_PORT:
>> return add_rule_net_port(ruleset, rule_attr);
>> + case LANDLOCK_RULE_SOCKET:
>> + return add_rule_socket(ruleset, rule_attr);
>> default:
>> return -EINVAL;
>> }
>
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 06/19] landlock: Add hook on socket creation
From: Günther Noack @ 2025-11-22 11:41 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-7-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:26PM +0800, Mikhail Ivanov wrote:
> Add hook on security_socket_create(), which checks whether the socket
> of requested protocol is allowed by domain.
>
> Due to support of masked protocols Landlock tries to find one of the
> 4 rules that can allow creation of requested protocol.
>
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> Changes since v3:
> * Changes LSM hook from socket_post_create to socket_create so
> creation would be blocked before socket allocation and initialization.
> * Uses credential instead of domain in hook_socket create.
> * Removes get_raw_handled_socket_accesses.
> * Adds checks for rules with wildcard type and protocol values.
> * Minor refactoring, fixes.
>
> Changes since v2:
> * Adds check in `hook_socket_create()` to not restrict kernel space
> sockets.
> * Inlines `current_check_access_socket()` in the `hook_socket_create()`.
> * Fixes commit message.
>
> Changes since v1:
> * Uses lsm hook arguments instead of struct socket fields as family-type
> values.
> * Packs socket family and type using helper.
> * Fixes commit message.
> * Formats with clang-format.
> ---
> security/landlock/setup.c | 2 +
> security/landlock/socket.c | 78 ++++++++++++++++++++++++++++++++++++++
> security/landlock/socket.h | 2 +
> 3 files changed, 82 insertions(+)
>
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index bd53c7a56ab9..140a53b022f7 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -17,6 +17,7 @@
> #include "fs.h"
> #include "id.h"
> #include "net.h"
> +#include "socket.h"
> #include "setup.h"
> #include "task.h"
>
> @@ -68,6 +69,7 @@ static int __init landlock_init(void)
> landlock_add_task_hooks();
> landlock_add_fs_hooks();
> landlock_add_net_hooks();
> + landlock_add_socket_hooks();
> landlock_init_id();
> landlock_initialized = true;
> pr_info("Up and running.\n");
> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
> index 28a80dcad629..d7e6e7b92b7a 100644
> --- a/security/landlock/socket.c
> +++ b/security/landlock/socket.c
> @@ -103,3 +103,81 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>
> return err;
> }
> +
> +static int check_socket_access(const struct landlock_ruleset *dom,
> + uintptr_t key,
> + layer_mask_t (*const layer_masks)[],
> + access_mask_t handled_access)
> +{
> + const struct landlock_rule *rule;
> + struct landlock_id id = {
> + .type = LANDLOCK_KEY_SOCKET,
> + };
> +
> + id.key.data = key;
This line can be made part of the designated initializer:
struct landlock_id id = {
.type = ...,
.key.data = ...,
};
> + rule = landlock_find_rule(dom, id);
> + if (landlock_unmask_layers(rule, handled_access, layer_masks,
> + LANDLOCK_NUM_ACCESS_SOCKET))
> + return 0;
> + return -EACCES;
> +}
> +
> +static int hook_socket_create(int family, int type, int protocol, int kern)
> +{
> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
> + access_mask_t handled_access;
> + const struct access_masks masks = {
> + .socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_cred_security *const subject =
> + landlock_get_applicable_subject(current_cred(), masks, NULL);
> + uintptr_t key;
> +
> + if (!subject)
> + return 0;
> + /* Checks only user space sockets. */
> + if (kern)
> + return 0;
> +
> + handled_access = landlock_init_layer_masks(
> + subject->domain, LANDLOCK_ACCESS_SOCKET_CREATE, &layer_masks,
> + LANDLOCK_KEY_SOCKET);
Nit: I had to double check to confirm that the same PF_INET/PF_PACKET
transformation (which net/socket.c refers to as the "uglymoron") has
already happened on the arguments before hook_socket_create() gets
called from there. Maybe it's worth a brief mention in a comment
here.
> + /*
> + * Error could happen due to parameters are outside of the allowed range,
Grammar nit: drop the "are"
Suggestion: "If this error happens, the parameters are outside of the
allowed range, so this combination can't have been added to the
ruleset previously."
> + * so this combination couldn't be added in ruleset previously.
> + * Therefore, it's not permitted.
> + */
> + if (pack_socket_key(family, type, protocol, &key) == -EACCES)
> + return -EACCES;
BUG: pack_socket_key() does never return -EACCES!
(Consider whether that function should really return an error? Maybe
a boolean would be better, if you anyway need a different error code
in both locations where it is called.)
Can this code path actually get hit, or do the entry points for
creating sockets refuse these wrong values at an earlier stage with
EINVAL already?
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + /* Ranges were already checked. */
> + (void)pack_socket_key(family, TYPE_ALL, protocol, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, type, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, TYPE_ALL, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + return -EACCES;
> +}
It initially doesn't look very nice to drop the error from
pack_socket_key() repeatedly. The call repeats the bounds checks and
requires more cross-function reasoning to understand.
Since 'key' is an uintptr_t anyway, and the wildcards are all ones,
maybe a simpler way is to define masks for the wildcards?
const uintptr_t any_type_mask = (union key){.data.type = UINT8_MAX}.packed;
const uintptr_t any_protocol_mask = (union key){.data.protocol = UINT16_MAX}.packed;
and then, after calling pack_socket_key() once with error check, use
the combinations
* key
* key | any_type
* key | any_protocol
* key | any_type | any_protocol
to construct the wildcard-enabled keys in the four calls to
check_socket_access()? You could have compile-time assertions or
tests to check that the masking does the same as packing it from
scratch when passing -1.
(That being said, I don't feel strongly about it.)
Remark on the side: I was briefly confused why we don't need to guard
on CONFIG_SECURITY_NETWORK, but this is already required by
CONFIG_LANDLOCK. So that looks good.
–Günther
^ permalink raw reply
* [PATCH RESEND] apparmor: replace sprintf with snprintf in aa_new_learning_profile
From: Thorsten Blum @ 2025-11-22 11:54 UTC (permalink / raw)
To: John Johansen, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, apparmor, linux-security-module, linux-kernel
Replace unbounded sprintf() calls with snprintf() to prevent potential
buffer overflows in aa_new_learning_profile(). While the current code
works correctly, snprintf() is safer and follows secure coding best
practices. No functional changes.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/policy.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 50d5345ff5cb..b09323867fea 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -697,24 +697,27 @@ struct aa_profile *aa_new_learning_profile(struct aa_profile *parent, bool hat,
struct aa_profile *p, *profile;
const char *bname;
char *name = NULL;
+ size_t name_sz;
AA_BUG(!parent);
if (base) {
- name = kmalloc(strlen(parent->base.hname) + 8 + strlen(base),
- gfp);
+ name_sz = strlen(parent->base.hname) + 8 + strlen(base);
+ name = kmalloc(name_sz, gfp);
if (name) {
- sprintf(name, "%s//null-%s", parent->base.hname, base);
+ snprintf(name, name_sz, "%s//null-%s",
+ parent->base.hname, base);
goto name;
}
/* fall through to try shorter uniq */
}
- name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, gfp);
+ name_sz = strlen(parent->base.hname) + 2 + 7 + 8;
+ name = kmalloc(name_sz, gfp);
if (!name)
return NULL;
- sprintf(name, "%s//null-%x", parent->base.hname,
- atomic_inc_return(&parent->ns->uniq_null));
+ snprintf(name, name_sz, "%s//null-%x", parent->base.hname,
+ atomic_inc_return(&parent->ns->uniq_null));
name:
/* lookup to see if this is a dup creation */
--
2.51.1
^ permalink raw reply related
* [PATCH RESEND] apparmor: Replace sprintf/strcpy with scnprintf/strscpy in aa_policy_init
From: Thorsten Blum @ 2025-11-22 11:55 UTC (permalink / raw)
To: John Johansen, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, apparmor, linux-security-module, linux-kernel
strcpy() is deprecated and sprintf() does not perform bounds checking
either. Although an overflow is unlikely, it's better to proactively
avoid it by using the safer strscpy() and scnprintf(), respectively.
Additionally, unify memory allocation for 'hname' to simplify and
improve aa_policy_init().
Link: https://github.com/KSPP/linux/issues/88
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/lib.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index 82dbb97ad406..acf7f5189bec 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -478,19 +478,17 @@ bool aa_policy_init(struct aa_policy *policy, const char *prefix,
const char *name, gfp_t gfp)
{
char *hname;
+ size_t hname_sz;
+ hname_sz = (prefix ? strlen(prefix) + 2 : 0) + strlen(name) + 1;
/* freed by policy_free */
- if (prefix) {
- hname = aa_str_alloc(strlen(prefix) + strlen(name) + 3, gfp);
- if (hname)
- sprintf(hname, "%s//%s", prefix, name);
- } else {
- hname = aa_str_alloc(strlen(name) + 1, gfp);
- if (hname)
- strcpy(hname, name);
- }
+ hname = aa_str_alloc(hname_sz, gfp);
if (!hname)
return false;
+ if (prefix)
+ scnprintf(hname, hname_sz, "%s//%s", prefix, name);
+ else
+ strscpy(hname, name, hname_sz);
policy->hname = hname;
/* base.name is a substring of fqname */
policy->name = basename(policy->hname);
--
2.51.1
^ permalink raw reply related
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Günther Noack @ 2025-11-22 12:18 UTC (permalink / raw)
To: Mikhail Ivanov, mic
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <af464773-b01b-f3a4-474d-0efb2cfae142@huawei-partners.com>
On Sat, Nov 22, 2025 at 02:13:08PM +0300, Mikhail Ivanov wrote:
> On 11/22/2025 1:49 PM, Günther Noack wrote:
> > (Remark, should those be exposed as constants?)
>
> I thought it could overcomplicate socket rules definition and Landlock
> API. Do you think introducing such constants will be better decision?
No, I am not convinced either. FWIW, there is a bit of prior art for
"wildcard-like" -1 constants (grepping include/uapi for 'define.*-1'),
but then again, the places where people did the opposite are hard to
grep for. I would also be OK if we documented "-1" in that place and
left out the constant.
Mickaël, maybe you have a preference for the API style here?
> > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > > index 33eafb71e4f3..e9f500f97c86 100644
> > > --- a/security/landlock/syscalls.c
> > > +++ b/security/landlock/syscalls.c
> > > @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
> > > * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
> > > * handled accesses)
> > > * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
> > > + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
> > > + * &landlock_socket_attr.protocol is greater than 65534;
> >
> > Hmm, this is a bit annoying that these values have such unusual
> > bounds, even though the input parameters are 32 bit. We are exposing
> > a little bit that we are internally storing this with only 8 and 16
> > bits... (I don't know a better solution immediately either, though. I
> > think we discussed this on a previous version of the patch set as well
> > and ended up with permitting larger values than the narrower SOCK_MAX
> > etc bounds.)
>
> I agree, one of the possible solutions may be to store larger values in
> socket keys (eg. s32), but this would require to make a separate
> interface for storing socket rules (in order to not change key size for
> other type of rules which is currently 32-64 bit depending on virtual
> address size).
Yes, I'd be OK with it.
Do I remember this correctly that we settled on enforcing the looser
UINT8_MAX and UINT16_MAX instead of SOCK_MAX, AF_MAX, which we used in
v3 and before? I tried to find the conversation but could not find it
any more. (Or did you have other reasons why you switched the
implementation to use these larger bounds?)
Thanks,
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Mikhail Ivanov @ 2025-11-22 16:51 UTC (permalink / raw)
To: Günther Noack, mic
Cc: gnoack, willemdebruijn.kernel, matthieu, linux-security-module,
netdev, netfilter-devel, yusongping, artem.kuzin,
konstantin.meskhidze
In-Reply-To: <20251122.d391a246d7dd@gnoack.org>
On 11/22/2025 3:18 PM, Günther Noack wrote:
> On Sat, Nov 22, 2025 at 02:13:08PM +0300, Mikhail Ivanov wrote:
>> On 11/22/2025 1:49 PM, Günther Noack wrote:
>>> (Remark, should those be exposed as constants?)
>>
>> I thought it could overcomplicate socket rules definition and Landlock
>> API. Do you think introducing such constants will be better decision?
>
> No, I am not convinced either. FWIW, there is a bit of prior art for
> "wildcard-like" -1 constants (grepping include/uapi for 'define.*-1'),
> but then again, the places where people did the opposite are hard to
> grep for. I would also be OK if we documented "-1" in that place and
> left out the constant.
>
> Mickaël, maybe you have a preference for the API style here?
>
>
>>>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>>>> index 33eafb71e4f3..e9f500f97c86 100644
>>>> --- a/security/landlock/syscalls.c
>>>> +++ b/security/landlock/syscalls.c
>>>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>>>> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>>>> * handled accesses)
>>>> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>>>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>>>> + * &landlock_socket_attr.protocol is greater than 65534;
>>>
>>> Hmm, this is a bit annoying that these values have such unusual
>>> bounds, even though the input parameters are 32 bit. We are exposing
>>> a little bit that we are internally storing this with only 8 and 16
>>> bits... (I don't know a better solution immediately either, though. I
>>> think we discussed this on a previous version of the patch set as well
>>> and ended up with permitting larger values than the narrower SOCK_MAX
>>> etc bounds.)
>>
>> I agree, one of the possible solutions may be to store larger values in
>> socket keys (eg. s32), but this would require to make a separate
>> interface for storing socket rules (in order to not change key size for
>> other type of rules which is currently 32-64 bit depending on virtual
>> address size).
>
> Yes, I'd be OK with it.
>
> Do I remember this correctly that we settled on enforcing the looser
> UINT8_MAX and UINT16_MAX instead of SOCK_MAX, AF_MAX, which we used in
> v3 and before? I tried to find the conversation but could not find it
> any more. (Or did you have other reasons why you switched the
> implementation to use these larger bounds?)
Mickaël mentioned that Landlock should accept rules defined even for
unsupported protocol families:
https://lore.kernel.org/all/20241128.um9voo5Woo3I@digikod.net/
>
> Thanks,
> –Günther
^ permalink raw reply
* Re: [PATCH v18] exec: Fix dead-lock in de_thread with ptrace_attach
From: Bernd Edlinger @ 2025-11-22 17:10 UTC (permalink / raw)
To: Eric W. Biederman
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, Ryan Lee, apparmor, selinux
In-Reply-To: <87wm3ky5n9.fsf@email.froward.int.ebiederm.org>
On 11/20/25 18:29, Eric W. Biederman wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> This introduces signal->exec_bprm, which is used to
>>> fix the case when at least one of the sibling threads
>>> is traced, and therefore the trace process may dead-lock
>>> in ptrace_attach, but de_thread will need to wait for the
>>> tracer to continue execution.
>>
>> A small quibble it isn't a dead lock. It isn't even really a live lock,
>> as it is possible to SIGKILL our way out.
>>
That is of course what I meant to say with that sentence. In my language an
application process can "dead-lock" by waiting on a mutex forever.
Indeed the original problem with the dead-lock (I think in mm_access) was
worse, as both involved processes were only killable by "kill -9", but with the
remaining problem in ptrace_attach, the debugger can be killed by a simple CTRL-C.
So if I understand you right, you want me use a different term here like
"the trace process may be blocked in trace_attach" or so?
Or could you please give me a concrete suggestion how to rephrase the patch
description.
BTW, unless there are objections I would also want to rephrase the description
of cred_guard_mutex to replace the term "Deprecated" with "Not recommended",
like this:
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -260,11 +260,11 @@ struct signal_struct {
struct mutex cred_guard_mutex; /* guard against foreign influences on
* credential calculations
* (notably. ptrace)
* Held while execve runs, except when
* a sibling thread is being traced.
- * Deprecated do not use in new code.
+ * Not recommended to use in new code.
* Use exec_update_lock instead.
*/
>> Thinking about this there is a really silly and simple way we can deal
>> with this situation for PTRACE_ATTACH. We can send SIGSTOP and wait for
>> the thread to stop before doing anything with cred_guard_mutex.
>>
>> PTRACE_ATTACH already implies sending SIGSTOP so as long as we have
>> enough permissions to send SIGSTOP I don't see that being a problem.
>>
>> The worst case I can see is that we get a case where we stop the
>> process, the permission check fails under cred_guard_mutex and
>> and ptrace attach has fails and has to send SIGCONT to undo it's
>> premature SIGSTOP. That might almost be visible, but it would still
>> be legitimate because we can still check that we have permission to
>> send SIGSTOP.
>
> Bah no I am full of it.
>
> The challenging behavior is in the semantics of the kernel operations.
> We need to describe it as such please.
>
> It is the same class of problem as a single threaded process calls exec
> with a pipe attached to both stdin and stdout of the new process.
>
> For the stdin and stdout we can say just use pull and nonblocking I/O.
>
> The problem is that both PTRACE_ATTACH and PTRACE_SEIZE block over
> the duration of exec, and if exec is waiting for a thread to exit,
> and that thread is blocked in PTRACE_EVENT_EXIT waiting for that very
> same tracer those processes will hang. Not deadlock.
>
>
> I haven't seen anyone clearly describe the problem lately so I am
> repeating it.
>
>
> Just looking at the code I don't think there is any fundamental reason
> to call commit_creds after de_thread. If we can change that we can sort
> this out without any change in userspace semantics.
>
> If we can't move commit_creds we have to either give
> PTRACE_ATTACH/PTRACE_SEIZE a non-block mode, or break out of
> PTRACE_EVENT_EXIT in de_thread.
>
> I will post a proof of concept of moving commit_creds in just a minute.
>
> Eric
Note: I forgot to add apparmor and selinux mailing list to this patch, previous
versions of this did try to avoid to touch the security engine code, and did
instead temporarily install the new credentials, mostiy for the benefit of the
security engines. But that is considered an unacceptable solution, therefore
I want to use instead a new option to ptrace_may_access.
All security engines have to handle this option, but the advantage is, that the
engines could detect and maybe also deny the unsafe execve.
This is an alternative to Eric's patch: "exec: Move cred computation under
exec_update_lock" that is supposed to solve the same problem, but tries instead
to avoid any user visible API change.
Thanks
Bernd.
^ permalink raw reply
* Re: [RFC PATCH v4 06/19] landlock: Add hook on socket creation
From: Mikhail Ivanov @ 2025-11-22 17:19 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.78c6cd69a873@gnoack.org>
On 11/22/2025 2:41 PM, Günther Noack wrote:
> On Tue, Nov 18, 2025 at 09:46:26PM +0800, Mikhail Ivanov wrote:
>> Add hook on security_socket_create(), which checks whether the socket
>> of requested protocol is allowed by domain.
>>
>> Due to support of masked protocols Landlock tries to find one of the
>> 4 rules that can allow creation of requested protocol.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>> Changes since v3:
>> * Changes LSM hook from socket_post_create to socket_create so
>> creation would be blocked before socket allocation and initialization.
>> * Uses credential instead of domain in hook_socket create.
>> * Removes get_raw_handled_socket_accesses.
>> * Adds checks for rules with wildcard type and protocol values.
>> * Minor refactoring, fixes.
>>
>> Changes since v2:
>> * Adds check in `hook_socket_create()` to not restrict kernel space
>> sockets.
>> * Inlines `current_check_access_socket()` in the `hook_socket_create()`.
>> * Fixes commit message.
>>
>> Changes since v1:
>> * Uses lsm hook arguments instead of struct socket fields as family-type
>> values.
>> * Packs socket family and type using helper.
>> * Fixes commit message.
>> * Formats with clang-format.
>> ---
>> security/landlock/setup.c | 2 +
>> security/landlock/socket.c | 78 ++++++++++++++++++++++++++++++++++++++
>> security/landlock/socket.h | 2 +
>> 3 files changed, 82 insertions(+)
>>
>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
>> index bd53c7a56ab9..140a53b022f7 100644
>> --- a/security/landlock/setup.c
>> +++ b/security/landlock/setup.c
>> @@ -17,6 +17,7 @@
>> #include "fs.h"
>> #include "id.h"
>> #include "net.h"
>> +#include "socket.h"
>> #include "setup.h"
>> #include "task.h"
>>
>> @@ -68,6 +69,7 @@ static int __init landlock_init(void)
>> landlock_add_task_hooks();
>> landlock_add_fs_hooks();
>> landlock_add_net_hooks();
>> + landlock_add_socket_hooks();
>> landlock_init_id();
>> landlock_initialized = true;
>> pr_info("Up and running.\n");
>> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
>> index 28a80dcad629..d7e6e7b92b7a 100644
>> --- a/security/landlock/socket.c
>> +++ b/security/landlock/socket.c
>> @@ -103,3 +103,81 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>>
>> return err;
>> }
>> +
>> +static int check_socket_access(const struct landlock_ruleset *dom,
>> + uintptr_t key,
>> + layer_mask_t (*const layer_masks)[],
>> + access_mask_t handled_access)
>> +{
>> + const struct landlock_rule *rule;
>> + struct landlock_id id = {
>> + .type = LANDLOCK_KEY_SOCKET,
>> + };
>> +
>> + id.key.data = key;
>
> This line can be made part of the designated initializer:
>
> struct landlock_id id = {
> .type = ...,
> .key.data = ...,
> };
>
Indeed, thats would be better.
>
>> + rule = landlock_find_rule(dom, id);
>> + if (landlock_unmask_layers(rule, handled_access, layer_masks,
>> + LANDLOCK_NUM_ACCESS_SOCKET))
>> + return 0;
>> + return -EACCES;
>> +}
>> +
>> +static int hook_socket_create(int family, int type, int protocol, int kern)
>> +{
>> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
>> + access_mask_t handled_access;
>> + const struct access_masks masks = {
>> + .socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + };
>> + const struct landlock_cred_security *const subject =
>> + landlock_get_applicable_subject(current_cred(), masks, NULL);
>> + uintptr_t key;
>> +
>> + if (!subject)
>> + return 0;
>> + /* Checks only user space sockets. */
>> + if (kern)
>> + return 0;
>> +
>> + handled_access = landlock_init_layer_masks(
>> + subject->domain, LANDLOCK_ACCESS_SOCKET_CREATE, &layer_masks,
>> + LANDLOCK_KEY_SOCKET);
>
> Nit: I had to double check to confirm that the same PF_INET/PF_PACKET
> transformation (which net/socket.c refers to as the "uglymoron") has
> already happened on the arguments before hook_socket_create() gets
> called from there. Maybe it's worth a brief mention in a comment
> here.
Ok, thanks!
>
>> + /*
>> + * Error could happen due to parameters are outside of the allowed range,
>
> Grammar nit: drop the "are"
>
> Suggestion: "If this error happens, the parameters are outside of the
> allowed range, so this combination can't have been added to the
> ruleset previously."
Thanks, I'll use it.
>
>> + * so this combination couldn't be added in ruleset previously.
>> + * Therefore, it's not permitted.
>> + */
>> + if (pack_socket_key(family, type, protocol, &key) == -EACCES)
>> + return -EACCES;
>
> BUG: pack_socket_key() does never return -EACCES!
Thanks a lot, will be fixed!
>
> (Consider whether that function should really return an error? Maybe
> a boolean would be better, if you anyway need a different error code
> in both locations where it is called.)
Agreed
>
> Can this code path actually get hit, or do the entry points for
> creating sockets refuse these wrong values at an earlier stage with
> EINVAL already?
There are checks for family and type ranges in __sock_create. Protocol
ranges should be checked in methods specific to protocol family after
LSM hook is triggered. But it would be safer to keep this check in order
to be independent of the specific kernel version.
>
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + /* Ranges were already checked. */
>> + (void)pack_socket_key(family, TYPE_ALL, protocol, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + (void)pack_socket_key(family, type, PROTOCOL_ALL, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + (void)pack_socket_key(family, TYPE_ALL, PROTOCOL_ALL, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + return -EACCES;
>> +}
>
> It initially doesn't look very nice to drop the error from
> pack_socket_key() repeatedly. The call repeats the bounds checks and
> requires more cross-function reasoning to understand.
Agreed
>
> Since 'key' is an uintptr_t anyway, and the wildcards are all ones,
> maybe a simpler way is to define masks for the wildcards?
>
> const uintptr_t any_type_mask = (union key){.data.type = UINT8_MAX}.packed;
> const uintptr_t any_protocol_mask = (union key){.data.protocol = UINT16_MAX}.packed;
>
> and then, after calling pack_socket_key() once with error check, use
> the combinations
>
> * key
> * key | any_type
> * key | any_protocol
> * key | any_type | any_protocol
>
> to construct the wildcard-enabled keys in the four calls to
> check_socket_access()? You could have compile-time assertions or
> tests to check that the masking does the same as packing it from
> scratch when passing -1.
>
> (That being said, I don't feel strongly about it.)
It seems clearer and simpler to me, so I think we should use your
approach. Probably, pack_socket_key() should be changed to pack values
using bit operations instead of socket_key structure:
key = protocol << 16 | type << 8 | family;
>
> Remark on the side: I was briefly confused why we don't need to guard
> on CONFIG_SECURITY_NETWORK, but this is already required by
> CONFIG_LANDLOCK. So that looks good.
>
> –Günther
^ permalink raw reply
* Re: [GIT PULL] selinux/selinux-pr-20251121
From: pr-tracker-bot @ 2025-11-22 18:29 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <e6a2c26951147ce12c57ff1fd53651c0@paul-moore.com>
The pull request you sent on Fri, 21 Nov 2025 15:57:27 -0500:
> https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20251121
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5703357ede59ce8b0af11e02c374a3db73c55ee8
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* Re: [PATCH 5/6] landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is disabled.
From: Tingmao Wang @ 2025-11-22 23:35 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-6-utilityemal77@gmail.com>
On 11/20/25 22:23, Justin Suess wrote:
> This was necessary when fixing the no inherit patch in the implementation of kunit tests.
>
> When compiled without the audit flag enabled, kunit tests would fail to compile because of
> the missing quiet flag field. This fixes this issue.
>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
Thanks for spotting these issues! I will include your fix in the next
version of my patch (it's not merged yet, so mistakes can be fixed "at
source"). Feel free to reply to my patch directly if you spot more
problems :)
(For the landlock_log_denial one, we might not end up changing the
function's parameter after all - Mickaël has suggested the rule_flags be
embedded inside the landlock_request)
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-23 2:08 UTC (permalink / raw)
To: Helge Deller, John Paul Adrian Glaubitz, Helge Deller
Cc: linux-kernel, apparmor, linux-security-module
In-Reply-To: <be9c143d-1d5e-4c5b-9078-4a7804489258@gmx.de>
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.
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.
^ permalink raw reply
* Re: [PATCH 5/6] landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is disabled.
From: Justin Suess @ 2025-11-23 16:43 UTC (permalink / raw)
To: m; +Cc: gnoack, jack, linux-security-module, mic, utilityemal77, xandfury
In-Reply-To: <baa3449e-e238-4caa-a16e-c252016bf480@maowtm.org>
No problem it was an easy fix. I'll reply directly to your patch in the
future, I'm still figuring out mailing list development and kernel
development workflows.
I do realize now I have some behaviors in this series that impact the
quiet flag that I want your input on. I'll add those as replies to
your patch series as well.
I plan to keep rebasing this patch off the quiet flag series as you release
new versions. I also plan to introduce some selftests combining the two flags
and ensuring they interact with eachother as expected.
The next version of this patch is going to include some big refactorings
(most likely removing the xarrays) and fix some edge cases I discovered.
King Regards,
Justin Suess
^ permalink raw reply
* Re: [PATCH v4 00/10] Implement LANDLOCK_ADD_RULE_QUIET
From: Justin Suess @ 2025-11-23 17:01 UTC (permalink / raw)
To: m; +Cc: gnoack, jack, linux-security-module, mic, utilityemal77, xandfury
In-Reply-To: <5c0de8ee7e00aff1aceb3a80f5af162eeaaa06db.1763330228.git.m@maowtm.org>
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.
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.
Alternatives could be a new flag to control whether NO_INHERIT also
suppresses flag inheritence.
Or simply having LANDLOCK_ADD_RULE_NO_INHERIT continue to only apply to
access masks.
The latest version of LANDLOCK_ADD_RULE_NO_INHERIT is below for
convienence.
v3:
https://lore.kernel.org/linux-security-module/20251120222346.1157004-1-utilityemal77@gmail.com/T/#t
Kind Regards,
Justin Suess
^ permalink raw reply
* Re: [PATCH v3 0/9] module: Introduce hash-based integrity checking
From: Sebastian Andrzej Siewior @ 2025-11-23 17:05 UTC (permalink / raw)
To: Thomas Weißschuh
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: <20251119154834.A-tQsLzh@linutronix.de>
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.
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.
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.
diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000000000..e4a35c15f0a94
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,7 @@
+CC := gcc
+CFLAGS := -O2 -g -Wall
+LDLIBS := -lcrypto
+
+all: compute_hashes mk-files verify_hash
+test: compute_hashes mk-files verify_hash
+ ./verify_test.sh
diff --git a/compute_hashes.c b/compute_hashes.c
new file mode 100644
index 0000000000000..da61b214137b8
--- /dev/null
+++ b/compute_hashes.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Compute hashes for individual files and build a merkle tree.
+ *
+ * Author: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
+ *
+ */
+#define _GNU_SOURCE 1
+#include <ftw.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include <sys/stat.h>
+#include <sys/mman.h>
+
+#include <openssl/evp.h>
+
+#include "helpers.h"
+
+struct file_entry {
+ char *name;
+ size_t fsize;
+ unsigned int pos;
+ unsigned char hash[EVP_MAX_MD_SIZE];
+};
+
+static struct file_entry *fh_list;
+static size_t num_files;
+
+struct leaf_hash {
+ unsigned char hash[EVP_MAX_MD_SIZE];
+};
+
+struct mtree {
+ struct leaf_hash **l;
+ unsigned int *entries;
+ unsigned int levels;
+};
+
+static unsigned int get_pow2(unsigned int val)
+{
+ return 31 - __builtin_clz(val);
+}
+
+static unsigned int roundup_pow2(unsigned int val)
+{
+ return 1 << (get_pow2(val - 1) + 1);
+}
+
+static unsigned int log2_roundup(unsigned int val)
+{
+ return get_pow2(roundup_pow2(val));
+}
+
+static int str_endswith(const char *s, const char *suffix)
+{
+ size_t ls, lf;
+
+ ls = strlen(s);
+ lf = strlen(suffix);
+
+ if (ls <= lf)
+ return -1;
+ return strcmp(s + ls - lf, suffix);
+}
+
+static void __print_hash(unsigned char *h)
+{
+ int i;
+
+ for (i = 0; i < hash_size; i++)
+ printf("%02x", h[i]);
+}
+
+static void print_hash(unsigned char *h)
+{
+ __print_hash(h);
+ printf("\n");
+}
+
+static int hash_file(struct file_entry *fe)
+{
+ void *mem;
+ int fd;
+
+ fd = open(fe->name, O_RDONLY);
+ if (fd < 0) {
+ printf("Failed to open %s: %m\n", fe->name);
+ exit(1);
+ }
+
+ mem = mmap(NULL, fe->fsize, PROT_READ, MAP_SHARED, fd, 0);
+ close(fd);
+
+ if (mem == MAP_FAILED) {
+ printf("Failed to mmap %s: %m\n", fe->name);
+ exit(1);
+ }
+
+ hash_data(mem, fe->pos, fe->fsize, fe->hash);
+
+ munmap(mem, fe->fsize);
+ return 0;
+}
+
+static int add_files_cb(const char *fpath, const struct stat *sb, int tflag,
+ struct FTW *ftwbuf)
+{
+ if (tflag != FTW_F)
+ return 0;
+
+ if (str_endswith(fpath, ".ko"))
+ return 0;
+
+ fh_list = xrealloc(fh_list, (num_files + 1) * sizeof (struct file_entry));
+
+ fh_list[num_files].name = strdup(fpath);
+ if (!fh_list[num_files].name) {
+ printf("Failed to allocate memory\n");
+ exit(1);
+ }
+
+ fh_list[num_files].fsize = sb->st_size;
+
+ num_files++;
+ return 0;
+}
+
+static int cmp_file_entry(const void *p1, const void *p2)
+{
+ const struct file_entry *f1, *f2;
+
+ f1 = p1;
+ f2 = p2;
+
+ return strcmp(f1->name, f2->name);
+}
+
+static struct mtree *build_merkle(struct file_entry *fh, size_t num)
+{
+ unsigned int i, le;
+ struct mtree *mt;
+
+ mt = xmalloc(sizeof(struct mtree));
+ mt->levels = log2_roundup(num);
+ mt->l = xcalloc(sizeof(struct leaf_hash *), mt->levels);
+
+ mt->entries = xcalloc(sizeof(unsigned int), mt->levels);
+ le = num / 2;
+ if (num & 1)
+ le++;
+ mt->entries[0] = le;
+ mt->l[0] = xcalloc(sizeof(struct leaf_hash), le);
+
+ /* First level of pairs */
+ for (i = 0; i < num; i+= 2) {
+ if (i == num - 1) {
+ /* Odd number of files, no pair. Hash with itself */
+ hash_entry(fh[i].hash, fh[i].hash, mt->l[0][i/2].hash);
+ } else {
+ hash_entry(fh[i].hash, fh[i + 1].hash, mt->l[0][i/2].hash);
+ }
+ }
+ for (i = 1; i < mt->levels; i++) {
+ int n;
+ int odd = 0;
+
+ if (le & 1) {
+ le++;
+ odd++;
+ }
+
+ mt->entries[i] = le / 2;
+ mt->l[i] = xcalloc(sizeof(struct leaf_hash), le);
+
+ for (n = 0; n < le; n += 2) {
+ if (n == le - 2 && odd) {
+ /* Odd number of pairs, no pair. Hash with itself */
+ hash_entry(mt->l[i - 1][n].hash, mt->l[i - 1][n].hash,
+ mt->l[i][n/2].hash);
+ } else {
+ hash_entry(mt->l[i - 1][n].hash, mt->l[i - 1][n +1].hash,
+ mt->l[i][n/2].hash);
+ }
+ }
+ le = mt->entries[i];
+ }
+ return mt;
+}
+
+static void free_mtree(struct mtree *mt)
+{
+ int i;
+
+ for (i = 0; i < mt->levels; i++)
+ free(mt->l[i]);
+
+ free(mt->l);
+ free(mt->entries);
+ free(mt);
+}
+
+static void write_be_int(int fd, unsigned int v)
+{
+ unsigned int be_val = host_to_be32(v);
+
+ if (write(fd, &be_val, sizeof(be_val)) != sizeof(be_val)) {
+ printf("Failed writting to file: %m\n");
+ exit(1);
+ }
+}
+
+static void write_hash(int fd, const void *h)
+{
+ ssize_t wr;
+
+ wr = write(fd, h, hash_size);
+ if (wr != hash_size) {
+ printf("Failed writting to file: %m\n");
+ exit(1);
+ }
+}
+
+static void build_proof(struct mtree *mt, unsigned int n, int fd)
+{
+ unsigned char cur[EVP_MAX_MD_SIZE];
+ unsigned char tmp[EVP_MAX_MD_SIZE];
+ struct file_entry *fe, *fe_sib;
+ unsigned int i;
+
+ fe = &fh_list[n];
+
+ if ((n & 1) == 0) {
+ /* No pair, hash with itself */
+ if (n + 1 == num_files)
+ fe_sib = fe;
+ else
+ fe_sib = &fh_list[n + 1];
+ } else {
+ fe_sib = &fh_list[n - 1];
+ }
+ /* First comes the node position into the file */
+ write_be_int(fd, n);
+
+ if ((n & 1) == 0)
+ hash_entry(fe->hash, fe_sib->hash, cur);
+ else
+ hash_entry(fe_sib->hash, fe->hash, cur);
+
+ /* Next is the sibling hash, followed by hashes in the tree */
+ write_hash(fd, fe_sib->hash);
+
+ for (i = 0; i < mt->levels - 1; i++) {
+ n >>= 1;
+ if ((n & 1) == 0) {
+ void *h;
+
+ /* No pair, hash with itself */
+ if (n + 1 == mt->entries[i])
+ h = cur;
+ else
+ h = mt->l[i][n + 1].hash;
+
+ hash_entry(cur, h, tmp);
+ write_hash(fd, h);
+ } else {
+ hash_entry(mt->l[i][n - 1].hash, cur, tmp);
+ write_hash(fd, mt->l[i][n - 1].hash);
+ }
+ memcpy(cur, tmp, hash_size);
+ }
+
+ /* After all that, the end hash should match the root hash */
+ if (memcmp(cur, mt->l[mt->levels - 1][0].hash, hash_size))
+ printf("MISS-MATCH\n");
+}
+
+static void write_merkle_root(struct mtree *mt, const char *fp)
+{
+ char buf[1024];
+ int fd;
+
+ if (snprintf(buf, sizeof(buf), "%s/hash_root", fp) >= sizeof(buf)) {
+ printf("Root dir too long\n");
+ exit(1);
+ }
+ fd = open(buf, O_WRONLY | O_CREAT | O_TRUNC, DEF_F_PERM);
+ if (fd < 0) {
+ printf("Failed to create %s: %m\n", buf);
+ exit(1);
+ }
+
+ write_be_int(fd, mt->levels);
+ write_hash(fd, mt->l[mt->levels - 1][0].hash);
+ close(fd);
+ printf("Files %ld levels: %d root hash: ", num_files, mt->levels);
+ print_hash(mt->l[mt->levels - 1][0].hash);
+}
+
+static void write_merkle_root_h(struct mtree *mt, const char *fp)
+{
+ char buf[1024];
+ unsigned int i;
+ unsigned char *h;
+ FILE *f;
+
+ if (snprintf(buf, sizeof(buf), "%s/hash_root.h", fp) >= sizeof(buf)) {
+ printf("Root dir too long\n");
+ exit(1);
+ }
+ f = fopen(buf, "w");
+ if (!f) {
+ printf("Failed to create %s: %m\n", buf);
+ exit(1);
+ }
+ h = mt->l[mt->levels - 1][0].hash;
+
+ fprintf(f, "#ifndef __HASH_ROOT_TREE_H__\n");
+ fprintf(f, "#define __HASH_ROOT_TREE_H__\n\n");
+ fprintf(f, "unsigned int hashed_mods_levels = %u;\n", mt->levels);
+ fprintf(f, "unsigned char hashed_mods_root[%d] = {", hash_size);
+ for (i = 0; i < hash_size; i++) {
+ char *space = "";
+
+ if (!(i % 8))
+ fprintf(f, "\n\t");
+
+ if ((i + 1) % 8)
+ space = " ";
+
+ fprintf(f, "0x%02x,%s", h[i], space);
+ }
+ fprintf(f, "\n};\n#endif\n");
+ fclose(f);
+}
+
+int main(int argc, char *argv[])
+{
+ const EVP_MD *hash_evp;
+ char *fp;
+ struct mtree *mt;
+ int i;
+
+ ctx = EVP_MD_CTX_new();
+ if (!ctx)
+ goto err_ossl;
+
+ if (argc != 2) {
+ printf("%s: folder\n", argv[0]);
+ return 1;
+ }
+ fp = argv[1];
+
+ hash_evp = EVP_sha256();
+ hash_size = EVP_MD_get_size(hash_evp);
+ if (hash_size <= 0)
+ goto err_ossl;
+
+ if (EVP_DigestInit_ex(ctx, hash_evp, NULL) != 1)
+ goto err_ossl;
+
+ nftw(fp, add_files_cb, 64, 0);
+
+ qsort(fh_list, num_files, sizeof(struct file_entry), cmp_file_entry);
+
+ for (i = 0; i < num_files; i++) {
+ fh_list[i].pos = i;
+ hash_file(&fh_list[i]);
+ }
+
+ mt = build_merkle(fh_list, num_files);
+ write_merkle_root(mt, fp);
+ write_merkle_root_h(mt, fp);
+ for (i = 0; i < num_files; i++) {
+ char signame[1024];
+ int fd;
+ int ret;
+
+ ret = snprintf(signame, sizeof(signame), "%s.hash", fh_list[i].name);
+ if (ret >= sizeof(signame)) {
+ printf("path + name too long\n");
+ return 1;
+ }
+ fd = open(signame, O_WRONLY | O_CREAT | O_TRUNC, DEF_F_PERM);
+ if (fd < 0) {
+ printf("Can't create %s: %m\n", signame);
+ return 1;
+ }
+ build_proof(mt, i, fd);
+ close(fd);
+ }
+
+ free_mtree(mt);
+ for (i = 0; i < num_files; i++)
+ free(fh_list[i].name);
+ free(fh_list);
+
+ EVP_MD_CTX_free(ctx);
+ return 0;
+
+err_ossl:
+ printf("libssl operation failed\n");
+ return 1;
+}
diff --git a/helpers.h b/helpers.h
new file mode 100644
index 0000000000000..f52ad3543f890
--- /dev/null
+++ b/helpers.h
@@ -0,0 +1,109 @@
+#ifndef __HELPERS_H__
+#define __HELPERS_H__
+
+static EVP_MD_CTX *ctx;
+static int hash_size;
+
+#define DEF_F_PERM (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) /* 0644*/
+#define DEF_D_PERM (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH) /* 0755*/
+
+static unsigned int host_to_be32(unsigned int v)
+{
+#if __BYTE_ORDER__ == __LITTLE_ENDIAN
+ return __builtin_bswap32(v);
+#elif __BYTE_ORDER__ == __BIG_ENDIAN
+ return v;
+#else
+#error Missing endian define
+#endif
+}
+
+static inline void *xcalloc(size_t n, size_t size)
+{
+ void *p;
+
+ p = calloc(n, size);
+ if (p)
+ return p;
+ printf("Memory allocation failed\n");
+ exit(1);
+}
+
+static void *xmalloc(size_t size)
+{
+ void *p;
+
+ p = malloc(size);
+ if (p)
+ return p;
+ printf("Memory allocation failed\n");
+ exit(1);
+}
+
+static inline void *xrealloc(void *oldp, size_t size)
+{
+ void *p;
+
+ p = realloc(oldp, size);
+ if (p)
+ return p;
+ printf("Memory allocation failed\n");
+ exit(1);
+}
+
+static void hash_data(void *p, unsigned int pos, size_t size, void *ret_hash)
+{
+ unsigned char magic = 0x01;
+ unsigned int pos_be;
+
+ pos_be = host_to_be32(pos);
+ if (EVP_DigestInit_ex(ctx, NULL, NULL) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, &magic, sizeof(magic)) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, &pos_be, sizeof(pos_be)) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, p, size) != 1)
+ goto err;
+
+ if (EVP_DigestFinal_ex(ctx, ret_hash, NULL) != 1)
+ goto err;
+
+ return;
+
+err:
+ printf("libssl operation failed\n");
+ exit(1);
+}
+static void hash_entry(void *left, void *right, void *ret_hash)
+{
+ unsigned char magic = 0x02;
+
+ if (EVP_DigestInit_ex(ctx, NULL, NULL) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, &magic, sizeof(magic)) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, left, hash_size) != 1)
+ goto err;
+
+ if (EVP_DigestUpdate(ctx, right, hash_size) != 1)
+ goto err;
+
+ if (EVP_DigestFinal_ex(ctx, ret_hash, NULL) != 1)
+ goto err;
+
+ return;
+
+err:
+ printf("libssl operation failed\n");
+ exit(1);
+}
+
+
+
+#endif
diff --git a/verify_hash.c b/verify_hash.c
new file mode 100644
index 0000000000000..0a842f27f1ebc
--- /dev/null
+++ b/verify_hash.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Verify a file against and its hash against a merkle tree hash.
+ *
+ * Author: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
+ *
+ */
+#define _GNU_SOURCE 1
+#include <unistd.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include <sys/stat.h>
+#include <sys/mman.h>
+
+#include <openssl/evp.h>
+
+#include "helpers.h"
+
+struct hash_root {
+ unsigned int level;
+ unsigned char hash[EVP_MAX_MD_SIZE];
+};
+
+struct verify_sig {
+ unsigned int pos;
+ char hash_sigs[];
+};
+
+static int hash_file(const char *f, unsigned char *hash, unsigned int pos)
+{
+ struct stat sb;
+ int fd, ret;
+ void *mem;
+
+ fd = open(f, O_RDONLY);
+ if (fd < 0) {
+ printf("Failed to open %s: %m\n", f);
+ exit(1);
+ }
+
+ ret = fstat(fd, &sb);
+ if (ret) {
+ printf("stat failed %m\n");
+ exit(1);
+ }
+
+ mem = mmap(NULL, sb.st_size, PROT_READ, MAP_SHARED, fd, 0);
+ close(fd);
+
+ if (mem == MAP_FAILED) {
+ printf("Failed to mmap file: %m\n");
+ exit(1);
+ }
+
+ hash_data(mem, pos, sb.st_size, hash);
+
+ munmap(mem, sb.st_size);
+ return 0;
+}
+
+static void verify_hash(struct hash_root *hr, struct verify_sig *vs, unsigned char *cur,
+ const char *fn)
+{
+ unsigned char tmp[EVP_MAX_MD_SIZE];
+ unsigned sig_ofs = 0;
+ unsigned int i, n;
+
+ n = vs->pos;
+ if ((n & 1) == 0)
+ hash_entry(cur, &vs->hash_sigs[sig_ofs], tmp);
+ else
+ hash_entry(&vs->hash_sigs[sig_ofs], cur, tmp);
+
+ memcpy(cur, tmp, hash_size);
+ sig_ofs += hash_size;
+ for (i = 0; i < hr->level - 1; i++) {
+ n >>= 1;
+ if ((n & 1) == 0) {
+ hash_entry(cur, &vs->hash_sigs[sig_ofs], tmp);
+ } else {
+ hash_entry(&vs->hash_sigs[sig_ofs], cur, tmp);
+ }
+ memcpy(cur, tmp, hash_size);
+ sig_ofs += hash_size;
+ }
+
+ if (!memcmp(cur, hr->hash, hash_size)) {
+ exit(0);
+ } else {
+ printf("MISS-MATCH on %s\n", fn);
+ exit(1);
+ }
+}
+
+static void read_be_int(int fd, unsigned int *val)
+{
+ unsigned int val_be;
+
+ if (read(fd, &val_be, sizeof(val_be)) != sizeof(val_be)) {
+ printf("Can't read from file\n");
+ exit(1);
+ }
+ *val = host_to_be32(val_be);
+}
+
+struct hash_root *read_root_hash(const char *f)
+{
+ int fd;
+ struct hash_root *hr;
+
+ hr = xmalloc(sizeof(*hr));
+ fd = open(f, O_RDONLY);
+ if (fd < 0) {
+ printf("Can't open %s: %m\n", f);
+ exit(1);
+ }
+ read_be_int(fd, &hr->level);
+ if (read(fd, hr->hash, hash_size) != hash_size) {
+ printf("Can't read complete hash (%u): %m\n",
+ hash_size);
+ exit(1);
+ }
+ close(fd);
+ return hr;
+}
+
+static void load_hash_sig(const char *f, struct verify_sig *verify_sig,
+ unsigned int sig_num)
+{
+ ssize_t total_hash_size;
+ struct stat sb;
+ char buf[1024];
+ int fd;
+ int ret;
+
+ total_hash_size = sig_num * hash_size;
+
+ ret = snprintf(buf, sizeof(buf), "%s.hash", f);
+ if (ret >= sizeof(buf)) {
+ printf("Too long\n");
+ exit(1);
+ }
+ fd = open(buf, O_RDONLY);
+ if (fd < 0) {
+ printf("Failed to open %s\n", buf);
+ exit(1);
+ }
+ read_be_int(fd, &verify_sig->pos);
+
+ ret = fstat(fd, &sb);
+ if (ret < 0) {
+ printf("Failed to stat %s: %m\n", f);
+ exit(1);
+ }
+
+ if (sb.st_size != total_hash_size + 4) {
+ printf("Unexpected signature size: Expected %ld vs found %ld\n",
+ total_hash_size + 4, sb.st_size);
+ exit(1);
+ }
+ if (read(fd, verify_sig->hash_sigs, total_hash_size) != total_hash_size) {
+ printf("Failed to read the signature: %m\n");
+ exit(1);
+ }
+ close(fd);
+}
+
+int main(int argc, char *argv[])
+{
+ struct hash_root *hash_root;
+ struct verify_sig *vsig;
+ unsigned char fhash[EVP_MAX_MD_SIZE];
+ const EVP_MD *hash_evp;
+
+ ctx = EVP_MD_CTX_new();
+ if (!ctx)
+ goto err;
+
+ if (argc != 3) {
+ printf("%s: hash_root module\n", argv[0]);
+ return 1;
+ }
+
+ hash_evp = EVP_sha256();
+ hash_size = EVP_MD_get_size(hash_evp);
+ if (hash_size <= 0)
+ goto err;
+
+ if (EVP_DigestInit_ex(ctx, hash_evp, NULL) != 1)
+ goto err;
+
+ hash_root = read_root_hash(argv[1]);
+ vsig = xmalloc(sizeof(struct verify_sig) + hash_root->level * hash_size);
+
+ load_hash_sig(argv[2], vsig, hash_root->level);
+ hash_file(argv[2], fhash, vsig->pos);
+ verify_hash(hash_root, vsig, fhash, argv[2]);
+
+ EVP_MD_CTX_free(ctx);
+ return 0;
+err:
+ printf("libssl operation failed\n");
+ return 1;
+}
--
2.51.0
Sebastian
^ permalink raw reply related
* Re: [PATCH v3 7/9] module: Move lockdown check into generic module loader
From: Sebastian Andrzej Siewior @ 2025-11-23 17:10 UTC (permalink / raw)
To: Paul Moore
Cc: Thomas Weißschuh, Masahiro Yamada, Nathan Chancellor,
Arnd Bergmann, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, 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: <CAHC9VhTuf1u4B3uybZxdojcmz5sFG+_JHUCC=C0N=9gFDmurHg@mail.gmail.com>
On 2025-11-19 14:55:47 [-0500], Paul Moore wrote:
> On Wed, Nov 19, 2025 at 6:20 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > On 2025-04-29 15:04:34 [+0200], Thomas Weißschuh wrote:
> > > The lockdown check buried in module_sig_check() will not compose well
> > > with the introduction of hash-based module validation.
> >
> > An explanation of why would be nice.
>
> /me shrugs
>
> I thought the explanation was sufficient.
Okay. So if it is just me and everyone is well aware then okay.
Sebastian
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Oleg Nesterov @ 2025-11-23 18:32 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Christian Brauner, Alexander Viro, Alexey Dobriyan, Kees Cook,
Andy Lutomirski, Will Drewry, Andrew Morton, Michal Hocko,
Serge Hallyn, James Morris, Randy Dunlap, Suren Baghdasaryan,
Yafang Shao, Helge Deller, Eric W. Biederman, 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: <GV2PPF74270EBEE6F59267B0E9F28F536D0E4C9A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
Hi Bernd,
sorry for delay, I am on PTO, didn't read emails this week...
On 11/17, Bernd Edlinger wrote:
>
> On 11/17/25 16:01, Oleg Nesterov wrote:
> > On 11/17, Bernd Edlinger wrote:
> >>
> >> On 11/11/25 10:21, Christian Brauner wrote:
> >>> On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
> >>
> >>>> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
> >>>> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
> >>>> unconditionally.
> >>>>
> >>
> >> I would not like to drop the mutex when no absolutely necessary for performance reasons.
> >
> > OK, I won't insist... But I don't really understand how this can help to
> > improve the performance. If nothing else, this adds another for_other_threads()
> > loop.
> >
>
> If no dead-lock is possible it is better to complete the de_thread without
> releasing the mutex. For the debugger it is also the better experience,
> no matter when the ptrace_attack happens it will succeed rather quickly either
> before the execve or after the execve.
I still disagree, I still don't understand the "performance reasons", but since I can't
convince you I won't really argue.
> >>>>> + if (unlikely(unsafe_execve_in_progress)) {
> >>>>> + spin_unlock_irq(lock);
> >>>>> + sig->exec_bprm = bprm;
> >>>>> + mutex_unlock(&sig->cred_guard_mutex);
> >>>>> + spin_lock_irq(lock);
> >>>>
> >>>> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
> >>>>
> >>
> >> Since the spin lock was acquired while holding the mutex, both should be
> >> unlocked in reverse sequence and the spin lock re-acquired after releasing
> >> the mutex.
> >
> > Why?
> >
>
> It is generally more safe when each thread acquires its mutexes in order and
> releases them in reverse order.
> Consider this:
> Thread A:
> holds spin_lock_irq(siglock);
> does mutes_unlock(cred_guard_mutex); with irq disabled.
> task switch happens to Thread B which has irq enabled.
> and is waiting for cred_guard_mutex.
> Thrad B:
> does mutex_lock(cred_guard_mutex);
> but is interrupted this point and the interrupt handler I executes
> now iterrupt handler I wants to take siglock and is blocked,
> because the system one single CPU core.
I don't follow. Do you mean PREEMPT_RT ?
If yes. In this case spin_lock_irq() is rt_spin_lock() which doesn't disable irqs,
it does rt_lock_lock() (takes rt_mutex) + migrate_disable().
I do think that spin/mutex/whatever_unlock() is always safe. In any order, and
regardless of RT.
> > And just in case... Lets look at this code
> >
> > + rcu_assign_pointer(task->real_cred, bprm->cred);
> > + task->mm = bprm->mm;
> > + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> > + rcu_assign_pointer(task->real_cred, old_cred);
> > + task->mm = old_mm;
> >
> > again.
> >
> > This is mostly theoretical, but what if begin_new_exec() fails after de_thread()
> > and before exec_mmap() and/or commit_creds(bprm->cred) ? In this case the execing
> > thread will report SIGSEGV to debugger which can (say) read old_mm.
> >
> > No?
> >
>
> Yes, and that is the reason why the debugger has to prove the possession of access rights
> to the process before the execve which are necessary in case exeve fails, yes the debugger
> may inspect the result, and as well the debugger's access rights must be also sufficient
> to ptrace the process after execve succeeds, moreover the debugged process shall stop
> right at the first instruction where the new process starts.
Not sure I understand... OK, I see that you sent V18, and in this version ptrace_attach()
calls __ptrace_may_access() twice, so IIUC ptrace_attach() can only succeed if the debugger
has rights to trace the execing thread both before and after exec...
Oleg.
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Oleg Nesterov @ 2025-11-23 18:52 UTC (permalink / raw)
To: Eric W. Biederman
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: <87h5uoxw06.fsf_-_@email.froward.int.ebiederm.org>
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...
Oleg.
^ permalink raw reply
* Re: [PATCH v4 02/10] landlock: Add API support and docs for the quiet flags
From: Tingmao Wang @ 2025-11-23 21:00 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <20251120.Sae4geish0ei@digikod.net>
On 11/21/25 15:27, Mickaël Salaün wrote:
> On Sun, Nov 16, 2025 at 09:59:32PM +0000, Tingmao Wang wrote:
>> [...]
>> @@ -69,6 +100,39 @@ struct landlock_ruleset_attr {
>> #define LANDLOCK_CREATE_RULESET_ERRATA (1U << 1)
>> /* clang-format on */
>>
>> +/**
>> + * DOC: landlock_add_rule_flags
>> + *
>> + * **Flags**
>> + *
>> + * %LANDLOCK_ADD_RULE_QUIET
>> + * Together with the quiet_* fields in struct landlock_ruleset_attr,
>> + * this flag controls whether Landlock will log audit messages when
>> + * access to the objects covered by this rule is denied by this layer.
>> + *
>> + * If audit logging is enabled, when Landlock denies an access, it will
>> + * suppress the audit log if all of the following are true:
>> + *
>> + * - This layer is the innermost layer that denied the access;
>
> Because these items follows ":" they should not start with a capital
> letter (e.g. "- this layer ...").
>
>> + * - All requested accesses are part of the quiet_* fields in the
>> + * related struct landlock_ruleset_attr;
>
> This should be updated to reflect my comment in the next patch about
> landlock_log_denial().
Not sure I completely understand what needs to be changed - are you
suggesting clarifying that only the accesses denied by this layer (which
naturally has to be handled by this layer) needs to be in quiet_*? So
basically:
* - all accesses denied by this layer are part of the quiet_* fields
* in the related struct landlock_ruleset_attr;
or something else?
Note that quiet_* already has to be a subset of handled_access_*.
>
>> + * - The object (or one of its parents, for filesystem rules) is
>> + * marked as "quiet" via %LANDLOCK_ADD_RULE_QUIET.
>> + *
>> + * Because logging is only suppressed by a layer if the layer denies
>> + * access, a sandboxed program cannot use this flag to "hide" access
>> + * denials, without denying itself the access in the first place.
>> + *
>> + * The effect of this flag does not depend on the value of
>> + * allowed_access in the passed in rule_attr. When this flag is
>> + * present, the caller is also allowed to pass in an empty
>> + * allowed_access.
>> + */
>> +
>> +/* clang-format off */
>> +#define LANDLOCK_ADD_RULE_QUIET (1U << 0)
>> +/* clang-format on */
>> +
>> /**
>> * DOC: landlock_restrict_self_flags
>> *
>> diff --git a/security/landlock/domain.h b/security/landlock/domain.h
>> index 7fb70b25f85a..aadbf53505c0 100644
>> --- a/security/landlock/domain.h
>> +++ b/security/landlock/domain.h
>> @@ -114,6 +114,11 @@ struct landlock_hierarchy {
>> * %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON. Set to false by default.
>> */
>> log_new_exec : 1;
>> + /**
>> + * @quiet_masks: Bitmasks of access that should be quieted (i.e. not
>> + * logged) if the related object is marked as quiet.
>> + */
>> + struct access_masks quiet_masks;
>
> Please update the above @work_free doc.
I assume you meant the one in struct landlock_ruleset, not here. Will update.
>
>> #endif /* CONFIG_AUDIT */
>> };
>>
^ permalink raw reply
* Re: [PATCH v4 03/10] landlock: Suppress logging when quiet flag is present
From: Tingmao Wang @ 2025-11-23 21:01 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <20251120.eoghapeeGh7i@digikod.net>
On 11/21/25 15:27, Mickaël Salaün wrote:
> On Sun, Nov 16, 2025 at 09:59:33PM +0000, Tingmao Wang wrote:
>> [...]
>> for_each_set_bit(access_bit, &access_opt,
>> BITS_PER_TYPE(access_mask_t)) {
>> if (access_req & BIT(access_bit)) {
>> const size_t layer =
>> (deny_masks >> (access_index * 4)) &
>> (LANDLOCK_MAX_NUM_LAYERS - 1);
>> + const bool layer_has_quiet =
>> + !!(quiet_optional_accesses & BIT(access_index));
>>
>> if (layer > youngest_layer) {
>> youngest_layer = layer;
>> + *quiet = layer_has_quiet;
>> missing = BIT(access_bit);
>> } else if (layer == youngest_layer) {
>> missing |= BIT(access_bit);
>> + /*
>> + * Whether the layer has rules with quiet flag covering
>> + * the file accessed does not depend on the access, and so
>> + * if this fails, quiet_optional_accesses is corrupted.
>> + */
>> + WARN_ON_ONCE(*quiet && !layer_has_quiet);
>> + *quiet = layer_has_quiet;
>
> In this case, why update *quiet?
A legitimate case where we end up here is if layer = youngest_layer = 0,
and layer_has_quiet = true, in which case *quiet starts out as false and
we have to set it to true here.
The comment is saying the WARN_ON_ONCE should not fail because the quiet
flag does not depend on access (and hence we should not be trying to set
*quiet from true back to false if the youngest layer hasn't changed), but
*the line after that WARN is still necessary.
I've updated the comment to clarify.
>
>> }
>> }
>> access_index++;
>> @@ -312,42 +323,188 @@ static void test_get_layer_from_deny_masks(struct kunit *const test)
>> {
>> deny_masks_t deny_mask;
>> access_mask_t access;
>> + u8 quiet_optional_accesses;
>> + bool quiet;
>>
>> /* truncate:0 ioctl_dev:2 */
>> deny_mask = 0x20;
>> + quiet_optional_accesses = 0;
>>
>> access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> KUNIT_EXPECT_EQ(test, 0,
>> - get_layer_from_deny_masks(&access,
>> - _LANDLOCK_ACCESS_FS_OPTIONAL,
>> - deny_mask));
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + /* layer denying truncate: quiet, ioctl: not quiet */
>> + quiet_optional_accesses = 0b01;
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> + KUNIT_EXPECT_EQ(test, 0,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>> +
>> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + /* Reverse order - truncate:2 ioctl_dev:0 */
>> + deny_mask = 0x02;
>> + quiet_optional_accesses = 0;
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 0,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + /* layer denying truncate: quiet, ioctl: not quiet */
>> + quiet_optional_accesses = 0b01;
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>> +
>> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 0,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>>
>> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> KUNIT_EXPECT_EQ(test, 2,
>> - get_layer_from_deny_masks(&access,
>> - _LANDLOCK_ACCESS_FS_OPTIONAL,
>> - deny_mask));
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>> +
>> + /* layer denying truncate: not quiet, ioctl: quiet */
>> + quiet_optional_accesses = 0b10;
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 0,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 2,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>>
>> /* truncate:15 ioctl_dev:15 */
>> deny_mask = 0xff;
>> + quiet_optional_accesses = 0;
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> + KUNIT_EXPECT_EQ(test, 15,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> + KUNIT_EXPECT_EQ(test, 15,
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> + KUNIT_EXPECT_EQ(test, access,
>> + LANDLOCK_ACCESS_FS_TRUNCATE |
>> + LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, false);
>> +
>> + /* Both quiet (same layer so quietness must be the same) */
>> + quiet_optional_accesses = 0b11;
>>
>> access = LANDLOCK_ACCESS_FS_TRUNCATE;
>> KUNIT_EXPECT_EQ(test, 15,
>> - get_layer_from_deny_masks(&access,
>> - _LANDLOCK_ACCESS_FS_OPTIONAL,
>> - deny_mask));
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>>
>> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> KUNIT_EXPECT_EQ(test, 15,
>> - get_layer_from_deny_masks(&access,
>> - _LANDLOCK_ACCESS_FS_OPTIONAL,
>> - deny_mask));
>> + get_layer_from_deny_masks(
>> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
>> + deny_mask, quiet_optional_accesses, &quiet));
>> KUNIT_EXPECT_EQ(test, access,
>> LANDLOCK_ACCESS_FS_TRUNCATE |
>> LANDLOCK_ACCESS_FS_IOCTL_DEV);
>> + KUNIT_EXPECT_EQ(test, quiet, true);
>> }
>>
>> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
>> @@ -381,19 +538,39 @@ static bool is_valid_request(const struct landlock_request *const request)
>> return true;
>> }
>>
>> +static access_mask_t
>> +pick_access_mask_for_request_type(const enum landlock_request_type type,
>> + const struct access_masks access_masks)
>> +{
>> + switch (type) {
>> + case LANDLOCK_REQUEST_FS_ACCESS:
>> + return access_masks.fs;
>> + case LANDLOCK_REQUEST_NET_ACCESS:
>> + return access_masks.net;
>> + default:
>> + WARN_ONCE(1, "Invalid request type %d passed to %s", type,
>> + __func__);
>> + return 0;
>> + }
>> +}
>> +
>> /**
>> * landlock_log_denial - Create audit records related to a denial
>> *
>> * @subject: The Landlock subject's credential denying an action.
>> * @request: Detail of the user space request.
>> + * @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if
>> + * this is a scope request (no particular object involved).
>> */
>> void landlock_log_denial(const struct landlock_cred_security *const subject,
>> - const struct landlock_request *const request)
>> + const struct landlock_request *const request,
>> + const struct collected_rule_flags rule_flags)
>
> It would be simpler and limit code change to move rule_flags/quiet_flags
> into struct landlock_request, which means we can also remove
> no_rule_flags.
That's true, I can do that. In fact this way we also don't have to have 2
extra parameters for is_access_to_paths_allowed - it can just operate on
log_request_parent{1,2}->rule_flags. However I do see that
landlock_request is intended to only be used by audit/logging code (and
there is a comment in audit.h about not using it outside CONFIG_AUDIT to
enable it to be optimized away, although testing a fresh build on next it
doesn't look like it is taken out of vmlinux if compiled without
CONFIG_AUDIT). While this is fine for the purpose of this series as the
quiet flag only affects audit logging, I wonder if this might cause a
problem when we want to add more flags that might not be related to audit
(e.g. Justin's LANDLOCK_ADD_RULE_NO_INHERIT).
Alternatively maybe is_access_to_paths_allowed can still take extra
parameters for rule flags, and we can make it so that the new rule_flags
field in landlock_request is only assigned to right before
landlock_log_denial, not from is_access_to_paths_allowed? (I won't do
this for v5 which I will send in a minute)
>
>> {
>> struct audit_buffer *ab;
>> struct landlock_hierarchy *youngest_denied;
>> size_t youngest_layer;
>> - access_mask_t missing;
>> + access_mask_t missing, quiet_mask;
>> + bool object_quiet_flag = false, quiet_applicable_to_access = false;
>>
>> if (WARN_ON_ONCE(!subject || !subject->domain ||
>> !subject->domain->hierarchy || !request))
>> @@ -409,10 +586,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
>> youngest_layer = get_denied_layer(
>> subject->domain, &missing, request->layer_masks,
>> request->layer_masks_size);
>> + object_quiet_flag = !!(rule_flags.quiet_masks & BIT(youngest_layer));
>> } else {
>> youngest_layer = get_layer_from_deny_masks(
>> &missing, request->all_existing_optional_access,
>> - request->deny_masks);
>> + request->deny_masks,
>> + request->quiet_optional_accesses,
>> + &object_quiet_flag);
>> }
>> youngest_denied =
>> get_hierarchy(subject->domain, youngest_layer);
>> @@ -447,6 +627,49 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
>> return;
>> }
>>
>> + /*
>> + * Checks if the object is marked quiet by the layer that denied the
>> + * request. If it's a different layer that marked it as quiet, but
>> + * that layer is not the one that denied the request, we should still
>> + * audit log the denial.
>> + */
>> + if (object_quiet_flag) {
>> + /*
>> + * We now check if the denied requests are all covered by the
>> + * layer's quiet access bits.
>> + */
>> + quiet_mask = pick_access_mask_for_request_type(
>
> This quiet_mask is only used in this branch, so we can declare it here
> and make it const:
>
> const access_mask_t quiet_mask = pick_access_mask_for_request_type(
>
>
>> + request->type, youngest_denied->quiet_masks);
>> + quiet_applicable_to_access = (quiet_mask & missing) == missing;
>
> I think it should be:
>
> quiet_applicable_to_access = (quiet_mask & missing) == (handled_mask & missing);
There is no handled_mask in this context, so I assume you meant
handled_mask of the youngest_layer? But still - not sure I understand why -
missing contains requested access bits that are denied by the youngest
denying layer, and so missing would never be != youngest->handled_mask & missing,
right? Since a layer that doesn't handle an access can't deny it.
>
> We should have a test for this case: an access request (e.g. read-write)
> is denied, half by one layer (e.g. read) and half by another (e.g.
> write). Tests should cover this matrix.
Added as quiet_two_layers_different_handled_{1,2,3}
^ permalink raw reply
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