* Re: [PATCH] xfrm: force flush upon NETDEV_UNREGISTER event
From: Tetsuo Handa @ 2026-01-22 13:07 UTC (permalink / raw)
To: Steffen Klassert, linux-security-module
Cc: Boris Pismenny, David S. Miller, Florian Westphal,
Kristian Evensen, Leon Romanovsky, Leon Romanovsky, Raed Salem,
Raed Salem, Saeed Mahameed, Yossi Kuperman, Network Development,
Aviad Yehezkel, Herbert Xu
In-Reply-To: <aXIKwNJv59KnsnLw@secunet.com>
On 2026/01/22 20:32, Steffen Klassert wrote:
> On Thu, Jan 22, 2026 at 08:28:31PM +0900, Tetsuo Handa wrote:
>> On 2026/01/22 20:15, Steffen Klassert wrote:
>>> Hm, I'd say we should not try to offload to a device that does
>>> not support NETIF_F_HW_ESP.
>>
>> I was about to post the patch below, but you are suggesting that "do not allow calling
>> xfrm_dev_state_add()/xfrm_dev_policy_add() if (dev->features & NETIF_F_HW_ESP) == 0" ?
>
> As said, I think this is the correct way to do it. But let's wait
> on opinions from the hardware people.
OK. I guess something like below.
net/xfrm/xfrm_device.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 52ae0e034d29..19aa61609d24 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -292,6 +292,13 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
dst_release(dst);
}
+ if (!(dev->features & NETIF_F_HW_ESP)) {
+ NL_SET_ERR_MSG(extack, "Device doesn't support offload");
+ xso->dev = NULL;
+ dev_put(dev);
+ return -EINVAL;
+ }
+
if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_state_add) {
xso->dev = NULL;
dev_put(dev);
@@ -367,7 +374,8 @@ int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
if (!dev)
return -EINVAL;
- if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_policy_add) {
+ if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_policy_add ||
+ !(dev->features & NETIF_F_HW_ESP)) {
xdo->dev = NULL;
dev_put(dev);
NL_SET_ERR_MSG(extack, "Policy offload is not supported");
On 2026/01/22 20:15, Steffen Klassert wrote:
>> But I have a question regarding security_xfrm_state_delete()/security_xfrm_policy_delete().
>>
>> xfrm_dev_state_flush_secctx_check() calls security_xfrm_state_delete() which can make
>> xfrm_dev_state_flush() no-op by returning an error value.
>> xfrm_dev_policy_flush_secctx_check() calls security_xfrm_policy_delete() which can make
>> xfrm_dev_policy_flush() no-op by returning an error value.
>>
>> Since xfrm_dev_state_flush()/xfrm_dev_policy_flush() are called by NETDEV_UNREGISTER
>> event (which is a signal for releasing all resources that prevent "struct net_device"
>> references from dropping), making xfrm_dev_state_flush()/xfrm_dev_policy_flush() no-op (by
>> allowing security_xfrm_state_delete()/security_xfrm_policy_delete() to return an error) is
>> a denial-of-service bug.
>
> This means that the calling task doesn't have the permission to delete the
> state, some LSM has a policy the does not grant this permission.
But NETDEV_UNREGISTER event can fire without explicit request from a user.
Roughly speaking, current behavior is that
while (security_xfrm_state_delete() != 0) {
schedule_timeout_uninterruptible(10 * HZ);
pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
dev->name, netdev_refcnt_read(dev));
}
while (security_xfrm_policy_delete() != 0) {
schedule_timeout_uninterruptible(10 * HZ);
pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
dev->name, netdev_refcnt_read(dev));
}
might be executed upon e.g. termination of a userspace process.
>
>>
>> Therefore, I wonder what are security_xfrm_state_delete() and security_xfrm_policy_delete()
>> for. Can I kill xfrm_dev_state_flush_secctx_check() and xfrm_dev_policy_flush_secctx_check() ?
>
> This might violate a LSM policy then.
But LSM policy that results in system hung upon automatic cleanup logic is so stupid.
I want to kill xfrm_dev_state_flush_secctx_check() and xfrm_dev_policy_flush_secctx_check()
in order to eliminate possibility of system hung.
^ permalink raw reply related
* Re: [PATCH] xfrm: force flush upon NETDEV_UNREGISTER event
From: Steffen Klassert @ 2026-01-22 11:32 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Boris Pismenny, David S. Miller, Florian Westphal,
Kristian Evensen, Leon Romanovsky, Leon Romanovsky, Raed Salem,
Raed Salem, Saeed Mahameed, Yossi Kuperman, Network Development,
linux-security-module, Aviad Yehezkel
In-Reply-To: <447378de-3cc9-44f5-872e-a1fc477f591e@I-love.SAKURA.ne.jp>
On Thu, Jan 22, 2026 at 08:28:31PM +0900, Tetsuo Handa wrote:
> On 2026/01/22 20:15, Steffen Klassert wrote:
> > Hm, I'd say we should not try to offload to a device that does
> > not support NETIF_F_HW_ESP.
>
> I was about to post the patch below, but you are suggesting that "do not allow calling
> xfrm_dev_state_add()/xfrm_dev_policy_add() if (dev->features & NETIF_F_HW_ESP) == 0" ?
As said, I think this is the correct way to do it. But let's wait
on opinions from the hardware people.
^ permalink raw reply
* Re: [PATCH] xfrm: force flush upon NETDEV_UNREGISTER event
From: Tetsuo Handa @ 2026-01-22 11:28 UTC (permalink / raw)
To: Steffen Klassert
Cc: Boris Pismenny, David S. Miller, Florian Westphal,
Kristian Evensen, Leon Romanovsky, Leon Romanovsky, Raed Salem,
Raed Salem, Saeed Mahameed, Yossi Kuperman, Network Development,
linux-security-module, Aviad Yehezkel
In-Reply-To: <aXIGxmCB2QU86-iA@secunet.com>
On 2026/01/22 20:15, Steffen Klassert wrote:
> Hm, I'd say we should not try to offload to a device that does
> not support NETIF_F_HW_ESP.
I was about to post the patch below, but you are suggesting that "do not allow calling
xfrm_dev_state_add()/xfrm_dev_policy_add() if (dev->features & NETIF_F_HW_ESP) == 0" ?
[PATCH] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events
syzbot is reporting that "struct xfrm_state" refcount is leaking.
unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
ref_tracker: netdev@ffff888052f24618 has 1/1 users at
__netdev_tracker_alloc include/linux/netdevice.h:4400 [inline]
netdev_tracker_alloc include/linux/netdevice.h:4412 [inline]
xfrm_dev_state_add+0x3a5/0x1080 net/xfrm/xfrm_device.c:316
xfrm_state_construct net/xfrm/xfrm_user.c:986 [inline]
xfrm_add_sa+0x34ff/0x5fa0 net/xfrm/xfrm_user.c:1022
xfrm_user_rcv_msg+0x58e/0xc00 net/xfrm/xfrm_user.c:3507
netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2550
xfrm_netlink_rcv+0x71/0x90 net/xfrm/xfrm_user.c:3529
netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
netlink_unicast+0x5aa/0x870 net/netlink/af_netlink.c:1344
netlink_sendmsg+0x8c8/0xdd0 net/netlink/af_netlink.c:1894
sock_sendmsg_nosec net/socket.c:727 [inline]
__sock_sendmsg net/socket.c:742 [inline]
____sys_sendmsg+0xa5d/0xc30 net/socket.c:2592
___sys_sendmsg+0x134/0x1d0 net/socket.c:2646
__sys_sendmsg+0x16d/0x220 net/socket.c:2678
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xcd/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Since xfrm_dev_state_add() takes a reference to "struct net_device",
the corresponding NETDEV_UNREGISTER handler must release that reference.
Commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
introduced xfrm_dev_state_add() which grabs a reference to
"struct net_device". That commit called xfrm_dev_state_add() from
xfrm_state_construct() and introduced the NETDEV_UNREGISTER case to
xfrm_dev_event(). But that commit implemented xfrm_dev_unregister() as
a no-op, and implemented xfrm_dev_down() to call xfrm_dev_state_flush()
only if (dev->features & NETIF_F_HW_ESP) != 0. Maybe that commit expected
that NETDEV_DOWN event is fired before NETDEV_UNREGISTER event fires, and
also assumed that xfrm_dev_state_add() is called only if
(dev->features & NETIF_F_HW_ESP) != 0.
Commit ec30d78c14a8 ("xfrm: add xdst pcpu cache") added
xfrm_policy_cache_flush() call to xfrm_dev_unregister(), but
commit e4db5b61c572 ("xfrm: policy: remove pcpu policy cache") removed
xfrm_policy_cache_flush() call from xfrm_dev_unregister() and also
removed the NETDEV_UNREGISTER case from xfrm_dev_event() because
xfrm_dev_unregister() again became no-op.
Commit 03891f820c21 ("xfrm: handle NETDEV_UNREGISTER for xfrm device")
re-introduced the NETDEV_UNREGISTER case to xfrm_dev_event(), but that
commit for unknown reason chose to share xfrm_dev_down() between
the NETDEV_DOWN case and the NETDEV_UNREGISTER case. But since syzbot is
demonstrating that it is possible to call xfrm_dev_state_add() even if
(dev->features & NETIF_F_HW_ESP) == 0, we need to make sure that
netdev_put() from xfrm_dev_state_free() from xfrm_dev_state_flush() is
called upon NETDEV_UNREGISTER event.
Assuming that it is correct behavior to call netdev_put() upon NETDEV_DOWN
event even if (dev->features & NETIF_F_HW_ESP) == 0, this patch updates
xfrm_dev_down() rather than re-introducing xfrm_dev_unregister().
Reported-by: syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Since no reproducer is available for this problem, I can't ask syzbot to
test this change. But I confirmed using linux-next tree that calling
xfrm_dev_state_flush() upon NETDEV_UNREGISTER event even if
(dev->features & NETIF_F_HW_ESP) == 0 solved this problem.
net/xfrm/xfrm_device.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 52ae0e034d29..26e62b6a9db5 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -536,10 +536,8 @@ static int xfrm_api_check(struct net_device *dev)
static int xfrm_dev_down(struct net_device *dev)
{
- if (dev->features & NETIF_F_HW_ESP) {
- xfrm_dev_state_flush(dev_net(dev), dev, true);
- xfrm_dev_policy_flush(dev_net(dev), dev, true);
- }
+ xfrm_dev_state_flush(dev_net(dev), dev, true);
+ xfrm_dev_policy_flush(dev_net(dev), dev, true);
return NOTIFY_DONE;
}
--
2.47.3
^ permalink raw reply related
* Re: [PATCH] cipso: harden use of skb_cow() in cipso_v4_skbuff_setattr()
From: patchwork-bot+netdevbpf @ 2026-01-22 11:20 UTC (permalink / raw)
To: Will Rosenberg
Cc: paul, davem, dsahern, edumazet, kuba, pabeni, horms, netdev,
linux-security-module, linux-kernel
In-Reply-To: <20260120155738.982771-1-whrosenb@asu.edu>
Hello:
This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Tue, 20 Jan 2026 08:57:38 -0700 you wrote:
> If skb_cow() is passed a headroom <= -NET_SKB_PAD, it will trigger a
> BUG. As a result, use cases should avoid calling with a headroom that
> is negative to prevent triggering this issue.
>
> This is the same code pattern fixed in Commit 58fc7342b529 ("ipv6:
> BUG() in pskb_expand_head() as part of calipso_skbuff_setattr()").
>
> [...]
Here is the summary with links:
- cipso: harden use of skb_cow() in cipso_v4_skbuff_setattr()
https://git.kernel.org/netdev/net-next/c/40cb4cb77ea2
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH] xfrm: force flush upon NETDEV_UNREGISTER event
From: Steffen Klassert @ 2026-01-22 11:15 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Aviad Yehezkel, Aviv Heller, Boris Pismenny, David S. Miller,
Florian Westphal, Guy Shapiro, Ilan Tayari, Kristian Evensen,
Leon Romanovsky, Leon Romanovsky, Raed Salem, Raed Salem,
Saeed Mahameed, Yossi Kuperman, Network Development,
linux-security-module
In-Reply-To: <287edf7f-85fb-46c3-9c70-c8ec7014a0db@I-love.SAKURA.ne.jp>
On Thu, Jan 22, 2026 at 05:24:22PM +0900, Tetsuo Handa wrote:
> A debug patch in linux-next-20260121
> ( https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit?id=fc0f090e41e652d158f946c616cdd82baed3c8f4 )
> has demonstrated that calling xfrm_dev_state_flush()/xfrm_dev_policy_flush()
> when (dev->features & NETIF_F_HW_ESP) == 0 helps releasing "struct net_device" refcount.
>
> unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
> ref_tracker: netdev@ffff88805d3c0628 has 1/1 users at
> xfrm_dev_state_add+0x6f4/0xc40 net/xfrm/xfrm_device.c:316
> xfrm_state_construct net/xfrm/xfrm_user.c:986 [inline]
> xfrm_add_sa+0x34ca/0x4230 net/xfrm/xfrm_user.c:1022
> xfrm_user_rcv_msg+0x746/0xb20 net/xfrm/xfrm_user.c:3507
> netlink_rcv_skb+0x232/0x4b0 net/netlink/af_netlink.c:2550
> xfrm_netlink_rcv+0x79/0x90 net/xfrm/xfrm_user.c:3529
> netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
> netlink_unicast+0x80f/0x9b0 net/netlink/af_netlink.c:1344
> netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1894
> sock_sendmsg_nosec+0x18f/0x1d0 net/socket.c:737
> __sock_sendmsg net/socket.c:752 [inline]
> ____sys_sendmsg+0x589/0x8c0 net/socket.c:2610
> ___sys_sendmsg+0x2a5/0x360 net/socket.c:2664
> __sys_sendmsg net/socket.c:2696 [inline]
> __do_sys_sendmsg net/socket.c:2701 [inline]
> __se_sys_sendmsg net/socket.c:2699 [inline]
> __x64_sys_sendmsg+0x1bd/0x2a0 net/socket.c:2699
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0xe2/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> infiniband: balance for netdevsim0@ib_gid_table_entry is 0
> ***** Releasing 1 refcount on 0000000000000000
> ***** Refcount for netdevsim0 changed from 2 to 1
>
> The bond_master_netdev_event(NETDEV_UNREGISTER) case is already calling
> xfrm_dev_state_flush() without checking (dev->features & NETIF_F_HW_ESP) != 0.
> Therefore, I assume that (dev->features & NETIF_F_HW_ESP) != 0 check in
> xfrm_dev_down() is wrong, and I would like to propose
>
> static int xfrm_dev_down(struct net_device *dev)
> {
> - if (dev->features & NETIF_F_HW_ESP) {
> - xfrm_dev_state_flush(dev_net(dev), dev, true);
> - xfrm_dev_policy_flush(dev_net(dev), dev, true);
> - }
> + xfrm_dev_state_flush(dev_net(dev), dev, true);
> + xfrm_dev_policy_flush(dev_net(dev), dev, true);
>
> return NOTIFY_DONE;
> }
Hm, I'd say we should not try to offload to a device that does
not support NETIF_F_HW_ESP.
>
> change as a fix for "unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2"
> problem.
>
>
>
> But I have a question regarding security_xfrm_state_delete()/security_xfrm_policy_delete().
>
> xfrm_dev_state_flush_secctx_check() calls security_xfrm_state_delete() which can make
> xfrm_dev_state_flush() no-op by returning an error value.
> xfrm_dev_policy_flush_secctx_check() calls security_xfrm_policy_delete() which can make
> xfrm_dev_policy_flush() no-op by returning an error value.
>
> Since xfrm_dev_state_flush()/xfrm_dev_policy_flush() are called by NETDEV_UNREGISTER
> event (which is a signal for releasing all resources that prevent "struct net_device"
> references from dropping), making xfrm_dev_state_flush()/xfrm_dev_policy_flush() no-op (by
> allowing security_xfrm_state_delete()/security_xfrm_policy_delete() to return an error) is
> a denial-of-service bug.
This means that the calling task doesn't have the permission to delete the
state, some LSM has a policy the does not grant this permission.
>
> Therefore, I wonder what are security_xfrm_state_delete() and security_xfrm_policy_delete()
> for. Can I kill xfrm_dev_state_flush_secctx_check() and xfrm_dev_policy_flush_secctx_check() ?
This might violate a LSM policy then.
^ permalink raw reply
* Re: [PATCH v2] ima: Fallback to ctime check for FS without kstat.change_cookie
From: Roberto Sassu @ 2026-01-22 9:52 UTC (permalink / raw)
To: Frederick Lawler, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Paul Moore, James Morris, Serge E. Hallyn,
Darrick J. Wong, Christian Brauner, Josef Bacik, Jeff Layton
Cc: linux-kernel, linux-integrity, linux-security-module, kernel-team
In-Reply-To: <20260120-xfs-ima-fixup-v2-1-f332ead8b043@cloudflare.com>
On Tue, 2026-01-20 at 14:20 -0600, Frederick Lawler wrote:
> Commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps")
> introduced a means to track change detection for an inode
> via ctime updates, opposed to setting kstat.change_cookie to
> an i_version when calling into xfs_vn_getattr().
>
> This introduced a regression for IMA such that an action
> performed on a LOWER inode on a stacked file systems always
> requires a re-evaluation if the LOWER file system does not
> leverage kstat.change_cookie to track inode i_version or lacks
> i_version support all together.
>
> In the case of stacking XFS on XFS, an action on either the LOWER or UPPER
> will require re-evaluation. Stacking TPMFS on XFS for instance, once the
> inode is UPPER is mutated, IMA resumes normal behavior because TMPFS
> leverages generic_fillattr() to update the change cookie.
>
> This is because IMA caches kstat.change_cookie to compare against an
> inode's i_version directly in integrity_inode_attrs_changed(), and thus
> could be out of date depending on how file systems set
> kstat.change_cookie.
>
> To address this, require integrity_inode_attrs_changed() to query
> vfs_getattr_nosec() to compare the cached version against
> kstat.change_cookie directly. This ensures that when updates occur,
> we're accessing the same changed inode version on changes, and fallback
> to compare against kstat.ctime when STATX_CHANGE_COOKIE is missing from
> result mask.
>
> Lastly, because EVM still relies on querying and caching a inode's
> i_version directly, the integrity_inode_attrs_changed() falls back to the
> original inode.i_version != cached comparison.
>
> Link: https://lore.kernel.org/all/aTspr4_h9IU4EyrR@CMGLRV3
> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Frederick Lawler <fred@cloudflare.com>
> ---
> We uncovered a case in kernels >= 6.13 where XFS is no longer updating
> struct kstat.change_cookie on i_op getattr() access calls. Instead, XFS is
> using multigrain ctime (as well as other file systems) for
> change detection in commit 1cf7e834a6fb ("xfs: switch to
> multigrain timestamps").
>
> Because file systems may implement i_version as they see fit, IMA
> caching may be behind as well as file systems that don't support/export
> i_version. Thus we're proposing to compare against the kstat.change_cookie
> directly to the cached version, and fall back to a ctime guard when
> that's not updated.
>
> EVM is largely left alone since there's no trivial way to query a file
> directly in the LSM call paths to obtain kstat.change_cookie &
> kstat.ctime to cache. Thus retains accessing i_version directly.
>
> Regression tests will be added to the Linux Test Project instead of
> selftest to help catch future file system changes that may impact
> future evaluation of IMA.
>
> I'd like this to be backported to at least 6.18 if possible.
>
> Below is a simplified test that demonstrates the issue:
>
> _fragment.config_
> CONFIG_XFS_FS=y
> CONFIG_OVERLAY_FS=y
> CONFIG_IMA=y
> CONFIG_IMA_WRITE_POLICY=y
> CONFIG_IMA_READ_POLICY=y
>
> _./test.sh_
>
> IMA_POLICY="/sys/kernel/security/ima/policy"
> TEST_BIN="/bin/date"
> MNT_BASE="/tmp/ima_test_root"
>
> mkdir -p "$MNT_BASE"
> mount -t tmpfs tmpfs "$MNT_BASE"
> mkdir -p "$MNT_BASE"/{xfs_disk,upper,work,ovl}
>
> dd if=/dev/zero of="$MNT_BASE/xfs.img" bs=1M count=300
> mkfs.xfs -q "$MNT_BASE/xfs.img"
> mount "$MNT_BASE/xfs.img" "$MNT_BASE/xfs_disk"
> cp "$TEST_BIN" "$MNT_BASE/xfs_disk/test_prog"
>
> mount -t overlay overlay -o \
> "lowerdir=$MNT_BASE/xfs_disk,upperdir=$MNT_BASE/upper,workdir=$MNT_BASE/work" \
> "$MNT_BASE/ovl"
>
> echo "audit func=BPRM_CHECK uid=$(id -u nobody)" > "$IMA_POLICY"
>
> target_prog="$MNT_BASE/ovl/test_prog"
> setpriv --reuid nobody "$target_prog"
> setpriv --reuid nobody "$target_prog"
> setpriv --reuid nobody "$target_prog"
>
> audit_count=$(dmesg | grep -c "file=\"$target_prog\"")
>
> if [[ "$audit_count" -eq 1 ]]; then
> echo "PASS: Found exactly 1 audit event."
> else
> echo "FAIL: Expected 1 audit event, but found $audit_count."
> exit 1
> fi
> ---
> Changes since RFC:
> - Remove calls to I_IS_VERSION()
> - Function documentation/comments
> - Abide IMA/EVM change detection fallback invariants
> - Combined ctime guard into version for attributes struct
> - Link to RFC: https://lore.kernel.org/r/20251229-xfs-ima-fixup-v1-1-6a717c939f7c@cloudflare.com
> ---
> Changes in v2:
> - Updated commit description + message to clarify the problem.
> - compare struct timespec64 to avoid collision possibility [Roberto].
> - Don't check inode_attr_changed() in ima_check_last_writer()
> - Link to v1: https://lore.kernel.org/r/20260112-xfs-ima-fixup-v1-1-8d13b6001312@cloudflare.com
> ---
> include/linux/integrity.h | 40 ++++++++++++++++++++++++++++++++-----
> security/integrity/evm/evm_crypto.c | 4 +++-
> security/integrity/evm/evm_main.c | 5 ++---
> security/integrity/ima/ima_api.c | 20 +++++++++++++------
> security/integrity/ima/ima_main.c | 18 ++++++++++-------
> 5 files changed, 65 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index f5842372359be5341b6870a43b92e695e8fc78af..46f57402b790c9c275b85f0b30819fa196fc20c7 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -9,6 +9,8 @@
>
> #include <linux/fs.h>
> #include <linux/iversion.h>
> +#include <linux/kernel.h>
> +#include <linux/time64.h>
>
> enum integrity_status {
> INTEGRITY_PASS = 0,
> @@ -31,6 +33,7 @@ static inline void integrity_load_keys(void)
>
> /* An inode's attributes for detection of changes */
> struct integrity_inode_attributes {
> + struct timespec64 ctime;
I found the helper timespec64_to_ns(), I think it would be better for
memory occupation perspective to fit in the version field.
Thanks
Roberto
> u64 version; /* track inode changes */
> unsigned long ino;
> dev_t dev;
> @@ -42,8 +45,10 @@ struct integrity_inode_attributes {
> */
> static inline void
> integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
> - u64 i_version, const struct inode *inode)
> + u64 i_version, struct timespec64 ctime,
> + const struct inode *inode)
> {
> + attrs->ctime = ctime;
> attrs->version = i_version;
> attrs->dev = inode->i_sb->s_dev;
> attrs->ino = inode->i_ino;
> @@ -51,14 +56,39 @@ integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
>
> /*
> * On stacked filesystems detect whether the inode or its content has changed.
> + *
> + * Must be called in process context.
> */
> static inline bool
> integrity_inode_attrs_changed(const struct integrity_inode_attributes *attrs,
> - const struct inode *inode)
> + struct file *file, struct inode *inode)
> {
> - return (inode->i_sb->s_dev != attrs->dev ||
> - inode->i_ino != attrs->ino ||
> - !inode_eq_iversion(inode, attrs->version));
> + struct kstat stat;
> +
> + might_sleep();
> +
> + if (inode->i_sb->s_dev != attrs->dev || inode->i_ino != attrs->ino)
> + return true;
> +
> + /*
> + * EVM currently relies on backing inode i_version. While IS_I_VERSION
> + * is not a good indicator of i_version support, this still retains
> + * the logic such that a re-evaluation should still occur for EVM, and
> + * only for IMA if vfs_getattr_nosec() fails.
> + */
> + if (!file || vfs_getattr_nosec(&file->f_path, &stat,
> + STATX_CHANGE_COOKIE | STATX_CTIME,
> + AT_STATX_SYNC_AS_STAT))
> + return !IS_I_VERSION(inode) ||
> + !inode_eq_iversion(inode, attrs->version);
> +
> + if (stat.result_mask & STATX_CHANGE_COOKIE)
> + return stat.change_cookie != attrs->version;
> +
> + if (stat.result_mask & STATX_CTIME)
> + return !timespec64_equal(&stat.ctime, &attrs->ctime);
> +
> + return true;
> }
>
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a5e730ffda57fbc0a91124adaa77b946a12d08b4..361ee7b216247a0d6d2f518e82fb6e92dc355afe 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -297,10 +297,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> hmac_add_misc(desc, inode, type, data->digest);
>
> if (inode != d_backing_inode(dentry) && iint) {
> + struct timespec64 ctime = {0};
> +
> if (IS_I_VERSION(inode))
> i_version = inode_query_iversion(inode);
> integrity_inode_attrs_store(&iint->metadata_inode, i_version,
> - inode);
> + ctime, inode);
> }
>
> /* Portable EVM signatures must include an IMA hash */
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 73d500a375cb37a54f295b0e1e93fd6e5d9ecddc..6a4e0e246005246d5700b1db590c1759242b9cb6 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -752,9 +752,8 @@ bool evm_metadata_changed(struct inode *inode, struct inode *metadata_inode)
> bool ret = false;
>
> if (iint) {
> - ret = (!IS_I_VERSION(metadata_inode) ||
> - integrity_inode_attrs_changed(&iint->metadata_inode,
> - metadata_inode));
> + ret = integrity_inode_attrs_changed(&iint->metadata_inode,
> + NULL, metadata_inode);
> if (ret)
> iint->evm_status = INTEGRITY_UNKNOWN;
> }
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c35ea613c9f8d404ba4886e3b736c3bab29d1668..0d8e0a3ebd34b70bb1b4cc995aae5d4adc90a585 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -254,6 +254,7 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> int length;
> void *tmpbuf;
> u64 i_version = 0;
> + struct timespec64 ctime = {0};
>
> /*
> * Always collect the modsig, because IMA might have already collected
> @@ -272,10 +273,15 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> * to an initial measurement/appraisal/audit, but was modified to
> * assume the file changed.
> */
> - result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE,
> + result = vfs_getattr_nosec(&file->f_path, &stat,
> + STATX_CHANGE_COOKIE | STATX_CTIME,
> AT_STATX_SYNC_AS_STAT);
> - if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> - i_version = stat.change_cookie;
> + if (!result) {
> + if (stat.result_mask & STATX_CHANGE_COOKIE)
> + i_version = stat.change_cookie;
> + if (stat.result_mask & STATX_CTIME)
> + ctime = stat.ctime;
> + }
> hash.hdr.algo = algo;
> hash.hdr.length = hash_digest_size[algo];
>
> @@ -305,11 +311,13 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
>
> iint->ima_hash = tmpbuf;
> memcpy(iint->ima_hash, &hash, length);
> - if (real_inode == inode)
> + if (real_inode == inode) {
> iint->real_inode.version = i_version;
> - else
> + iint->real_inode.ctime = ctime;
> + } else {
> integrity_inode_attrs_store(&iint->real_inode, i_version,
> - real_inode);
> + ctime, real_inode);
> + }
>
> /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> if (!result)
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 5770cf691912aa912fc65280c59f5baac35dd725..54b638663c9743d39e5fb65711dbd9698b38e39b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -22,12 +22,14 @@
> #include <linux/mount.h>
> #include <linux/mman.h>
> #include <linux/slab.h>
> +#include <linux/stat.h>
> #include <linux/xattr.h>
> #include <linux/ima.h>
> #include <linux/fs.h>
> #include <linux/iversion.h>
> #include <linux/evm.h>
> #include <linux/crash_dump.h>
> +#include <linux/time64.h>
>
> #include "ima.h"
>
> @@ -199,10 +201,13 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> &iint->atomic_flags);
> if ((iint->flags & IMA_NEW_FILE) ||
> vfs_getattr_nosec(&file->f_path, &stat,
> - STATX_CHANGE_COOKIE,
> - AT_STATX_SYNC_AS_STAT) ||
> - !(stat.result_mask & STATX_CHANGE_COOKIE) ||
> - stat.change_cookie != iint->real_inode.version) {
> + STATX_CHANGE_COOKIE | STATX_CTIME,
> + AT_STATX_SYNC_AS_STAT) ||
> + ((stat.result_mask & STATX_CHANGE_COOKIE) ?
> + stat.change_cookie != iint->real_inode.version :
> + (!(stat.result_mask & STATX_CTIME) ||
> + !timespec64_equal(&stat.ctime,
> + &iint->real_inode.ctime)))) {
> iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> iint->measured_pcrs = 0;
> if (update)
> @@ -328,9 +333,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
> real_inode = d_real_inode(file_dentry(file));
> if (real_inode != inode &&
> (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
> - if (!IS_I_VERSION(real_inode) ||
> - integrity_inode_attrs_changed(&iint->real_inode,
> - real_inode)) {
> + if (integrity_inode_attrs_changed(&iint->real_inode,
> + file, real_inode)) {
> iint->flags &= ~IMA_DONE_MASK;
> iint->measured_pcrs = 0;
> }
>
> ---
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> change-id: 20251212-xfs-ima-fixup-931780a62c2c
>
> Best regards,
^ permalink raw reply
* Re: [PATCH -next] lockdown: Add break in lockdown_write
From: Xiu Jianfeng @ 2026-01-22 9:31 UTC (permalink / raw)
To: Cai Xinchen, nicolas.bouchinet, paul, jmorris, serge
Cc: linux-security-module, linux-kernel
In-Reply-To: <20260119091226.3195309-1-caixinchen1@huawei.com>
On 1/19/2026 5:12 PM, Cai Xinchen wrote:
> After the label is matched successful, any other levels judgements
> are meaningless. Therefore, add break to return early
>
> Signed-off-by: Cai Xinchen <caixinchen1@huawei.com>
Looks good to me, thanks.
Acked-by: Xiu Jianfeng <xiujianfeng@huawei.com>
Paul,
Would you mind if this patch went through the LSM tree? :)
Best regards
Xiu Jianfeng
^ permalink raw reply
* Re: [PATCH] xfrm: force flush upon NETDEV_UNREGISTER event
From: Tetsuo Handa @ 2026-01-22 8:24 UTC (permalink / raw)
To: Aviad Yehezkel, Aviv Heller, Boris Pismenny, David S. Miller,
Florian Westphal, Guy Shapiro, Ilan Tayari, Kristian Evensen,
Leon Romanovsky, Leon Romanovsky, Raed Salem, Raed Salem,
Saeed Mahameed, Steffen Klassert, Yossi Kuperman
Cc: Network Development, linux-security-module
In-Reply-To: <537343f7-c580-43b0-9ad2-691701b9fb8e@I-love.SAKURA.ne.jp>
A debug patch in linux-next-20260121
( https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit?id=fc0f090e41e652d158f946c616cdd82baed3c8f4 )
has demonstrated that calling xfrm_dev_state_flush()/xfrm_dev_policy_flush()
when (dev->features & NETIF_F_HW_ESP) == 0 helps releasing "struct net_device" refcount.
unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
ref_tracker: netdev@ffff88805d3c0628 has 1/1 users at
xfrm_dev_state_add+0x6f4/0xc40 net/xfrm/xfrm_device.c:316
xfrm_state_construct net/xfrm/xfrm_user.c:986 [inline]
xfrm_add_sa+0x34ca/0x4230 net/xfrm/xfrm_user.c:1022
xfrm_user_rcv_msg+0x746/0xb20 net/xfrm/xfrm_user.c:3507
netlink_rcv_skb+0x232/0x4b0 net/netlink/af_netlink.c:2550
xfrm_netlink_rcv+0x79/0x90 net/xfrm/xfrm_user.c:3529
netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
netlink_unicast+0x80f/0x9b0 net/netlink/af_netlink.c:1344
netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1894
sock_sendmsg_nosec+0x18f/0x1d0 net/socket.c:737
__sock_sendmsg net/socket.c:752 [inline]
____sys_sendmsg+0x589/0x8c0 net/socket.c:2610
___sys_sendmsg+0x2a5/0x360 net/socket.c:2664
__sys_sendmsg net/socket.c:2696 [inline]
__do_sys_sendmsg net/socket.c:2701 [inline]
__se_sys_sendmsg net/socket.c:2699 [inline]
__x64_sys_sendmsg+0x1bd/0x2a0 net/socket.c:2699
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xe2/0xf80 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
infiniband: balance for netdevsim0@ib_gid_table_entry is 0
***** Releasing 1 refcount on 0000000000000000
***** Refcount for netdevsim0 changed from 2 to 1
The bond_master_netdev_event(NETDEV_UNREGISTER) case is already calling
xfrm_dev_state_flush() without checking (dev->features & NETIF_F_HW_ESP) != 0.
Therefore, I assume that (dev->features & NETIF_F_HW_ESP) != 0 check in
xfrm_dev_down() is wrong, and I would like to propose
static int xfrm_dev_down(struct net_device *dev)
{
- if (dev->features & NETIF_F_HW_ESP) {
- xfrm_dev_state_flush(dev_net(dev), dev, true);
- xfrm_dev_policy_flush(dev_net(dev), dev, true);
- }
+ xfrm_dev_state_flush(dev_net(dev), dev, true);
+ xfrm_dev_policy_flush(dev_net(dev), dev, true);
return NOTIFY_DONE;
}
change as a fix for "unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2"
problem.
But I have a question regarding security_xfrm_state_delete()/security_xfrm_policy_delete().
xfrm_dev_state_flush_secctx_check() calls security_xfrm_state_delete() which can make
xfrm_dev_state_flush() no-op by returning an error value.
xfrm_dev_policy_flush_secctx_check() calls security_xfrm_policy_delete() which can make
xfrm_dev_policy_flush() no-op by returning an error value.
Since xfrm_dev_state_flush()/xfrm_dev_policy_flush() are called by NETDEV_UNREGISTER
event (which is a signal for releasing all resources that prevent "struct net_device"
references from dropping), making xfrm_dev_state_flush()/xfrm_dev_policy_flush() no-op (by
allowing security_xfrm_state_delete()/security_xfrm_policy_delete() to return an error) is
a denial-of-service bug.
Therefore, I wonder what are security_xfrm_state_delete() and security_xfrm_policy_delete()
for. Can I kill xfrm_dev_state_flush_secctx_check() and xfrm_dev_policy_flush_secctx_check() ?
^ permalink raw reply
* [PATCH] evm: Use ordered xattrs list to calculate HMAC in evm_init_hmac()
From: Roberto Sassu @ 2026-01-22 8:07 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, eric.snowberg, paul, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu
From: Roberto Sassu <roberto.sassu@huawei.com>
Commit 8e5d9f916a96 ("smack: deduplicate xattr setting in
smack_inode_init_security()") introduced xattr_dupval() to simplify setting
the xattrs to be provided by the SMACK LSM on inode creation, in the
smack_inode_init_security().
Unfortunately, moving lsm_get_xattr_slot() caused the SMACK64TRANSMUTE
xattr be added in the array of new xattrs before SMACK64. This causes the
HMAC of xattrs calculated by evm_init_hmac() for new files to diverge from
the one calculated by both evm_calc_hmac_or_hash() and evmctl.
evm_init_hmac() calculates the HMAC of the xattrs of new files based on the
order LSMs provide them, while evm_calc_hmac_or_hash() and evmctl calculate
the HMAC based on an ordered xattrs list.
Fix the issue by making evm_init_hmac() calculate the HMAC of new files
based on the ordered xattrs list too.
Fixes: 8e5d9f916a96 ("smack: deduplicate xattr setting in smack_inode_init_security()")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/evm/evm_crypto.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a5e730ffda57..5a8cef45bacf 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -401,6 +401,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
{
struct shash_desc *desc;
const struct xattr *xattr;
+ struct xattr_list *xattr_entry;
desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
if (IS_ERR(desc)) {
@@ -408,11 +409,16 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
return PTR_ERR(desc);
}
- for (xattr = xattrs; xattr->name; xattr++) {
- if (!evm_protected_xattr(xattr->name))
- continue;
+ list_for_each_entry_lockless(xattr_entry, &evm_config_xattrnames,
+ list) {
+ for (xattr = xattrs; xattr->name; xattr++) {
+ if (strcmp(xattr_entry->name +
+ XATTR_SECURITY_PREFIX_LEN, xattr->name) != 0)
+ continue;
- crypto_shash_update(desc, xattr->value, xattr->value_len);
+ crypto_shash_update(desc, xattr->value,
+ xattr->value_len);
+ }
}
hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH tip/locking/core 0/6] compiler-context-analysis: Scoped init guards
From: Christoph Hellwig @ 2026-01-22 6:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Christoph Hellwig, Marco Elver, Ingo Molnar, Thomas Gleixner,
Will Deacon, Boqun Feng, Waiman Long, Steven Rostedt,
Bart Van Assche, kasan-dev, llvm, linux-crypto, linux-doc,
linux-security-module, linux-kernel
In-Reply-To: <20260120105211.GW830755@noisy.programming.kicks-ass.net>
On Tue, Jan 20, 2026 at 11:52:11AM +0100, Peter Zijlstra wrote:
> > So I think the first step is to avoid implying the safety of guarded
> > member access by initialing the lock. We then need to think how to
> > express they are save, which would probably require explicit annotation
> > unless we can come up with a scheme that makes these accesses fine
> > before the mutex_init in a magic way.
>
> But that is exactly what these patches do!
>
> Note that the current state of things (tip/locking/core,next) is that
> mutex_init() is 'special'. And I agree with you that that is quite
> horrible.
>
> Now, these patches, specifically patch 6, removes this implied
> horribleness.
>
> The alternative is an explicit annotation -- as you suggest.
>
>
> So given something like:
>
> struct my_obj {
> struct mutex mutex;
> int data __guarded_by(&mutex);
> ...
> };
>
>
> tip/locking/core,next:
>
> init_my_obj(struct my_obj *obj)
> {
> mutex_init(&obj->mutex); // implies obj->mutex is taken until end of function
> obj->data = FOO; // OK, because &obj->mutex 'held'
> ...
> }
>
> And per these patches that will no longer be true. So if you apply just
> patch 6, which removes this implied behaviour, you get a compile fail.
> Not good!
>
> So patches 1-5 introduces alternatives.
>
> So your preferred solution:
>
> hch_my_obj(struct my_obj *obj)
> {
> mutex_init(&obj->mutex);
> mutex_lock(&obj->mutex); // actually acquires lock
> obj->data = FOO;
> ...
> }
>
> is perfectly fine and will work. But not everybody wants this. For the
> people that only need to init the data fields and don't care about the
> lock state we get:
>
> init_my_obj(struct my_obj *obj)
> {
> guard(mutex_init)(&obj->mutex); // initializes mutex and considers lock
> // held until end of function
> obj->data = FOO;
> ...
> }
And this is just as bad as the original version, except it is now
even more obfuscated.
> And for the people that *reaaaaaly* hate guards, it is possible to write
> something like:
>
> ugly_my_obj(struct my_obj *obj)
> {
> mutex_init(&obj->mutex);
> __acquire_ctx_lock(&obj->mutex);
> obj->data = FOO;
> ...
> __release_ctx_lock(&obj->mutex);
>
> mutex_lock(&obj->lock);
> ...
That's better. What would be even better for everyone would be:
mutex_prepare(&obj->mutex); /* acquire, but with a nice name */
obj->data = FOO;
mutex_init_prepared(&obj->mutex); /* release, barrier, actual init */
mutex_lock(&obj->mutex); /* IFF needed only */
^ permalink raw reply
* Re: [PATCH net-next v2 0/4] net: uapi: Provide an UAPI definition of 'struct sockaddr'
From: Jakub Kicinski @ 2026-01-22 3:27 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
David S. Miller, Simon Horman, Shuah Khan, Matthieu Baerts,
Mat Martineau, Geliang Tang, Mickaël Salaün,
Günther Noack, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Hao Luo, Jiri Olsa, netdev, linux-kernel,
linux-api, Arnd Bergmann, linux-kselftest, mptcp,
linux-security-module, bpf, libc-alpha, Carlos O'Donell,
Adhemerval Zanella, Rich Felker, klibc, Florian Weimer
In-Reply-To: <20260120-uapi-sockaddr-v2-0-63c319111cf6@linutronix.de>
On Tue, 20 Jan 2026 15:10:30 +0100 Thomas Weißschuh wrote:
> Various UAPI headers reference 'struct sockaddr'. Currently the
> definition of this struct is pulled in from the libc header
> sys/socket.h. This is problematic as it introduces a dependency
> on a full userspace toolchain.
>
> Add a definition of 'struct sockaddr' to the UAPI headers.
> Before that, reorder some problematic header inclusions in the selftests.
> include/linux/socket.h | 10 ----------
> include/uapi/linux/if.h | 4 ----
> include/uapi/linux/libc-compat.h | 12 ++++++++++++
> include/uapi/linux/socket.h | 14 ++++++++++++++
> samples/bpf/xdp_adjust_tail_user.c | 6 ++++--
> samples/bpf/xdp_fwd_user.c | 7 ++++---
> samples/bpf/xdp_router_ipv4_user.c | 6 +++---
> samples/bpf/xdp_sample_user.c | 15 ++++++++-------
> samples/bpf/xdp_tx_iptunnel_user.c | 4 ++--
> tools/testing/selftests/landlock/audit.h | 7 ++++---
> tools/testing/selftests/net/af_unix/diag_uid.c | 9 +++++----
> tools/testing/selftests/net/busy_poller.c | 3 ++-
> tools/testing/selftests/net/mptcp/mptcp_diag.c | 11 ++++++-----
> tools/testing/selftests/net/nettest.c | 4 ++--
> tools/testing/selftests/net/tcp_ao/icmps-discard.c | 6 +++---
> tools/testing/selftests/net/tcp_ao/lib/netlink.c | 9 +++++----
> tools/testing/selftests/net/tun.c | 5 +++--
> 17 files changed, 77 insertions(+), 55 deletions(-)
Are all those selftests / samples getting broken by this patch set?
I understand that we should avoid libc dependencies in uAPI but at
least speaking for networking - building selftests without libc is..
not a practical proposition?
^ permalink raw reply
* Re: [LSF/MM/BPF TOPIC] Refactor LSM hooks for VFS mount operations
From: Song Liu @ 2026-01-22 3:00 UTC (permalink / raw)
To: Paul Moore
Cc: bpf, Linux-Fsdevel, lsf-pc, linux-security-module,
Christian Brauner, Al Viro
In-Reply-To: <CAHC9VhRU_vtN4oXHVuT4Tt=WFP=4FrKc=i8t=xDz+bamUG7r6g@mail.gmail.com>
Hi Paul,
On Wed, Jan 21, 2026 at 4:14 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Jan 21, 2026 at 4:18 PM Song Liu <song@kernel.org> wrote:
> >
> > Current LSM hooks do not have good coverage for VFS mount operations.
> > Specifically, there are the following issues (and maybe more..):
>
> I don't recall LSM folks normally being invited to LSFMMBPF so it
> seems like this would be a poor forum to discuss LSM hooks.
Agreed this might not be the best forum to discuss LSM hooks.
However, I am not aware of a better forum for in person discussions.
AFAICT, in-tree LSMs have straightforward logics around mount
monitoring. As long as we get these logic translated properly, I
don't expect much controversy with in-tree LSMs.
> > PS: I am not sure whether other folks are already working on it. I will prepare
> > some RFC patches before the conference if I don't see other proposals.
>
> FWIW, I'm not aware of anyone currently working on revising the mount
> hooks, but it's possible. Posting a patchset, even an early RFC
> draft, is always a good way to find out who might be working in the
> same space :)
>
> Posting to the mailing list also has the advantage of reaching
> everyone who might be interested, whereas discussing this at a
> conference, especially one that is invite-only, is limiting.
I expect there will be RFCs posted to the mailing list before the
conference. We will incorporate feedbacks from the mailing list
to make the discussion more productive at the conference. It is
totally possible that some patches get accepted before the
conference, so that we can simply celebrate at the conference. :)
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] cipso: harden use of skb_cow() in cipso_v4_skbuff_setattr()
From: Paul Moore @ 2026-01-22 0:48 UTC (permalink / raw)
To: Will Rosenberg
Cc: David S. Miller, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, netdev, linux-security-module,
linux-kernel
In-Reply-To: <20260120155738.982771-1-whrosenb@asu.edu>
On Tue, Jan 20, 2026 at 10:57 AM Will Rosenberg <whrosenb@asu.edu> wrote:
>
> If skb_cow() is passed a headroom <= -NET_SKB_PAD, it will trigger a
> BUG. As a result, use cases should avoid calling with a headroom that
> is negative to prevent triggering this issue.
>
> This is the same code pattern fixed in Commit 58fc7342b529 ("ipv6:
> BUG() in pskb_expand_head() as part of calipso_skbuff_setattr()").
>
> In cipso_v4_skbuff_setattr(), len_delta can become negative, leading to
> a negative headroom passed to skb_cow(). However, the BUG is not
> triggerable because the condition headroom <= -NET_SKB_PAD cannot be
> satisfied due to limits on the IPv4 options header size.
>
> Avoid potential problems in the future by only using skb_cow() to grow
> the skb headroom.
>
> Signed-off-by: Will Rosenberg <whrosenb@asu.edu>
> ---
>
> Notes:
> Given that IPv4 option length should not change,
> this may not be a worthwhile patch.
>
> Apologies in advance if this ends up being a waste
> of time.
>
> net/ipv4/cipso_ipv4.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
I think it's a reasonable thing to do in an effort to avoid future
problems. Thanks Will.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 709021197e1c..32b951ebc0c2 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -2196,7 +2196,8 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
> /* if we don't ensure enough headroom we could panic on the skb_push()
> * call below so make sure we have enough, we are also "mangling" the
> * packet so we should probably do a copy-on-write call anyway */
> - ret_val = skb_cow(skb, skb_headroom(skb) + len_delta);
> + ret_val = skb_cow(skb,
> + skb_headroom(skb) + (len_delta > 0 ? len_delta : 0));
> if (ret_val < 0)
> return ret_val;
>
>
> base-commit: 58bae918d73e3b6cd57d1e39fcf7c75c7dd1a8fe
> --
> 2.34.1
--
paul-moore.com
^ permalink raw reply
* Re: [LSF/MM/BPF TOPIC] Refactor LSM hooks for VFS mount operations
From: Paul Moore @ 2026-01-22 0:14 UTC (permalink / raw)
To: Song Liu
Cc: bpf, Linux-Fsdevel, lsf-pc, linux-security-module,
Christian Brauner, Al Viro
In-Reply-To: <CAPhsuW4=heDwYEkmRzSnLHDdW=da71qDd1KqUj9sYUOT5uOx3w@mail.gmail.com>
On Wed, Jan 21, 2026 at 4:18 PM Song Liu <song@kernel.org> wrote:
>
> Current LSM hooks do not have good coverage for VFS mount operations.
> Specifically, there are the following issues (and maybe more..):
I don't recall LSM folks normally being invited to LSFMMBPF so it
seems like this would be a poor forum to discuss LSM hooks.
> PS: I am not sure whether other folks are already working on it. I will prepare
> some RFC patches before the conference if I don't see other proposals.
FWIW, I'm not aware of anyone currently working on revising the mount
hooks, but it's possible. Posting a patchset, even an early RFC
draft, is always a good way to find out who might be working in the
same space :)
Posting to the mailing list also has the advantage of reaching
everyone who might be interested, whereas discussing this at a
conference, especially one that is invite-only, is limiting.
--
paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
From: Justin Suess @ 2026-01-21 23:08 UTC (permalink / raw)
To: Tingmao Wang, Günther Noack, Mickaël Salaün
Cc: linux-security-module, Samasth Norway Ananda, Matthieu Buffet,
Mikhail Ivanov, konstantin.meskhidze
In-Reply-To: <6a789aa9-c479-43f9-ac24-bc227f8388c6@maowtm.org>
On 1/20/26 19:26, Tingmao Wang wrote:
> On 12/30/25 10:39, Günther Noack wrote:
>> The layer masks data structure tracks the requested but unfulfilled
>> access rights during an operations security check. It stores one bit
>> for each combination of access right and layer index. If the bit is
>> set, that access right is not granted (yet) in the given layer and we
>> have to traverse the path further upwards to grant it.
>>
>> Previously, the layer masks were stored as arrays mapping from access
>> right indices to layer_mask_t. The layer_mask_t value then indicates
>> all layers in which the given access right is still (tentatively)
>> denied.
>>
>> This patch introduces struct layer_access_masks instead: This struct
>> contains an array with the access_mask_t of each (tentatively) denied
>> access right in that layer.
>>
>> The hypothesis of this patch is that this simplifies the code enough
>> so that the resulting code will run faster:
>>
>> * We can use bitwise operations in multiple places where we previously
>> looped over bits individually with macros. (Should require less
>> branch speculation)
>>
>> * Code is ~160 lines smaller.
>>
>> Other noteworthy changes:
>>
>> * Clarify deny_mask_t and the code assembling it.
>> * Document what that value looks like
>> * Make writing and reading functions specific to file system rules.
>> (It only worked for FS rules before as well, but going all the way
>> simplifies the code logic more.)
> In the original commit message that added this type [1] there was this
> statement:
>
>> Implementing deny_masks_t with a bitfield instead of a struct enables a
>> generic implementation to store and extract layer levels.
> At some point when looking at this I was wondering why this wasn't a
> struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code.
> While it is possible that I might have just misunderstood it, reading the
> above statement my take-away was that a struct would have forced us to
> address the indices with specific names, e.g. it would need to be defined
> like
>
> struct deny_masks_t {
> u8 ioctl:4;
> u8 truncate:4;
> }
>
> And it would thus not be possible to manipulate the indices in a generic
> way (e.g. the way it was implemented before, given
> all_existing_optional_access and access_bit, read and write the right
> bits).
>
> However, since we're now removing that generic-ability, should we consider
> turning it into a struct? (If later on we have different access types
> that also have optional accesses, we could use a union of structs)
>
>
> btw, since this causes conflicts with the quiet flag series and Mickaël
> has indicated that this should be merged first, I will probably have to
> make my series based on top of this. Will watch this series to see if
> there are more changes.
Likewise for my NO_INHERIT series, which will need some rebase work as
well. (my series is built on the quiet flag series, to reuse the similar "bubble up"
flag collection logic).
I'll keep an eye on your tree Tingmao and start rebasing my NO_INHERIT
on your patches if you put your work there. (Otherwise I'll do it when you
send it on the mailing list)
^ permalink raw reply
* Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
From: Mickaël Salaün @ 2026-01-21 22:27 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Justin Suess, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze
In-Reply-To: <6a789aa9-c479-43f9-ac24-bc227f8388c6@maowtm.org>
On Wed, Jan 21, 2026 at 12:26:52AM +0000, Tingmao Wang wrote:
> On 12/30/25 10:39, Günther Noack wrote:
> > The layer masks data structure tracks the requested but unfulfilled
> > access rights during an operations security check. It stores one bit
> > for each combination of access right and layer index. If the bit is
> > set, that access right is not granted (yet) in the given layer and we
> > have to traverse the path further upwards to grant it.
> >
> > Previously, the layer masks were stored as arrays mapping from access
> > right indices to layer_mask_t. The layer_mask_t value then indicates
> > all layers in which the given access right is still (tentatively)
> > denied.
> >
> > This patch introduces struct layer_access_masks instead: This struct
> > contains an array with the access_mask_t of each (tentatively) denied
> > access right in that layer.
> >
> > The hypothesis of this patch is that this simplifies the code enough
> > so that the resulting code will run faster:
> >
> > * We can use bitwise operations in multiple places where we previously
> > looped over bits individually with macros. (Should require less
> > branch speculation)
> >
> > * Code is ~160 lines smaller.
> >
> > Other noteworthy changes:
> >
> > * Clarify deny_mask_t and the code assembling it.
> > * Document what that value looks like
> > * Make writing and reading functions specific to file system rules.
> > (It only worked for FS rules before as well, but going all the way
> > simplifies the code logic more.)
>
> In the original commit message that added this type [1] there was this
> statement:
>
> > Implementing deny_masks_t with a bitfield instead of a struct enables a
> > generic implementation to store and extract layer levels.
>
> At some point when looking at this I was wondering why this wasn't a
> struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code.
> While it is possible that I might have just misunderstood it, reading the
> above statement my take-away was that a struct would have forced us to
> address the indices with specific names, e.g. it would need to be defined
> like
>
> struct deny_masks_t {
> u8 ioctl:4;
> u8 truncate:4;
> }
>
> And it would thus not be possible to manipulate the indices in a generic
> way (e.g. the way it was implemented before, given
> all_existing_optional_access and access_bit, read and write the right
> bits).
>
> However, since we're now removing that generic-ability, should we consider
> turning it into a struct? (If later on we have different access types
> that also have optional accesses, we could use a union of structs)
I would prefer to have a more generic implementation, or at least to
make it easy to add this kind of access rights. Any idea how to improve
the situation?
>
>
> btw, since this causes conflicts with the quiet flag series and Mickaël
> has indicated that this should be merged first, I will probably have to
> make my series based on top of this. Will watch this series to see if
> there are more changes.
I'd like to make sure your quiet flag series is still OK with this
patch, and what would be the impact, so yes, please review and
experiment with this series.
>
> Also, this transpose and code simplification should also simplify the
> mutable domains work so thanks for the refactor!
Good :)
>
> A while ago I also made some benchmarking script which I sent a PR to
> landlock-test-tools [2], and earlier I tested this patch with it, and saw
> some improvement (but it was much less in terms of percentage, which may
> be due to the lower directory depth, or may be due to other unknown
> reason):
>
> TestDescription(landlock=True, dir_depth=10, nb_extra_rules=10)
> base.2:
> c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1718.15, min=1663.00, max=275949.00, median=1696.46, stddev=437.52
> 95% confidence interval: [1718.03 .. 1718.28]
> Estimated landlock overhead (vs no-landlock): 226.5%
> 48bd90e91fe6.2:
> c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1709.60, min=1633.00, max=280608.00, median=1688.83, stddev=441.83
> 95% confidence interval: [1709.48 .. 1709.73]
> ** Improved 0.5% **
> ...
> [1660 .. 1669]: [1660 .. 1669]: ###
> [1670 .. 1679]: ## [1670 .. 1679]: ###############
> [1680 .. 1689]: ###################### [1680 .. 1689]: #################################
> [1690 .. 1699]: ######################################## [1690 .. 1699]: ##################################
> [1700 .. 1709]: ############################ [1700 .. 1709]: #############
> [1710 .. 1719]: ######### [1710 .. 1719]: ##
> [1720 .. 1729]: ## [1720 .. 1729]:
> ...
> Estimated landlock overhead (vs no-landlock): 223.0%
>
> TestDescription(landlock=True, dir_depth=29, nb_extra_rules=10)
> base.2:
> c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3869.66, min=3727.00, max=272563.00, median=3813.42, stddev=666.18
> 95% confidence interval: [3869.47 .. 3869.86]
> Estimated landlock overhead (vs no-landlock): 427.3%
> 48bd90e91fe6.2:
> c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3855.61, min=3697.00, max=271690.00, median=3804.82, stddev=682.74
> 95% confidence interval: [3855.41 .. 3855.81]
> ** Improved 0.4% **
> ...
> [3750 .. 3759]: [3750 .. 3759]: #
> [3760 .. 3769]: [3760 .. 3769]: #######
> [3770 .. 3779]: [3770 .. 3779]: ###############
> [3780 .. 3789]: #### [3780 .. 3789]: ###################
> [3790 .. 3799]: ################### [3790 .. 3799]: ###################
> [3800 .. 3809]: ###################################### [3800 .. 3809]: ########################
> [3810 .. 3819]: ######################################## [3810 .. 3819]: ############################
> [3820 .. 3829]: ########################## [3820 .. 3829]: #####################
> [3830 .. 3839]: ############# [3830 .. 3839]: #########
> [3840 .. 3849]: ###### [3840 .. 3849]: ##
> [3850 .. 3859]: ## [3850 .. 3859]:
> [3860 .. 3869]: [3860 .. 3869]:
> [3870 .. 3879]: [3870 .. 3879]:
> ...
> [4980 .. 4989]: [4980 .. 4989]:
> [4990 .. 4999]: [4990 .. 4999]:
> [5000 .. 272563]: # [5000 .. 271690]: #
> Estimated landlock overhead (vs no-landlock): 424.2%
Thanks for the benchmark.
>
> Full data including test with 0 depth, or 1000 rules:
> https://fileshare.maowtm.org/landlock-20251230/index.html
>
>
> [1]: https://lore.kernel.org/all/20250320190717.2287696-15-mic@digikod.net/
> [2]: https://github.com/landlock-lsm/landlock-test-tools/pull/17
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
From: Mickaël Salaün @ 2026-01-21 22:22 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: linux-security-module, Justin Suess, Samasth Norway Ananda,
Matthieu Buffet, Mikhail Ivanov, konstantin.meskhidze
In-Reply-To: <20251230103917.10549-7-gnoack3000@gmail.com>
The goal of the initial design was to minimize the amount of memory wrt
the number of different access rights because the maximum number of
layers is 16 whereas access rights could grow up to 64.
Transposing the matrix increases the memory footprint in theory but
because we still need the struct layer_access_masks matrix, it should
actually be better. See stack usage delta with audit (generated with
check-linux.sh):
landlock_unmask_layers 208 80 -128
landlock_init_layer_masks 192 96 -96
landlock_log_denial 176 80 -96
current_check_access_path 336 304 -32
current_check_refer_path 592 560 -32
hook_file_open 352 320 -32
hook_file_send_sigiotask 176 160 -16
hook_file_truncate 112 96 -16
hook_move_mount 128 112 -16
hook_ptrace_access_check 192 176 -16
hook_ptrace_traceme 160 144 -16
hook_sb_mount 128 112 -16
hook_sb_pivotroot 128 112 -16
hook_sb_remount 128 112 -16
hook_sb_umount 128 112 -16
hook_task_kill 176 160 -16
current_check_access_socket 336 352 +16
is_access_to_paths_allowed 384 400 +16
...and stack usage delta without audit:
landlock_unmask_layers 208 80 -128
landlock_init_layer_masks 192 96 -96
hook_file_open 208 192 -16
current_check_access_socket 176 208 +32
However, when we'll add the next access right, access_mask_t will be u32
instead of u16, and stack usage delta will increase:
current_check_access_socket 352 384 +32
hook_file_open 320 352 +32
current_check_access_path 304 352 +48
current_check_refer_path 560 608 +48
is_access_to_paths_allowed 400 464 +64
Even if the cumulative stack usage delta seems reasonable, the commit
message should talk about these drawbacks.
I think the improved compiled code, and the overall simplification are
worth it.
On Tue, Dec 30, 2025 at 11:39:21AM +0100, Günther Noack wrote:
> The layer masks data structure tracks the requested but unfulfilled
> access rights during an operations security check. It stores one bit
operation?
> for each combination of access right and layer index. If the bit is
> set, that access right is not granted (yet) in the given layer and we
> have to traverse the path further upwards to grant it.
>
> Previously, the layer masks were stored as arrays mapping from access
> right indices to layer_mask_t. The layer_mask_t value then indicates
> all layers in which the given access right is still (tentatively)
> denied.
>
> This patch introduces struct layer_access_masks instead: This struct
> contains an array with the access_mask_t of each (tentatively) denied
> access right in that layer.
>
> The hypothesis of this patch is that this simplifies the code enough
> so that the resulting code will run faster:
>
> * We can use bitwise operations in multiple places where we previously
> looped over bits individually with macros. (Should require less
> branch speculation)
>
> * Code is ~160 lines smaller.
>
> Other noteworthy changes:
>
> * Clarify deny_mask_t and the code assembling it.
> * Document what that value looks like
> * Make writing and reading functions specific to file system rules.
> (It only worked for FS rules before as well, but going all the way
> simplifies the code logic more.)
> * In no_more_access(), call a new helper function may_refer(), which
> only solves the asymmetric case. Previously, the code interleaved
> the checks for the two symmetric cases in RENAME_EXCHANGE. It feels
> that the code is clearer when renames without RENAME_EXCHANGE are
> more obviously the normal case.
>
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
> security/landlock/access.h | 10 +-
> security/landlock/audit.c | 155 ++++++----------
> security/landlock/audit.h | 3 +-
> security/landlock/domain.c | 120 +++----------
> security/landlock/domain.h | 6 +-
> security/landlock/fs.c | 350 ++++++++++++++++--------------------
> security/landlock/net.c | 10 +-
> security/landlock/ruleset.c | 78 +++-----
> security/landlock/ruleset.h | 18 +-
> 9 files changed, 290 insertions(+), 460 deletions(-)
>
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d7..aa0efa36a37db 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -61,14 +61,14 @@ union access_masks_all {
> static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
> sizeof(typeof_member(union access_masks_all, all)));
>
> -typedef u16 layer_mask_t;
> -
> -/* Makes sure all layers can be checked. */
> -static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
> -
> /*
> * Tracks domains responsible of a denied access. This is required to avoid
> * storing in each object the full layer_masks[] required by update_request().
> + *
> + * Each nibble represents the layer index of the newest layer which denied a
> + * certain access right. For file system access rights, the upper four bits are
> + * the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the
> + * lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE.
> */
> typedef u8 deny_masks_t;
>
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index c52d079cdb77b..650bd7f5cb6be 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -182,36 +182,18 @@ static void test_get_hierarchy(struct kunit *const test)
>
> static size_t get_denied_layer(const struct landlock_ruleset *const domain,
> access_mask_t *const access_request,
> - const layer_mask_t (*const layer_masks)[],
> - const size_t layer_masks_size)
> + const struct layer_access_masks *masks)
> {
> - const unsigned long access_req = *access_request;
> - unsigned long access_bit;
> - access_mask_t missing = 0;
> - long youngest_layer = -1;
> -
> - for_each_set_bit(access_bit, &access_req, layer_masks_size) {
> - const access_mask_t mask = (*layer_masks)[access_bit];
> - long layer;
> -
> - if (!mask)
> - continue;
> -
> - /* __fls(1) == 0 */
> - layer = __fls(mask);
> - if (layer > youngest_layer) {
> - youngest_layer = layer;
> - missing = BIT(access_bit);
> - } else if (layer == youngest_layer) {
> - missing |= BIT(access_bit);
> + for (int i = LANDLOCK_MAX_NUM_LAYERS - 1; i >= 0; i--) {
All the loop indexes should be size_t (same as before).
Instead of LANDLOCK_MAX_NUM_LAYERS, ARRAY_SIZE(masks->access) would be
better.
> + if (masks->access[i] & *access_request) {
> + *access_request &= masks->access[i];
> + return i;
> }
> }
>
> - *access_request = missing;
> - if (youngest_layer == -1)
> - return domain->num_layers - 1;
> -
> - return youngest_layer;
> + /* Not found - fall back to default values */
> + *access_request = 0;
> + return domain->num_layers - 1;
> }
>
> #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> @@ -221,94 +203,82 @@ static void test_get_denied_layer(struct kunit *const test)
> const struct landlock_ruleset dom = {
> .num_layers = 5,
> };
> - const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = BIT(1),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = BIT(1) | BIT(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = BIT(2),
> + const struct layer_access_masks masks = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE |
> + LANDLOCK_ACCESS_FS_READ_DIR,
> + .access[1] = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_READ_DIR,
> + .access[2] = LANDLOCK_ACCESS_FS_REMOVE_DIR,
> };
> access_mask_t access;
>
> access = LANDLOCK_ACCESS_FS_EXECUTE;
> - KUNIT_EXPECT_EQ(test, 0,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 0, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_EXECUTE);
>
> access = LANDLOCK_ACCESS_FS_READ_FILE;
> - KUNIT_EXPECT_EQ(test, 1,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 1, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_FILE);
>
> access = LANDLOCK_ACCESS_FS_READ_DIR;
> - KUNIT_EXPECT_EQ(test, 1,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 1, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_DIR);
>
> access = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR;
> - KUNIT_EXPECT_EQ(test, 1,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 1, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access,
> LANDLOCK_ACCESS_FS_READ_FILE |
> LANDLOCK_ACCESS_FS_READ_DIR);
>
> access = LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_READ_DIR;
> - KUNIT_EXPECT_EQ(test, 1,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 1, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_DIR);
>
> access = LANDLOCK_ACCESS_FS_WRITE_FILE;
> - KUNIT_EXPECT_EQ(test, 4,
> - get_denied_layer(&dom, &access, &layer_masks,
> - sizeof(layer_masks)));
> + KUNIT_EXPECT_EQ(test, 4, get_denied_layer(&dom, &access, &masks));
> KUNIT_EXPECT_EQ(test, access, 0);
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
>
> -static size_t
> -get_layer_from_deny_masks(access_mask_t *const access_request,
> - const access_mask_t all_existing_optional_access,
> - const deny_masks_t deny_masks)
> +/*
> + * get_layer_from_fs_deny_masks - get the layer which denied the access request
> + *
> + * As a side effect, stores the denied access rights from that layer(!) in
> + * *access_request.
> + */
> +static size_t get_layer_from_fs_deny_masks(access_mask_t *const access_request,
> + const deny_masks_t deny_masks)
I'm not a fan of this change. We come from a generic approach to a
specific and hardcoded one. This is simpler *for now*, but could we get
a better implementation?
Anyway, please create at least a dedicated patch for the
non-transposition changes.
> {
> - const unsigned long access_opt = all_existing_optional_access;
> - const unsigned long access_req = *access_request;
> - access_mask_t missing = 0;
> + const access_mask_t access_req = *access_request;
> size_t youngest_layer = 0;
> - size_t access_index = 0;
> - unsigned long access_bit;
> + access_mask_t missing = 0;
>
> - /* This will require change with new object types. */
> - WARN_ON_ONCE(access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL);
> + WARN_ON_ONCE((access_req | _LANDLOCK_ACCESS_FS_OPTIONAL) !=
> + _LANDLOCK_ACCESS_FS_OPTIONAL);
>
> - 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);
> + if (access_req & LANDLOCK_ACCESS_FS_TRUNCATE) {
> + size_t layer = deny_masks & 0x0f;
>
> - if (layer > youngest_layer) {
> - youngest_layer = layer;
> - missing = BIT(access_bit);
> - } else if (layer == youngest_layer) {
> - missing |= BIT(access_bit);
> - }
> - }
> - access_index++;
> + missing |= LANDLOCK_ACCESS_FS_TRUNCATE;
> + youngest_layer = max(youngest_layer, layer);
> }
Add a new line.
> + if (access_req & LANDLOCK_ACCESS_FS_IOCTL_DEV) {
> + size_t layer = (deny_masks & 0xf0) >> 4;
>
> + if (layer > youngest_layer)
> + missing = 0;
> +
> + missing |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + youngest_layer = max(youngest_layer, layer);
> + }
Add a new line.
> *access_request = missing;
> return youngest_layer;
> }
>
> #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
>
> -static void test_get_layer_from_deny_masks(struct kunit *const test)
> +static void test_get_layer_from_fs_deny_masks(struct kunit *const test)
> {
> deny_masks_t deny_mask;
> access_mask_t access;
> @@ -318,16 +288,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test)
>
> 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_fs_deny_masks(&access, deny_mask));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>
> 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_fs_deny_masks(&access, deny_mask));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
>
> /* truncate:15 ioctl_dev:15 */
> @@ -335,16 +301,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test)
>
> 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_fs_deny_masks(&access, deny_mask));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
>
> 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_fs_deny_masks(&access, deny_mask));
> KUNIT_EXPECT_EQ(test, access,
> LANDLOCK_ACCESS_FS_TRUNCATE |
> LANDLOCK_ACCESS_FS_IOCTL_DEV);
> @@ -361,18 +323,15 @@ static bool is_valid_request(const struct landlock_request *const request)
> return false;
>
> if (request->access) {
> - if (WARN_ON_ONCE(!(!!request->layer_masks ^
> + if (WARN_ON_ONCE(!(!!request->masks ^
> !!request->all_existing_optional_access)))
> return false;
> } else {
> - if (WARN_ON_ONCE(request->layer_masks ||
> + if (WARN_ON_ONCE(request->masks ||
> request->all_existing_optional_access))
> return false;
> }
>
> - if (WARN_ON_ONCE(!!request->layer_masks ^ !!request->layer_masks_size))
> - return false;
> -
> if (request->deny_masks) {
> if (WARN_ON_ONCE(!request->all_existing_optional_access))
> return false;
> @@ -405,14 +364,12 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> missing = request->access;
> if (missing) {
> /* Gets the nearest domain that denies the request. */
> - if (request->layer_masks) {
> + if (request->masks) {
> youngest_layer = get_denied_layer(
> - subject->domain, &missing, request->layer_masks,
> - request->layer_masks_size);
> + subject->domain, &missing, request->masks);
> } else {
> - youngest_layer = get_layer_from_deny_masks(
> - &missing, request->all_existing_optional_access,
> - request->deny_masks);
> + youngest_layer = get_layer_from_fs_deny_masks(
> + &missing, request->deny_masks);
> }
> youngest_denied =
> get_hierarchy(subject->domain, youngest_layer);
> @@ -507,7 +464,7 @@ static struct kunit_case test_cases[] = {
> /* clang-format off */
> KUNIT_CASE(test_get_hierarchy),
> KUNIT_CASE(test_get_denied_layer),
> - KUNIT_CASE(test_get_layer_from_deny_masks),
> + KUNIT_CASE(test_get_layer_from_fs_deny_masks),
> {}
> /* clang-format on */
> };
> diff --git a/security/landlock/audit.h b/security/landlock/audit.h
> index 92428b7fc4d80..104472060ef5e 100644
> --- a/security/landlock/audit.h
> +++ b/security/landlock/audit.h
> @@ -43,8 +43,7 @@ struct landlock_request {
> access_mask_t access;
>
> /* Required fields for requests with layer masks. */
> - const layer_mask_t (*layer_masks)[];
> - size_t layer_masks_size;
> + const struct layer_access_masks *masks;
>
> /* Required fields for requests with deny masks. */
> const access_mask_t all_existing_optional_access;
> diff --git a/security/landlock/domain.c b/security/landlock/domain.c
> index a647b68e8d060..e8e4ae5d075fe 100644
> --- a/security/landlock/domain.c
> +++ b/security/landlock/domain.c
> @@ -23,6 +23,7 @@
> #include "common.h"
> #include "domain.h"
> #include "id.h"
> +#include "limits.h"
>
> #ifdef CONFIG_AUDIT
>
> @@ -133,111 +134,47 @@ int landlock_init_hierarchy_log(struct landlock_hierarchy *const hierarchy)
> return 0;
> }
>
> -static deny_masks_t
> -get_layer_deny_mask(const access_mask_t all_existing_optional_access,
> - const unsigned long access_bit, const size_t layer)
> -{
> - unsigned long access_weight;
> -
> - /* This may require change with new object types. */
> - WARN_ON_ONCE(all_existing_optional_access !=
> - _LANDLOCK_ACCESS_FS_OPTIONAL);
> -
> - if (WARN_ON_ONCE(layer >= LANDLOCK_MAX_NUM_LAYERS))
> - return 0;
> -
> - access_weight = hweight_long(all_existing_optional_access &
> - GENMASK(access_bit, 0));
> - if (WARN_ON_ONCE(access_weight < 1))
> - return 0;
> -
> - return layer
> - << ((access_weight - 1) * HWEIGHT(LANDLOCK_MAX_NUM_LAYERS - 1));
> -}
> -
> -#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> -
> -static void test_get_layer_deny_mask(struct kunit *const test)
> -{
> - const unsigned long truncate = BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE);
> - const unsigned long ioctl_dev = BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV);
> -
> - KUNIT_EXPECT_EQ(test, 0,
> - get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - truncate, 0));
> - KUNIT_EXPECT_EQ(test, 0x3,
> - get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - truncate, 3));
> -
> - KUNIT_EXPECT_EQ(test, 0,
> - get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - ioctl_dev, 0));
> - KUNIT_EXPECT_EQ(test, 0xf0,
> - get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - ioctl_dev, 15));
> -}
> -
> -#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> -
> deny_masks_t
> -landlock_get_deny_masks(const access_mask_t all_existing_optional_access,
> - const access_mask_t optional_access,
> - const layer_mask_t (*const layer_masks)[],
> - const size_t layer_masks_size)
> +landlock_get_fs_deny_masks(const access_mask_t optional_access,
> + const struct layer_access_masks *layer_masks)
> {
> - const unsigned long access_opt = optional_access;
> - unsigned long access_bit;
> - deny_masks_t deny_masks = 0;
> + u8 truncate_layer = 0;
> + u8 ioctl_dev_layer = 0;
>
> - /* This may require change with new object types. */
> - WARN_ON_ONCE(access_opt !=
> - (optional_access & all_existing_optional_access));
> -
> - if (WARN_ON_ONCE(!layer_masks))
> - return 0;
> -
> - if (WARN_ON_ONCE(!access_opt))
> - return 0;
> -
> - for_each_set_bit(access_bit, &access_opt, layer_masks_size) {
> - const layer_mask_t mask = (*layer_masks)[access_bit];
> -
> - if (!mask)
> - continue;
> -
> - /* __fls(1) == 0 */
> - deny_masks |= get_layer_deny_mask(all_existing_optional_access,
> - access_bit, __fls(mask));
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
size_t i
ARRAY_SIZE(masks->access)
> + if (layer_masks->access[i] & optional_access &
> + LANDLOCK_ACCESS_FS_TRUNCATE)
> + truncate_layer = i;
> + if (layer_masks->access[i] & optional_access &
> + LANDLOCK_ACCESS_FS_IOCTL_DEV)
> + ioctl_dev_layer = i;
> }
> - return deny_masks;
> + return ((ioctl_dev_layer << 4) & 0xf0) | (truncate_layer & 0x0f);
> }
>
> #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
>
> -static void test_landlock_get_deny_masks(struct kunit *const test)
> +static void test_landlock_get_fs_deny_masks(struct kunit *const test)
> {
> - const layer_mask_t layers1[BITS_PER_TYPE(access_mask_t)] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0) |
> - BIT_ULL(9),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = BIT_ULL(1),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = BIT_ULL(2) |
> - BIT_ULL(0),
> + const struct layer_access_masks layers1 = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE |
> + LANDLOCK_ACCESS_FS_IOCTL_DEV,
> + .access[1] = LANDLOCK_ACCESS_FS_TRUNCATE,
> + .access[2] = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> + .access[9] = LANDLOCK_ACCESS_FS_EXECUTE,
> };
>
> KUNIT_EXPECT_EQ(test, 0x1,
> - landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - LANDLOCK_ACCESS_FS_TRUNCATE,
> - &layers1, ARRAY_SIZE(layers1)));
> + landlock_get_fs_deny_masks(LANDLOCK_ACCESS_FS_TRUNCATE,
> + &layers1));
> KUNIT_EXPECT_EQ(test, 0x20,
> - landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - LANDLOCK_ACCESS_FS_IOCTL_DEV,
> - &layers1, ARRAY_SIZE(layers1)));
> + landlock_get_fs_deny_masks(LANDLOCK_ACCESS_FS_IOCTL_DEV,
> + &layers1));
> KUNIT_EXPECT_EQ(
> test, 0x21,
> - landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,
> - LANDLOCK_ACCESS_FS_TRUNCATE |
> - LANDLOCK_ACCESS_FS_IOCTL_DEV,
> - &layers1, ARRAY_SIZE(layers1)));
> + landlock_get_fs_deny_masks(LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_IOCTL_DEV,
> + &layers1));
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> @@ -246,8 +183,7 @@ static void test_landlock_get_deny_masks(struct kunit *const test)
>
> static struct kunit_case test_cases[] = {
> /* clang-format off */
> - KUNIT_CASE(test_get_layer_deny_mask),
> - KUNIT_CASE(test_landlock_get_deny_masks),
> + KUNIT_CASE(test_landlock_get_fs_deny_masks),
> {}
> /* clang-format on */
> };
> diff --git a/security/landlock/domain.h b/security/landlock/domain.h
> index 7fb70b25f85a1..39600acb63897 100644
> --- a/security/landlock/domain.h
> +++ b/security/landlock/domain.h
> @@ -120,10 +120,8 @@ struct landlock_hierarchy {
> #ifdef CONFIG_AUDIT
>
> deny_masks_t
> -landlock_get_deny_masks(const access_mask_t all_existing_optional_access,
> - const access_mask_t optional_access,
> - const layer_mask_t (*const layer_masks)[],
> - size_t layer_masks_size);
> +landlock_get_fs_deny_masks(const access_mask_t optional_access,
> + const struct layer_access_masks *layer_masks);
>
> int landlock_init_hierarchy_log(struct landlock_hierarchy *const hierarchy);
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index b4ce03bef4b8e..1e765d22d8d49 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -407,57 +407,55 @@ static bool access_mask_subset(access_mask_t a, access_mask_t b)
> return (a | b) == b;
> }
>
> +/*
> + * Returns true iff the child file with the given src_child access rights under
> + * src_parent would result in having the same or fewer access rights if it were
> + * moved under new_parent.
> + */
> +static bool may_refer(const struct layer_access_masks *const src_parent,
> + const struct layer_access_masks *const src_child,
> + const struct layer_access_masks *const new_parent,
> + const bool child_is_dir)
> +{
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
size_t i
ARRAY_SIZE(src_child->access)
> + access_mask_t child_access = src_parent->access[i] &
> + src_child->access[i];
> + access_mask_t parent_access = new_parent->access[i];
> +
> + if (!child_is_dir) {
> + child_access &= ACCESS_FILE;
> + parent_access &= ACCESS_FILE;
> + }
> +
> + if (!access_mask_subset(child_access, parent_access))
> + return false;
> + }
> + return true;
> +}
> +
> /*
> * Check that a destination file hierarchy has more restrictions than a source
> * file hierarchy. This is only used for link and rename actions.
> *
> - * @layer_masks_child2: Optional child masks.
> + * Returns: true if child1 may be moved from parent1 to parent2 without
> + * increasing its access rights. If child2 is set, an additional condition is
> + * that child2 may be used from parent2 to parent1 without increasing its access
> + * rights.
> */
> -static bool no_more_access(
> - const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS],
> - const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS],
> - const bool child1_is_directory,
> - const layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS],
> - const layer_mask_t (*const layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS],
> - const bool child2_is_directory)
> +static bool no_more_access(const struct layer_access_masks *const parent1,
> + const struct layer_access_masks *const child1,
> + const bool child1_is_dir,
> + const struct layer_access_masks *const parent2,
> + const struct layer_access_masks *const child2,
> + const bool child2_is_dir)
> {
> - unsigned long access_bit;
> + if (!may_refer(parent1, child1, parent2, child1_is_dir))
> + return false;
>
> - for (access_bit = 0; access_bit < ARRAY_SIZE(*layer_masks_parent2);
> - access_bit++) {
> - /* Ignores accesses that only make sense for directories. */
> - const bool is_file_access =
> - !!(BIT_ULL(access_bit) & ACCESS_FILE);
> + if (!child2)
> + return true;
>
> - if (child1_is_directory || is_file_access) {
> - /*
> - * Checks if the destination restrictions are a
> - * superset of the source ones (i.e. inherited access
> - * rights without child exceptions):
> - * restrictions(parent2) >= restrictions(child1)
> - */
> - if ((((*layer_masks_parent1)[access_bit] &
> - (*layer_masks_child1)[access_bit]) |
> - (*layer_masks_parent2)[access_bit]) !=
> - (*layer_masks_parent2)[access_bit])
> - return false;
> - }
> -
> - if (!layer_masks_child2)
> - continue;
> - if (child2_is_directory || is_file_access) {
> - /*
> - * Checks inverted restrictions for RENAME_EXCHANGE:
> - * restrictions(parent1) >= restrictions(child2)
> - */
> - if ((((*layer_masks_parent2)[access_bit] &
> - (*layer_masks_child2)[access_bit]) |
> - (*layer_masks_parent1)[access_bit]) !=
> - (*layer_masks_parent1)[access_bit])
> - return false;
> - }
> - }
> - return true;
> + return may_refer(parent2, child2, parent1, child2_is_dir);
> }
>
> #define NMA_TRUE(...) KUNIT_EXPECT_TRUE(test, no_more_access(__VA_ARGS__))
> @@ -467,25 +465,25 @@ static bool no_more_access(
>
> static void test_no_more_access(struct kunit *const test)
> {
> - const layer_mask_t rx0[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = BIT_ULL(0),
> + const struct layer_access_masks rx0 = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE |
> + LANDLOCK_ACCESS_FS_READ_FILE,
> };
> - const layer_mask_t mx0[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = BIT_ULL(0),
> + const struct layer_access_masks mx0 = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE |
> + LANDLOCK_ACCESS_FS_MAKE_REG,
> };
> - const layer_mask_t x0[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> + const struct layer_access_masks x0 = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE,
> };
> - const layer_mask_t x1[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(1),
> + const struct layer_access_masks x1 = {
> + .access[1] = LANDLOCK_ACCESS_FS_EXECUTE,
> };
> - const layer_mask_t x01[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0) |
> - BIT_ULL(1),
> + const struct layer_access_masks x01 = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE,
> + .access[1] = LANDLOCK_ACCESS_FS_EXECUTE,
> };
> - const layer_mask_t allows_all[LANDLOCK_NUM_ACCESS_FS] = {};
> + const struct layer_access_masks allows_all = {};
>
> /* Checks without restriction. */
> NMA_TRUE(&x0, &allows_all, false, &allows_all, NULL, false);
> @@ -573,31 +571,30 @@ static void test_no_more_access(struct kunit *const test)
> #undef NMA_TRUE
> #undef NMA_FALSE
>
> -static bool is_layer_masks_allowed(
> - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +static bool is_layer_masks_allowed(const struct layer_access_masks *masks)
> {
> - return !memchr_inv(layer_masks, 0, sizeof(*layer_masks));
> + return !memchr_inv(&masks->access, 0, sizeof(masks->access));
> }
>
> /*
> - * Removes @layer_masks accesses that are not requested.
> + * Removes @masks accesses that are not requested.
> *
> * Returns true if the request is allowed, false otherwise.
> */
> -static bool
> -scope_to_request(const access_mask_t access_request,
> - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +static bool scope_to_request(const access_mask_t access_request,
> + struct layer_access_masks *masks)
> {
> - const unsigned long access_req = access_request;
> - unsigned long access_bit;
> + bool saw_unfulfilled_access = false;
>
> - if (WARN_ON_ONCE(!layer_masks))
> + if (WARN_ON_ONCE(!masks))
> return true;
>
> - for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks))
> - (*layer_masks)[access_bit] = 0;
> -
> - return is_layer_masks_allowed(layer_masks);
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
size_t i
ARRAY_SIZE(masks->access)
> + masks->access[i] &= access_request;
> + if (masks->access[i])
> + saw_unfulfilled_access = true;
> + }
> + return !saw_unfulfilled_access;
> }
>
> #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> @@ -605,48 +602,41 @@ scope_to_request(const access_mask_t access_request,
> static void test_scope_to_request_with_exec_none(struct kunit *const test)
> {
> /* Allows everything. */
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> + struct layer_access_masks masks = {};
>
> /* Checks and scopes with execute. */
> - KUNIT_EXPECT_TRUE(test, scope_to_request(LANDLOCK_ACCESS_FS_EXECUTE,
> - &layer_masks));
> - KUNIT_EXPECT_EQ(test, 0,
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> - KUNIT_EXPECT_EQ(test, 0,
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> + KUNIT_EXPECT_TRUE(test,
> + scope_to_request(LANDLOCK_ACCESS_FS_EXECUTE, &masks));
> + KUNIT_EXPECT_EQ(test, 0, masks.access[0]);
> }
>
> static void test_scope_to_request_with_exec_some(struct kunit *const test)
> {
> /* Denies execute and write. */
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(1),
> + struct layer_access_masks masks = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE,
> + .access[1] = LANDLOCK_ACCESS_FS_WRITE_FILE,
> };
>
> /* Checks and scopes with execute. */
> KUNIT_EXPECT_FALSE(test, scope_to_request(LANDLOCK_ACCESS_FS_EXECUTE,
> - &layer_masks));
> - KUNIT_EXPECT_EQ(test, BIT_ULL(0),
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> - KUNIT_EXPECT_EQ(test, 0,
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> + &masks));
> + KUNIT_EXPECT_EQ(test, LANDLOCK_ACCESS_FS_EXECUTE, masks.access[0]);
> + KUNIT_EXPECT_EQ(test, 0, masks.access[1]);
> }
>
> static void test_scope_to_request_without_access(struct kunit *const test)
> {
> /* Denies execute and write. */
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(1),
> + struct layer_access_masks masks = {
> + .access[0] = LANDLOCK_ACCESS_FS_EXECUTE,
> + .access[1] = LANDLOCK_ACCESS_FS_WRITE_FILE,
> };
>
> /* Checks and scopes without access request. */
> - KUNIT_EXPECT_TRUE(test, scope_to_request(0, &layer_masks));
> - KUNIT_EXPECT_EQ(test, 0,
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> - KUNIT_EXPECT_EQ(test, 0,
> - layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> + KUNIT_EXPECT_TRUE(test, scope_to_request(0, &masks));
> + KUNIT_EXPECT_EQ(test, 0, masks.access[0]);
> + KUNIT_EXPECT_EQ(test, 0, masks.access[1]);
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> @@ -655,20 +645,16 @@ static void test_scope_to_request_without_access(struct kunit *const test)
> * Returns true if there is at least one access right different than
> * LANDLOCK_ACCESS_FS_REFER.
> */
> -static bool
> -is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
> - const access_mask_t access_request)
> +static bool is_eacces(const struct layer_access_masks *masks,
> + const access_mask_t access_request)
> {
> - unsigned long access_bit;
> - /* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */
> - const unsigned long access_check = access_request &
> - ~LANDLOCK_ACCESS_FS_REFER;
> -
> - if (!layer_masks)
> + if (!masks)
> return false;
>
> - for_each_set_bit(access_bit, &access_check, ARRAY_SIZE(*layer_masks)) {
> - if ((*layer_masks)[access_bit])
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
size_t i
ARRAY_SIZE(masks->access)
> + /* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */
> + if (masks->access[i] & access_request &
> + ~LANDLOCK_ACCESS_FS_REFER)
> return true;
> }
> return false;
> @@ -681,37 +667,37 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
>
> static void test_is_eacces_with_none(struct kunit *const test)
> {
> - const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> + const struct layer_access_masks masks = {};
>
> - IE_FALSE(&layer_masks, 0);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> + IE_FALSE(&masks, 0);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_EXECUTE);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> }
>
> static void test_is_eacces_with_refer(struct kunit *const test)
> {
> - const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = BIT_ULL(0),
> + const struct layer_access_masks masks = {
> + .access[0] = LANDLOCK_ACCESS_FS_REFER,
> };
>
> - IE_FALSE(&layer_masks, 0);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> + IE_FALSE(&masks, 0);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_EXECUTE);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> }
>
> static void test_is_eacces_with_write(struct kunit *const test)
> {
> - const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> - [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(0),
> + const struct layer_access_masks masks = {
> + .access[0] = LANDLOCK_ACCESS_FS_WRITE_FILE,
> };
>
> - IE_FALSE(&layer_masks, 0);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> - IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> + IE_FALSE(&masks, 0);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&masks, LANDLOCK_ACCESS_FS_EXECUTE);
>
> - IE_TRUE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> + IE_TRUE(&masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> @@ -761,26 +747,25 @@ static void test_is_eacces_with_write(struct kunit *const test)
> * - true if the access request is granted;
> * - false otherwise.
> */
> -static bool is_access_to_paths_allowed(
> - const struct landlock_ruleset *const domain,
> - const struct path *const path,
> - const access_mask_t access_request_parent1,
> - layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS],
> - struct landlock_request *const log_request_parent1,
> - struct dentry *const dentry_child1,
> - const access_mask_t access_request_parent2,
> - layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS],
> - struct landlock_request *const log_request_parent2,
> - struct dentry *const dentry_child2)
> +static bool
> +is_access_to_paths_allowed(const struct landlock_ruleset *const domain,
> + const struct path *const path,
> + const access_mask_t access_request_parent1,
> + struct layer_access_masks *layer_masks_parent1,
> + struct landlock_request *const log_request_parent1,
> + struct dentry *const dentry_child1,
> + const access_mask_t access_request_parent2,
> + struct layer_access_masks *layer_masks_parent2,
> + struct landlock_request *const log_request_parent2,
> + struct dentry *const dentry_child2)
> {
> bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check,
> child1_is_directory = true, child2_is_directory = true;
> struct path walker_path;
> access_mask_t access_masked_parent1, access_masked_parent2;
> - layer_mask_t _layer_masks_child1[LANDLOCK_NUM_ACCESS_FS],
> - _layer_masks_child2[LANDLOCK_NUM_ACCESS_FS];
> - layer_mask_t(*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS] = NULL,
> - (*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
> + struct layer_access_masks _layer_masks_child1, _layer_masks_child2;
> + struct layer_access_masks *layer_masks_child1 = NULL,
> + *layer_masks_child2 = NULL;
>
> if (!access_request_parent1 && !access_request_parent2)
> return true;
> @@ -820,22 +805,20 @@ static bool is_access_to_paths_allowed(
> }
>
> if (unlikely(dentry_child1)) {
> - landlock_unmask_layers(
> - find_rule(domain, dentry_child1),
> - landlock_init_layer_masks(
> - domain, LANDLOCK_MASK_ACCESS_FS,
> - &_layer_masks_child1, LANDLOCK_KEY_INODE),
> - &_layer_masks_child1, ARRAY_SIZE(_layer_masks_child1));
> + if (landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
> + &_layer_masks_child1,
> + LANDLOCK_KEY_INODE))
> + landlock_unmask_layers(find_rule(domain, dentry_child1),
> + &_layer_masks_child1);
> layer_masks_child1 = &_layer_masks_child1;
> child1_is_directory = d_is_dir(dentry_child1);
> }
> if (unlikely(dentry_child2)) {
> - landlock_unmask_layers(
> - find_rule(domain, dentry_child2),
> - landlock_init_layer_masks(
> - domain, LANDLOCK_MASK_ACCESS_FS,
> - &_layer_masks_child2, LANDLOCK_KEY_INODE),
> - &_layer_masks_child2, ARRAY_SIZE(_layer_masks_child2));
> + if (landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
> + &_layer_masks_child2,
> + LANDLOCK_KEY_INODE))
> + landlock_unmask_layers(find_rule(domain, dentry_child2),
> + &_layer_masks_child2);
> layer_masks_child2 = &_layer_masks_child2;
> child2_is_directory = d_is_dir(dentry_child2);
> }
> @@ -890,16 +873,12 @@ static bool is_access_to_paths_allowed(
> }
>
> rule = find_rule(domain, walker_path.dentry);
> - allowed_parent1 = allowed_parent1 ||
> - landlock_unmask_layers(
> - rule, access_masked_parent1,
> - layer_masks_parent1,
> - ARRAY_SIZE(*layer_masks_parent1));
> - allowed_parent2 = allowed_parent2 ||
> - landlock_unmask_layers(
> - rule, access_masked_parent2,
> - layer_masks_parent2,
> - ARRAY_SIZE(*layer_masks_parent2));
> + allowed_parent1 =
> + allowed_parent1 ||
> + landlock_unmask_layers(rule, layer_masks_parent1);
> + allowed_parent2 =
> + allowed_parent2 ||
> + landlock_unmask_layers(rule, layer_masks_parent2);
>
> /* Stops when a rule from each layer grants access. */
> if (allowed_parent1 && allowed_parent2)
> @@ -953,9 +932,7 @@ static bool is_access_to_paths_allowed(
> log_request_parent1->audit.type = LSM_AUDIT_DATA_PATH;
> log_request_parent1->audit.u.path = *path;
> log_request_parent1->access = access_masked_parent1;
> - log_request_parent1->layer_masks = layer_masks_parent1;
> - log_request_parent1->layer_masks_size =
> - ARRAY_SIZE(*layer_masks_parent1);
> + log_request_parent1->masks = layer_masks_parent1;
> }
>
> if (!allowed_parent2) {
> @@ -963,9 +940,7 @@ static bool is_access_to_paths_allowed(
> log_request_parent2->audit.type = LSM_AUDIT_DATA_PATH;
> log_request_parent2->audit.u.path = *path;
> log_request_parent2->access = access_masked_parent2;
> - log_request_parent2->layer_masks = layer_masks_parent2;
> - log_request_parent2->layer_masks_size =
> - ARRAY_SIZE(*layer_masks_parent2);
> + log_request_parent2->masks = layer_masks_parent2;
> }
> return allowed_parent1 && allowed_parent2;
> }
> @@ -978,7 +953,7 @@ static int current_check_access_path(const struct path *const path,
> };
> const struct landlock_cred_security *const subject =
> landlock_get_applicable_subject(current_cred(), masks, NULL);
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> + struct layer_access_masks layer_masks;
> struct landlock_request request = {};
>
> if (!subject)
> @@ -1053,10 +1028,10 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)
> * - true if all the domain access rights are allowed for @dir;
> * - false if the walk reached @mnt_root.
> */
> -static bool collect_domain_accesses(
> - const struct landlock_ruleset *const domain,
> - const struct dentry *const mnt_root, struct dentry *dir,
> - layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS])
> +static bool collect_domain_accesses(const struct landlock_ruleset *const domain,
> + const struct dentry *const mnt_root,
> + struct dentry *dir,
> + struct layer_access_masks *layer_masks_dom)
> {
> unsigned long access_dom;
> bool ret = false;
> @@ -1075,9 +1050,8 @@ static bool collect_domain_accesses(
> struct dentry *parent_dentry;
>
> /* Gets all layers allowing all domain accesses. */
> - if (landlock_unmask_layers(find_rule(domain, dir), access_dom,
> - layer_masks_dom,
> - ARRAY_SIZE(*layer_masks_dom))) {
> + if (landlock_unmask_layers(find_rule(domain, dir),
> + layer_masks_dom)) {
> /*
> * Stops when all handled accesses are allowed by at
> * least one rule in each layer.
> @@ -1165,8 +1139,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
> access_mask_t access_request_parent1, access_request_parent2;
> struct path mnt_dir;
> struct dentry *old_parent;
> - layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS] = {},
> - layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS] = {};
> + struct layer_access_masks layer_masks_parent1 = {},
> + layer_masks_parent2 = {};
> struct landlock_request request1 = {}, request2 = {};
>
> if (!subject)
> @@ -1323,7 +1297,8 @@ static void hook_sb_delete(struct super_block *const sb)
> * second call to iput() for the same Landlock object. Also
> * checks I_NEW because such inode cannot be tied to an object.
> */
> - if (inode_state_read(inode) & (I_FREEING | I_WILL_FREE | I_NEW)) {
> + if (inode_state_read(inode) &
> + (I_FREEING | I_WILL_FREE | I_NEW)) {
> spin_unlock(&inode->i_lock);
> continue;
> }
> @@ -1641,7 +1616,7 @@ static bool is_device(const struct file *const file)
>
> static int hook_file_open(struct file *const file)
> {
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> + struct layer_access_masks layer_masks = {};
> access_mask_t open_access_request, full_access_request, allowed_access,
> optional_access;
> const struct landlock_cred_security *const subject =
> @@ -1676,20 +1651,14 @@ static int hook_file_open(struct file *const file)
> &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) {
> allowed_access = full_access_request;
> } else {
> - unsigned long access_bit;
> - const unsigned long access_req = full_access_request;
> -
> /*
> * Calculate the actual allowed access rights from layer_masks.
> - * Add each access right to allowed_access which has not been
> - * vetoed by any layer.
> + * Remove the access rights from the full access request which
> + * are still unfulfilled in any of the layers.
> */
> - allowed_access = 0;
> - for_each_set_bit(access_bit, &access_req,
> - ARRAY_SIZE(layer_masks)) {
> - if (!layer_masks[access_bit])
> - allowed_access |= BIT_ULL(access_bit);
> - }
> + allowed_access = full_access_request;
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++)
size_t i
ARRAY_SIZE(layer_masks.access)
> + allowed_access &= ~layer_masks.access[i];
> }
>
> /*
> @@ -1700,9 +1669,8 @@ static int hook_file_open(struct file *const file)
> */
> landlock_file(file)->allowed_access = allowed_access;
> #ifdef CONFIG_AUDIT
> - landlock_file(file)->deny_masks = landlock_get_deny_masks(
> - _LANDLOCK_ACCESS_FS_OPTIONAL, optional_access, &layer_masks,
> - ARRAY_SIZE(layer_masks));
> + landlock_file(file)->deny_masks =
> + landlock_get_fs_deny_masks(optional_access, &layer_masks);
> #endif /* CONFIG_AUDIT */
>
> if (access_mask_subset(open_access_request, allowed_access))
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 1f3915a90a808..2a5456f4f017e 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -47,7 +47,7 @@ static int current_check_access_socket(struct socket *const sock,
> access_mask_t access_request)
> {
> __be16 port;
> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
> + struct layer_access_masks layer_masks = {};
> const struct landlock_rule *rule;
> struct landlock_id id = {
> .type = LANDLOCK_KEY_NET_PORT,
> @@ -178,8 +178,9 @@ static int current_check_access_socket(struct socket *const sock,
> access_request = landlock_init_layer_masks(subject->domain,
> access_request, &layer_masks,
> LANDLOCK_KEY_NET_PORT);
> - if (landlock_unmask_layers(rule, access_request, &layer_masks,
> - ARRAY_SIZE(layer_masks)))
> + if (!access_request)
> + return 0;
Add a new line.
> + if (landlock_unmask_layers(rule, &layer_masks))
> return 0;
>
> audit_net.family = address->sa_family;
> @@ -189,8 +190,7 @@ static int current_check_access_socket(struct socket *const sock,
> .audit.type = LSM_AUDIT_DATA_NET,
> .audit.u.net = &audit_net,
> .access = access_request,
> - .layer_masks = &layer_masks,
> - .layer_masks_size = ARRAY_SIZE(layer_masks),
> + .masks = &layer_masks,
> });
> return -EACCES;
> }
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index dfcdc19ea2683..d20e28d38e9c9 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,
> * request are empty).
> */
The above doc should be updated with the new arguments. You can also
convert it to a proper function docstring (which would have detected
this issue).
> bool landlock_unmask_layers(const struct landlock_rule *const rule,
> - const access_mask_t access_request,
> - layer_mask_t (*const layer_masks)[],
> - const size_t masks_array_size)
> + struct layer_access_masks *masks)
> {
> - size_t layer_level;
> -
> - if (!access_request || !layer_masks)
> + if (!masks)
> return true;
> if (!rule)
> return false;
>
> - /*
> - * An access is granted if, for each policy layer, at least one rule
> - * encountered on the pathwalk grants the requested access,
> - * regardless of its position in the layer stack. We must then check
> - * the remaining layers for each inode, from the first added layer to
> - * the last one. When there is multiple requested accesses, for each
> - * policy layer, the full set of requested accesses may not be granted
> - * by only one rule, but by the union (binary OR) of multiple rules.
> - * E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
> - */
I'd like to keep most of this comment, even if some parts are specific
to FS access, I think it helps understand the logic.
> - for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
> - const struct landlock_layer *const layer =
> - &rule->layers[layer_level];
> - const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
> - const unsigned long access_req = access_request;
> - unsigned long access_bit;
> - bool is_empty;
> + for (int i = 0; i < rule->num_layers; i++) {
size_t i
> + const struct landlock_layer *l = &rule->layers[i];
... *const layer = ...
>
> - /*
> - * Records in @layer_masks which layer grants access to each requested
> - * access: bit cleared if the related layer grants access.
> - */
At least the "bit cleared if the related layer grants access" comment
should be kept.
> - is_empty = true;
> - for_each_set_bit(access_bit, &access_req, masks_array_size) {
> - if (layer->access & BIT_ULL(access_bit))
> - (*layer_masks)[access_bit] &= ~layer_bit;
> - is_empty = is_empty && !(*layer_masks)[access_bit];
> - }
> - if (is_empty)
> - return true;
> + masks->access[l->level - 1] &= ~l->access;
It's indeed better to get rid of the requested access in this helper.
> }
> - return false;
> +
> + for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
size_t i
ARRAY_SIZE(masks->access)
> + if (masks->access[i])
> + return false;
> + }
> + return true;
> }
>
> typedef access_mask_t
> @@ -679,8 +654,7 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
> *
> * @domain: The domain that defines the current restrictions.
> * @access_request: The requested access rights to check.
> - * @layer_masks: It must contain %LANDLOCK_NUM_ACCESS_FS or
There is another mention @layer_masks above.
> - * %LANDLOCK_NUM_ACCESS_NET elements according to @key_type.
> + * @masks: Layer access masks to populate.
> * @key_type: The key type to switch between access masks of different types.
> *
> * Returns: An access mask where each access right bit is set which is handled
> @@ -689,23 +663,20 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
> access_mask_t
> landlock_init_layer_masks(const struct landlock_ruleset *const domain,
> const access_mask_t access_request,
> - layer_mask_t (*const layer_masks)[],
> + struct layer_access_masks *masks,
*const masks
> const enum landlock_key_type key_type)
> {
> access_mask_t handled_accesses = 0;
> - size_t layer_level, num_access;
> get_access_mask_t *get_access_mask;
>
> switch (key_type) {
> case LANDLOCK_KEY_INODE:
> get_access_mask = landlock_get_fs_access_mask;
> - num_access = LANDLOCK_NUM_ACCESS_FS;
> break;
>
> #if IS_ENABLED(CONFIG_INET)
> case LANDLOCK_KEY_NET_PORT:
> get_access_mask = landlock_get_net_access_mask;
> - num_access = LANDLOCK_NUM_ACCESS_NET;
> break;
> #endif /* IS_ENABLED(CONFIG_INET) */
>
> @@ -714,27 +685,18 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
> return 0;
> }
>
> - memset(layer_masks, 0,
> - array_size(sizeof((*layer_masks)[0]), num_access));
> -
> /* An empty access request can happen because of O_WRONLY | O_RDWR. */
> if (!access_request)
> return 0;
>
> - /* Saves all handled accesses per layer. */
> - for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
> - const unsigned long access_req = access_request;
> - const access_mask_t access_mask =
> - get_access_mask(domain, layer_level);
> - unsigned long access_bit;
> + for (int i = 0; i < domain->num_layers; i++) {
size_t i
> + const access_mask_t handled = get_access_mask(domain, i);
>
> - for_each_set_bit(access_bit, &access_req, num_access) {
> - if (BIT_ULL(access_bit) & access_mask) {
> - (*layer_masks)[access_bit] |=
> - BIT_ULL(layer_level);
> - handled_accesses |= BIT_ULL(access_bit);
> - }
> - }
> + masks->access[i] = access_request & handled;
> + handled_accesses |= masks->access[i];
> }
> + for (int i = domain->num_layers; i < LANDLOCK_MAX_NUM_LAYERS; i++)
size_t i
ARRAY_SIZE(masks->access)
> + masks->access[i] = 0;
> +
> return handled_accesses;
> }
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 1a78cba662b24..f7b80b18c2a70 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -301,15 +301,25 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
> return ruleset->access_masks[layer_level].scope;
> }
>
> +/**
> + * struct layer_accesses - A boolean matrix of layers and access rights
> + *
> + * This has a bit for each combination of layer numbers and access rights.
> + * During access checks, it is used to represent the access rights for each
> + * layer which still need to be fulfilled. When all bits are 0, the access
> + * request is considered to be fulfilled.
> + */
> +struct layer_access_masks {
> + access_mask_t access[LANDLOCK_MAX_NUM_LAYERS];
> +};
> +
> bool landlock_unmask_layers(const struct landlock_rule *const rule,
> - const access_mask_t access_request,
> - layer_mask_t (*const layer_masks)[],
> - const size_t masks_array_size);
> + struct layer_access_masks *masks);
>
> access_mask_t
> landlock_init_layer_masks(const struct landlock_ruleset *const domain,
> const access_mask_t access_request,
> - layer_mask_t (*const layer_masks)[],
> + struct layer_access_masks *masks,
> const enum landlock_key_type key_type);
>
> #endif /* _SECURITY_LANDLOCK_RULESET_H */
> --
> 2.52.0
>
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
From: Mickaël Salaün @ 2026-01-21 22:16 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze
In-Reply-To: <20260111.11c57c607174@gnoack.org>
On Sun, Jan 11, 2026 at 10:52:26PM +0100, Günther Noack wrote:
> On Tue, Dec 30, 2025 at 11:39:21AM +0100, Günther Noack wrote:
> > diff --git a/security/landlock/access.h b/security/landlock/access.h
> > index 7961c6630a2d7..aa0efa36a37db 100644
> > --- a/security/landlock/access.h
> > +++ b/security/landlock/access.h
> > [...]
> > /*
> > * Tracks domains responsible of a denied access. This is required to avoid
> > * storing in each object the full layer_masks[] required by update_request().
> > + *
> > + * Each nibble represents the layer index of the newest layer which denied a
> > + * certain access right. For file system access rights, the upper four bits are
> > + * the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the
> > + * lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE.
> > */
> > typedef u8 deny_masks_t;
>
> FYI: I left this out for now because it felt a bit out of scope (and
> transposing the layer masks was adventurous enough), but I was tempted
> to go one step further here and turn this into a struct with
> bitfields:
>
> /* A collection of layer indices denying specific access rights. */
> struct layers_denying_fs_access {
> unsigned int truncate : 4;
> unsigned int ioctl_dev : 4;
> }
>
> (Type name TBD, I am open for suggestions.)
>
> I think if we accept that this data structure is specific to FS access
> rights, we win clarity in the code. When I came across the code that
> put this together dynamically and in a more generic way, it took me a
> while to figure out what it did.
This change should help indeed (in a standalone patch). Maybe we could
make the rest of the logic more generic in a way that it would be
simpler to add this kind of access right?
^ permalink raw reply
* [LSF/MM/BPF TOPIC] Refactor LSM hooks for VFS mount operations
From: Song Liu @ 2026-01-21 19:54 UTC (permalink / raw)
To: bpf, Linux-Fsdevel, lsf-pc, linux-security-module
Cc: Christian Brauner, Al Viro
Current LSM hooks do not have good coverage for VFS mount operations.
Specifically, there are the following issues (and maybe more..):
1. security_sb_mount suffers from the TOCTOU bug for bind mount and
move mount [1];
2. There is not sufficient coverage for new mount syscalls (open_tree, fspick,
etc.) [2].
A key consideration of this refactor is to minimize lock contention, especially
around namespace_sem.
I also want to discuss what features in the kernel side (kfuncs,
iterators, etc.)
are needed to enable reliable monitoring of mount operations in BPF LSM.
Thanks,
Song
PS: I am not sure whether other folks are already working on it. I will prepare
some RFC patches before the conference if I don't see other proposals.
[1] https://lore.kernel.org/bpf/20251130064609.GR3538@ZenIV/
[2] https://lore.kernel.org/linux-security-module/20250711-pfirsich-worum-c408f9a14b13@brauner/
^ permalink raw reply
* Re: [PATCH 1/3] integrity: Make arch_ima_get_secureboot integrity-wide
From: Ard Biesheuvel @ 2026-01-21 16:25 UTC (permalink / raw)
To: Mimi Zohar
Cc: Coiby Xu, Dave Hansen, linux-integrity, Heiko Carstens,
Roberto Sassu, Catalin Marinas, Will Deacon, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy (CS GROUP),
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H. Peter Anvin, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn, Jarkko Sakkinen,
moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
open list:S390 ARCHITECTURE,
open list:EXTENSIBLE FIRMWARE INTERFACE (EFI),
open list:SECURITY SUBSYSTEM, open list:KEYS/KEYRINGS_INTEGRITY
In-Reply-To: <1a0b6e5601a673a81f8823de0815f92b7afbeb60.camel@linux.ibm.com>
On Wed, 21 Jan 2026 at 16:41, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2026-01-19 at 12:04 +0800, Coiby Xu wrote:
>
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 976e75f9b9ba..5dce572192d6 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -311,6 +311,7 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> > config IMA_SECURE_AND_OR_TRUSTED_BOOT
> > bool
> > depends on IMA_ARCH_POLICY
> > + depends on INTEGRITY_SECURE_BOOT
> >
> >
> > Another idea is make a tree-wide arch_get_secureboot i.e. to move
> > current arch_ima_get_secureboot code to arch-specific secure boot
> > implementation. By this way, there will no need for a new Kconfig option
> > INTEGRITY_SECURE_BOOT. But I'm not sure if there is any unforeseen
> > concern.
>
> Originally basing IMA policy on the secure boot mode was an exception. As long
> as making it public isn't an issue any longer, this sounds to me. Ard, Dave, do
> you have any issues with replacing arch_ima_get_secureboot() with
> arch_get_secureboot()?
I don't see an issue with that. If there is a legitimate need to
determine this even if IMA is not enabled, then this makes sense.
^ permalink raw reply
* Re: [PATCH 1/3] integrity: Make arch_ima_get_secureboot integrity-wide
From: Mimi Zohar @ 2026-01-21 15:40 UTC (permalink / raw)
To: Coiby Xu, Ard Biesheuvel, Dave Hansen
Cc: linux-integrity, Heiko Carstens, Roberto Sassu, Catalin Marinas,
Will Deacon, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E. Hallyn, Jarkko Sakkinen,
moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
open list:S390 ARCHITECTURE,
open list:EXTENSIBLE FIRMWARE INTERFACE (EFI),
open list:SECURITY SUBSYSTEM, open list:KEYS/KEYRINGS_INTEGRITY
In-Reply-To: <aW2i3yacr5TvWU-m@Rk>
On Mon, 2026-01-19 at 12:04 +0800, Coiby Xu wrote:
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 976e75f9b9ba..5dce572192d6 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -311,6 +311,7 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> config IMA_SECURE_AND_OR_TRUSTED_BOOT
> bool
> depends on IMA_ARCH_POLICY
> + depends on INTEGRITY_SECURE_BOOT
>
>
> Another idea is make a tree-wide arch_get_secureboot i.e. to move
> current arch_ima_get_secureboot code to arch-specific secure boot
> implementation. By this way, there will no need for a new Kconfig option
> INTEGRITY_SECURE_BOOT. But I'm not sure if there is any unforeseen
> concern.
Originally basing IMA policy on the secure boot mode was an exception. As long
as making it public isn't an issue any longer, this sounds to me. Ard, Dave, do
you have any issues with replacing arch_ima_get_secureboot() with
arch_get_secureboot()?
^ permalink raw reply
* Re: [PATCH 1/3] integrity: Make arch_ima_get_secureboot integrity-wide
From: Mimi Zohar @ 2026-01-21 15:29 UTC (permalink / raw)
To: Dave Hansen, Ard Biesheuvel
Cc: Coiby Xu, linux-integrity, Heiko Carstens, Roberto Sassu,
Catalin Marinas, Will Deacon, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy (CS GROUP),
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H. Peter Anvin, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn, Jarkko Sakkinen,
moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
open list:S390 ARCHITECTURE,
open list:EXTENSIBLE FIRMWARE INTERFACE (EFI),
open list:SECURITY SUBSYSTEM, open list:KEYS/KEYRINGS_INTEGRITY
In-Reply-To: <79185163-bf8f-4490-9396-3fd73b7a0c73@intel.com>
Hi Dave!
On Mon, 2026-01-19 at 10:44 -0800, Dave Hansen wrote:
> On 1/18/26 10:25, Mimi Zohar wrote:
> > As not all arch's implement arch_integrity_get_secureboot, the definition in
> > include/linux/integrity.h would need to be updated. Something like:
> >
> > -#ifdef CONFIG_INTEGRITY_SECURE_BOOT
> > +#if (defined(CONFIG_INTEGRITY_SECURE_BOOT) && \
> > + (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> > + || defined(CONFIG_PPC_SECURE_BOOT))
> >
> > Then IMA_SECURE_AND_OR_TRUSTED_BOOT and EVM could select INTEGRITY_SECURE_BOOT,
> > as suggested.
>
> This seems to be going a wee bit sideways. :)
Agreed, that was my point. :) "imply" was cleaner, but Ard objected to two
imply's.
>
> This kind of CONFIG complexity really should be left to Kconfig. C
> macros really aren't a great place to do it.
>
> The other idiom we use a lot is this in generic code:
>
> #ifndef arch_foo
> static inline void arch_foo(void) {}
> #endif
>
> Then all you have to do is make sure the arch header that #defines it is
> included before the generic code. I'm not a super huge fan of these
> because it can be hard to tell (for humans at least) _if_ the
> architecture has done the #define.
>
> But it sure beats that #ifdef maze.
Sure.
^ permalink raw reply
* あたま専門のもみほぐし店 収益性等／概要資料
From: ヘッドミント @ 2026-01-21 9:38 UTC (permalink / raw)
To: linux-security-module
お世話になります。
コンパクトにスタートできて手堅く収益をあげることのできる
フランチャイズビジネスの事業概要資料をご案内申し上げます。
小資本／小スペース／少人数の
コンパクト・フランチャイズ
ドライヘッドスパ専門店
“ヘッドミント”
・収益モデル
・開業に必要な資金
・ロイヤリティ
・スケジュール etc
↓ FC事業概要資料 ↓
https://dryheadspa-hm.biz/fc/
ドライヘッドスパとは ――― 水を使わないヘッドスパです。
足つぼや耳つぼなど、専門のもみほぐし店がありますが
ご紹介するサロンは「 頭に特化したもみほぐし店 」です。
ドライヘッドスパというジャンルの認知度は、
現時点ではそれほど高くありません。
にも関わらず、私どもがフランチャイズ展開する“ヘッドミント”の
店舗には月間450人以上の新規客が来店し、満席が続いています。
これから先、認知度が高まることで
爆発的に伸びるポテンシャルを秘めています。
フランチャイズによる事業を展開していますので、
新たな収益づくりをお考えの方は、まずは概要資料をご覧ください。
ドライヘッドスパ専門店
ヘッドミント
↓ FC事業概要資料 ↓
https://dryheadspa-hm.biz/fc/
よろしくお願いします。
------------------------------------------
株式会社じむや
愛知県名古屋市中区大須3-26-41堀田ビル
TEL：052-263-4688
------------------------------------------
本情報がご不要な方にはご迷惑をおかけし申し訳ございません。
メールマガジンの解除は、下記URLにて承っております。
https://dryheadspa-hm.biz/mail/
お手数お掛けしますがよろしくお願いします。
^ permalink raw reply
* Re: [PATCH v2 2/3] landlock: Add comprehensive errata documentation
From: Günther Noack @ 2026-01-21 10:19 UTC (permalink / raw)
To: Samasth Norway Ananda; +Cc: mic, linux-security-module, linux-kernel
In-Reply-To: <20260103002722.1465371-3-samasth.norway.ananda@oracle.com>
Hello!
Overall, this looks very good to me, thanks for documenting that!
Some smaller remarks, mostly on structure.
On Fri, Jan 02, 2026 at 04:27:14PM -0800, Samasth Norway Ananda wrote:
> Add comprehensive documentation for the Landlock errata mechanism,
> including how to query errata using LANDLOCK_CREATE_RULESET_ERRATA
> and links to enhanced detailed descriptions in the kernel source.
>
> Also enhance existing DOC sections in security/landlock/errata/abi-*.h
> files with Impact sections, and update the code comment in syscalls.c
> to remind developers to update errata documentation when applicable.
>
> This addresses the gap where the kernel implements errata tracking
> but provides no user-facing documentation on how to use it, while
> improving the existing technical documentation in-place rather than
> duplicating it.
>
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
> ---
> Documentation/userspace-api/landlock.rst | 60 +++++++++++++++++++++++-
> security/landlock/errata/abi-1.h | 8 ++++
> security/landlock/errata/abi-4.h | 7 +++
> security/landlock/errata/abi-6.h | 10 ++++
> security/landlock/syscalls.c | 4 +-
> 5 files changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 650c7b368561..930723fd7c1a 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
> =====================================
>
> :Author: Mickaël Salaün
> -:Date: March 2025
> +:Date: January 2026
>
> The goal of Landlock is to enable restriction of ambient rights (e.g. global
> filesystem or network access) for a set of processes. Because Landlock
> @@ -458,6 +458,64 @@ system call:
> printf("Landlock supports LANDLOCK_ACCESS_FS_REFER.\n");
> }
>
> +Landlock Errata
> +---------------
> +
> +In addition to ABI versions, Landlock provides an errata mechanism to track
> +fixes for issues that may affect backwards compatibility or require userspace
> +awareness. The errata bitmask can be queried using:
> +
> +.. code-block:: c
> +
> + int errata;
> +
> + errata = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_ERRATA);
> + if (errata < 0) {
> + /* Landlock not available or disabled */
> + return 0;
> + }
> +
> +The returned value is a bitmask where each bit represents a specific erratum.
> +If bit N is set (``errata & (1 << (N - 1))``), then erratum N has been fixed
> +in the running kernel.
> +
> +.. warning::
> +
> + **Most applications should NOT check errata.** In 99.9% of cases, checking
> + errata is unnecessary, increases code complexity, and can potentially
> + decrease protection if misused. For example, disabling the sandbox when an
> + erratum is not fixed could leave the system less secure than using
> + Landlock's best-effort protection. When in doubt, ignore errata.
> +
> +For detailed technical descriptions of each erratum, including their impact
> +and when they affect applications, see the DOC sections in the kernel source:
> +
> +- **Erratum 1: TCP socket identification (ABI 4)** - See ``erratum_1`` in ``security/landlock/errata/abi-4.h``
> +- **Erratum 2: Scoped signal handling (ABI 6)** - See ``erratum_2`` in ``security/landlock/errata/abi-6.h``
> +- **Erratum 3: Disconnected directory handling (ABI 1)** - See ``erratum_3`` in ``security/landlock/errata/abi-1.h``
Is it not possible to include the errata descriptions here through the header?
For instance, further below in this document, we also include the
system call documentation from the UAPI header, using:
.. kernel-doc:: include/uapi/linux/landlock.h
:identifiers: fs_access net_access scope
> +
> +How to Check for Errata
> +~~~~~~~~~~~~~~~~~~~~~~~
> +
> +If you determine that your application needs to check for specific errata,
> +use this pattern:
> +
> +.. code-block:: c
> +
> + int errata = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_ERRATA);
> + if (errata >= 0) {
> + /* Check for specific erratum (1-indexed) */
> + if (errata & (1 << (erratum_number - 1))) {
> + /* Erratum N is fixed in this kernel */
> + } else {
> + /* Erratum N is NOT fixed - consider implications for your use case */
> + }
> + }
> +
> +**Important:** Only check errata if your application specifically relies on
> +behavior that changed due to the fix. The fixes generally make Landlock less
> +restrictive or more correct, not more restrictive.
> +
> The following kernel interfaces are implicitly supported by the first ABI
> version. Features only supported from a specific version are explicitly marked
> as such.
At the end of your added text, there is a similar issue as in the
other commit, where a section that previously belonged elsewhere is
now part of your new section by accident.
I think the paragraph "The following kernel interfaces are implicitly
supported..." is meant to belong to the "Landlock ABI versions"
section which is above the text that you added. I would recommend to
rephrase it slightly, because it also talks about the "following
kernel interfaces", which are not immediately following any more, e.g.
"All Landlock kernel interfaces are supported by the first ABI
version unless it is explicitly noted in their documentation."
Please feel free to rephrase if a different phrasing seems more
suitable.
> diff --git a/security/landlock/errata/abi-1.h b/security/landlock/errata/abi-1.h
> index e8a2bff2e5b6..ba9895bf8ce1 100644
> --- a/security/landlock/errata/abi-1.h
> +++ b/security/landlock/errata/abi-1.h
> @@ -12,5 +12,13 @@
> * hierarchy down to its filesystem root and those from the related mount point
> * hierarchy. This prevents access right widening through rename or link
> * actions.
> + *
> + * Impact:
> + *
> + * Without this fix, it was possible to widen access rights through rename or
> + * link actions involving disconnected directories, potentially bypassing
> + * ``LANDLOCK_ACCESS_FS_REFER`` restrictions. This could allow privilege
> + * escalation in complex mount scenarios where directories become disconnected
> + * from their original mount points.
> */
> LANDLOCK_ERRATUM(3)
> diff --git a/security/landlock/errata/abi-4.h b/security/landlock/errata/abi-4.h
> index c052ee54f89f..59574759dc1e 100644
> --- a/security/landlock/errata/abi-4.h
> +++ b/security/landlock/errata/abi-4.h
> @@ -11,5 +11,12 @@
> * :manpage:`bind(2)` and :manpage:`connect(2)` operations. This change ensures
> * that only TCP sockets are subject to TCP access rights, allowing other
> * protocols to operate without unnecessary restrictions.
> + *
> + * Impact:
> + *
> + * In kernels without this fix, using ``LANDLOCK_ACCESS_NET_BIND_TCP`` or
> + * ``LANDLOCK_ACCESS_NET_CONNECT_TCP`` would incorrectly restrict non-TCP
> + * stream protocols (SMC, MPTCP, SCTP), potentially breaking applications
> + * that rely on these protocols while using Landlock network restrictions.
> */
> LANDLOCK_ERRATUM(1)
> diff --git a/security/landlock/errata/abi-6.h b/security/landlock/errata/abi-6.h
> index df7bc0e1fdf4..a3a48b2bf2db 100644
> --- a/security/landlock/errata/abi-6.h
> +++ b/security/landlock/errata/abi-6.h
> @@ -15,5 +15,15 @@
> * interaction between threads of the same process should always be allowed.
> * This change ensures that any thread is allowed to send signals to any other
> * thread within the same process, regardless of their domain.
> + *
> + * Impact:
> + *
> + * This problem only manifests when the userspace process is itself using
> + * :manpage:`libpsx(3)` or an equivalent mechanism to enforce a Landlock policy
> + * on multiple already-running threads at once. Programs which enforce a
> + * Landlock policy at startup time and only then become multithreaded are not
> + * affected. Without this fix, signal scoping could break multi-threaded
> + * applications that expect threads within the same process to freely signal
> + * each other.
> */
> LANDLOCK_ERRATUM(2)
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 0116e9f93ffe..cf5ba7715916 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -157,9 +157,11 @@ static const struct file_operations ruleset_fops = {
> /*
> * The Landlock ABI version should be incremented for each new Landlock-related
> * user space visible change (e.g. Landlock syscalls). This version should
> - * only be incremented once per Linux release, and the date in
> + * only be incremented once per Linux release. When incrementing, the date in
> * Documentation/userspace-api/landlock.rst should be updated to reflect the
> * UAPI change.
> + * If the change involves a fix that requires userspace awareness, also update
> + * the errata documentation in Documentation/userspace-api/landlock.rst.
> */
> const int landlock_abi_version = 7;
>
> --
> 2.50.1
>
The texts all look very good, thank you very much for documenting this!
—Günther
^ permalink raw reply
* Re: [PATCH v2 1/3] landlock: Add missing ABI 7 case in documentation example
From: Günther Noack @ 2026-01-21 9:37 UTC (permalink / raw)
To: Samasth Norway Ananda; +Cc: mic, linux-security-module, linux-kernel
In-Reply-To: <20260103002722.1465371-2-samasth.norway.ananda@oracle.com>
Thanks for the updated documentation!
The comments below are mostly about the structure in the bigger
document context. The wording seems good.
On Fri, Jan 02, 2026 at 04:27:13PM -0800, Samasth Norway Ananda wrote:
> Add the missing case 6 and case 7 handling in the ABI version
> compatibility example to properly handle ABI < 7 kernels.
>
> Add an optional backwards compatibility section for restrict flags
> between the case analysis and landlock_restrict_self() call. The main
> tutorial example remains unchanged with
> landlock_restrict_self(ruleset_fd, 0) to keep it simple for users who
> don't need logging flags.
>
> Also fix misleading description of the /usr rule which incorrectly
> stated it "only allow[s] reading" when the code actually allows both
> reading and executing (LANDLOCK_ACCESS_FS_EXECUTE is included in
> allowed_access).
>
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
> ---
> Documentation/userspace-api/landlock.rst | 35 +++++++++++++++++++++---
> 1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 1d0c2c15c22e..650c7b368561 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -127,6 +127,12 @@ version, and only use the available subset of access rights:
> /* Removes LANDLOCK_SCOPE_* for ABI < 6 */
> ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET |
> LANDLOCK_SCOPE_SIGNAL);
> + __attribute__((fallthrough));
> + case 6:
> + /* Removes logging flags for ABI < 7 */
> + __attribute__((fallthrough));
> + case 7:
> + break;
> }
This code does nothing, and the comment about removing logging flags
is unclear in that context without pointing to the later section.
IMHO we can either say that logging flags are removed in the code
further below, or just leave it out.
The "case 7" case is not necessary.
>
> This enables the creation of an inclusive ruleset that will contain our rules.
> @@ -142,8 +148,9 @@ This enables the creation of an inclusive ruleset that will contain our rules.
> }
>
> We can now add a new rule to this ruleset thanks to the returned file
> -descriptor referring to this ruleset. The rule will only allow reading the
> -file hierarchy ``/usr``. Without another rule, write actions would then be
> +descriptor referring to this ruleset. The rule will allow reading and
> +executing files in the ``/usr`` hierarchy. Without another rule, write actions
> +and other operations (make_dir, remove_file, etc.) would then be
> denied by the ruleset. To add ``/usr`` to the ruleset, we open it with the
> ``O_PATH`` flag and fill the &struct landlock_path_beneath_attr with this file
> descriptor.
> @@ -191,10 +198,30 @@ number for a specific action: HTTPS connections.
> err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> &net_port, 0);
>
> +Backwards compatibility for restrict flags
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If you were to add this header, then the section about prctl() and
landlock_restrict_self() would also now appear under "Backwards
compatibility for restrict flags", and that would be confusing. I
would recommend to drop the header for now and clarify in the text
what it is about. (IMHO, it is visually reasonably easy to understand
the structure of the different sections, as they always follow the
structure "text + code snippet".)
If we want to introduce a new level of headers below "Defining and
enforcing a security policy", we would have to introduce subsections
for the other steps as well, so that it is more structured. (If we
wanted to do that though, it would probably be better to do it in a
separate commit.)
> +When passing a non-zero ``flags`` argument to ``landlock_restrict_self()``, the
> +following backwards compatibility check needs to be taken into account:
> +
> +.. code-block:: c
> +
> + /*
> + * Desired restriction flags, see ABI version section above.
^^^^^^^^^^^^^^^^^^^^^^^^^
The ABI section does not talk about the restriction flags much;
I would have expected that to link to the place where the restriction
flags are documented, which would be the system call documentation
from the header?
https://docs.kernel.org/userspace-api/landlock.html#enforcing-a-ruleset
> + * This value is only an example and differs by use case.
> + */
> + int restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
> + if (abi < 7) {
> + /* Clear logging flags unsupported in ABI < 7 */
> + restrict_flags &= ~(LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF |
> + LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON |
> + LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF);
> + }
> +
> The next step is to restrict the current thread from gaining more privileges
> (e.g. through a SUID binary). We now have a ruleset with the first rule
> -allowing read access to ``/usr`` while denying all other handled accesses for
> -the filesystem, and a second rule allowing HTTPS connections.
> +allowing read and execute access to ``/usr`` while denying all other handled
> +accesses for the filesystem, and a second rule allowing HTTPS connections.
>
If you are setting a "restrict_flags" variable here, then you would
also need to use that variable in the call to landlock_restrict_self()
in the documentation below, so that the different code sections in the
documentation work together.
—Günther
^ 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