* Re: [PATCH] apparmor: Remove redundant if check in sk_peer_get_label
From: John Johansen @ 2026-03-18 5:59 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260204220734.1008069-2-thorsten.blum@linux.dev>
On 2/4/26 14:07, Thorsten Blum wrote:
> Remove the redundant if check in sk_peer_get_label() and return
> ERR_PTR(-ENOPROTOOPT) directly.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Acked-by: John Johansen <john.johansen@canonical.com>
I have pulled this into my tree
> ---
> security/apparmor/lsm.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index a87cd60ed206..54343f7c96a4 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1536,15 +1536,11 @@ static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> static struct aa_label *sk_peer_get_label(struct sock *sk)
> {
> struct aa_sk_ctx *ctx = aa_sock(sk);
> - struct aa_label *label = ERR_PTR(-ENOPROTOOPT);
>
> if (rcu_access_pointer(ctx->peer))
> return aa_get_label_rcu(&ctx->peer);
>
> - if (sk->sk_family != PF_UNIX)
> - return ERR_PTR(-ENOPROTOOPT);
> -
> - return label;
> + return ERR_PTR(-ENOPROTOOPT);
> }
>
> /**
^ permalink raw reply
* Re: [apparmor][PATCH] apparmor: fix incorrect success return value in unpack_tag_headers()
From: John Johansen @ 2026-03-18 5:53 UTC (permalink / raw)
To: Massimiliano Pellizzer; +Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260210172159.535137-1-mpellizzer.dev@gmail.com>
On 2/10/26 09:21, Massimiliano Pellizzer wrote:
> unpack_tag_headers() returns `true` (1) on success instead of 0.
> Since it's caller unpack_tags() checks the return value with
> `if (error)`, a non-zero success value is incorrectly treated as
> a failure, causing tag header unpacking to always even if the data
> is well-formed.
>
> Change the success return in unpack_tag_headers() from `true` to 0.
>
> Fixes: 3d28e2397af7 ("apparmor: add support loading per permission tagging")
> Signed-off-by: Massimiliano Pellizzer <mpellizzer.dev@gmail.com>
sorry, my reply to this seems to have failed. This was pulled in for the
7.0 PR
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/policy_unpack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index dc908e1f5a88..221208788025 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -825,7 +825,7 @@ static int unpack_tag_headers(struct aa_ext *e, struct aa_tags_struct *tags)
> tags->hdrs.size = size;
> tags->hdrs.table = hdrs;
> AA_DEBUG(DEBUG_UNPACK, "headers %ld size %d", (long) hdrs, size);
> - return true;
> + return 0;
>
> fail:
> kfree_sensitive(hdrs);
^ permalink raw reply
* [PATCH] lsm: Fix the crash issue in xfrm_decode_session
From: Feng Yang @ 2026-03-18 6:19 UTC (permalink / raw)
To: paul, jmorris, serge; +Cc: linux-security-module, linux-kernel
From: Feng Yang <yangfeng@kylinos.cn>
After hooking the following BPF program:
SEC("lsm/xfrm_decode_session")
int BPF_PROG(lsm_hook_xfrm_decode_session, struct sk_buff *skb, u32 *secid, int ckall)
{
return 1; // Any non-zero value
}
Subsequent packet transmission triggers will cause a kernel panic:
[ 112.838874] ------------[ cut here ]------------
[ 112.838895] kernel BUG at security/security.c:5282!
[ 112.838902] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[ 112.838905] CPU: 5 PID: 4962 Comm: test Kdump: loaded Not tainted 6.19.0-rc5-gae23bc81ddf7 #2 PREEMPT(full)
[ 112.838907] Source Version: 55e2f799c748c8e195569363edbd1d6a4159675a
[ 112.838908] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 112.838909] RIP: 0010:security_skb_classify_flow+0x3f/0x50
[ 112.838914] Code: 85 db 74 28 49 89 fc 48 8d 6e 14 eb 08 48 8b 1b 48 85 db 74 17 31 d2 48 8b 43 18 48 89 ee 4c 89 e7 e8 05 33 86 00 85 c0 74 e3 <0f> 0b 5b 5d 41 5c c3 cc cc cc cc 66 0f 1f 44 00 00 90 90 90 90 90
[ 112.838915] RSP: 0018:ffffc28400200b10 EFLAGS: 00010202
[ 112.838918] RAX: 0000000000000001 RBX: ffffffff91d346d8 RCX: 0000000000000000
[ 112.838919] RDX: ffffa0890f5eaf80 RSI: 0000000000000001 RDI: ffffa0890f5eaf80
[ 112.838920] RBP: ffffc28400200d04 R08: 00000000000000c7 R09: 0000000000000002
[ 112.838922] R10: 0000000000000000 R11: 000000000000000f R12: ffffa089086dedc0
[ 112.838923] R13: ffffc28400200cf0 R14: ffffa08901ab2000 R15: 0000000000000000
[ 112.838926] FS: 00007fb087dd2680(0000) GS:ffffa0891ba80000(0000) knlGS:0000000000000000
[ 112.838927] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 112.838929] CR2: 00007fb087d1b940 CR3: 0000000107520006 CR4: 00000000000706e0
[ 112.838930] Call Trace:
[ 112.838931] <IRQ>
[ 112.838933] icmp_route_lookup.constprop.0+0xd7/0x460
[ 112.838941] ? switch_hrtimer_base+0x135/0x180
[ 112.838944] ? update_sg_lb_stats+0x9c/0x440
[ 112.838949] __icmp_send+0x3d3/0x740
[ 112.838952] ? __udp4_lib_rcv+0x427/0x6f0
[ 112.838955] __udp4_lib_rcv+0x427/0x6f0
[ 112.838957] ip_protocol_deliver_rcu+0xb7/0x170
[ 112.838960] ip_local_deliver_finish+0x76/0xa0
[ 112.838961] __netif_receive_skb_one_core+0x89/0xa0
[ 112.838967] process_backlog+0x95/0x140
[ 112.838969] __napi_poll+0x2b/0x1c0
[ 112.838971] net_rx_action+0x2aa/0x3a0
[ 112.838972] ? swake_up_one+0x41/0x70
[ 112.838974] ? kvm_sched_clock_read+0x11/0x20
[ 112.838977] handle_softirqs+0xe3/0x2e0
[ 112.838980] do_softirq+0x43/0x60
[ 112.838982] </IRQ>
[ 112.838982] <TASK>
[ 112.838983] __local_bh_enable_ip+0x68/0x70
[ 112.838985] __dev_queue_xmit+0x1c4/0x820
[ 112.838987] ? nf_hook_slow+0x45/0xd0
[ 112.838989] ip_finish_output2+0x1da/0x4a0
[ 112.838992] ip_send_skb+0x86/0x90
[ 112.838994] udp_send_skb+0x15e/0x380
[ 112.838996] udp_sendmsg+0xb9a/0xf80
[ 112.838998] ? __pfx_ip_generic_getfrag+0x10/0x10
[ 112.839003] ? __sys_sendto+0x1e4/0x210
[ 112.839005] __sys_sendto+0x1e4/0x210
[ 112.839007] ? __handle_mm_fault+0x2fc/0x6c0
[ 112.839013] __x64_sys_sendto+0x24/0x30
[ 112.839014] do_syscall_64+0x5f/0x270
[ 112.839017] entry_SYSCALL_64_after_hwframe+0x76/0xe0
[ 112.839020] RIP: 0033:0x7fb087cfdb17
[ 112.839021] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b8 0f 1f 00 f3 0f 1e fa 80 3d 55 c8 0c 00 00 41 89 ca 74 10 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 71 c3 55 48 83 ec 30 44 89 4c 24 2c 4c 89 44
[ 112.839023] RSP: 002b:00007ffea64704e8 EFLAGS: 00000202 ORIG_RAX: 000000000000002c
[ 112.839025] RAX: ffffffffffffffda RBX: 00007ffea6470638 RCX: 00007fb087cfdb17
[ 112.839026] RDX: 0000000000000008 RSI: 00007ffea64704f8 RDI: 0000000000000003
[ 112.839027] RBP: 00007ffea6470520 R08: 00007ffea6470500 R09: 0000000000000010
[ 112.839029] R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000000000
[ 112.839030] R13: 00007ffea6470648 R14: 0000000000403df0 R15: 00007fb087e15000
[ 112.839032] </TASK>
This BUG_ON was first mentioned in [1], but I could not find any explanatory record of why this check is needed.
[1] https://lore.kernel.org/all/Pine.LNX.4.64.0607122149070.573@d.namei/
In the existing LSM_HOOK_INIT(xfrm_decode_session, selinux_xfrm_decode_session),
when the `ckall` parameter of the `selinux_xfrm_decode_session` function is 0,
it can only return 0 and will not trigger BUG_ON.
Therefore, remove the BUG_ON check to fix this issue.
Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
Closes: https://lore.kernel.org/all/4c4d04ba.6c12b.19c039b69e6.Coremail.kaiyanm@hust.edu.cn/
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
security/security.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/security/security.c b/security/security.c
index 67af9228c4e9..198f650070da 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4991,10 +4991,7 @@ int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
void security_skb_classify_flow(struct sk_buff *skb, struct flowi_common *flic)
{
- int rc = call_int_hook(xfrm_decode_session, skb, &flic->flowic_secid,
- 0);
-
- BUG_ON(rc);
+ call_int_hook(xfrm_decode_session, skb, &flic->flowic_secid, 0);
}
EXPORT_SYMBOL(security_skb_classify_flow);
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH RESEND] apparmor: Replace memcpy + NUL termination with kmemdup_nul in do_setattr
From: John Johansen @ 2026-03-18 6:26 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260222204645.285727-1-thorsten.blum@linux.dev>
On 2/22/26 12:46, Thorsten Blum wrote:
> Use kmemdup_nul() to copy 'value' instead of using memcpy() followed by
> a manual NUL termination. No functional changes.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
sorry this got dropped. It has been now pulled into my tree
> ---
> security/apparmor/lsm.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..49aa6ad68838 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -858,12 +858,9 @@ static int do_setattr(u64 attr, void *value, size_t size)
>
> /* AppArmor requires that the buffer must be null terminated atm */
> if (args[size - 1] != '\0') {
> - /* null terminate */
> - largs = args = kmalloc(size + 1, GFP_KERNEL);
> + largs = args = kmemdup_nul(value, size, GFP_KERNEL);
> if (!args)
> return -ENOMEM;
> - memcpy(args, value, size);
> - args[size] = '\0';
> }
>
> error = -EINVAL;
^ permalink raw reply
* Re: [PATCH] apparmor: Use sysfs_emit in param_get_{audit,mode}
From: John Johansen @ 2026-03-18 6:29 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260222214038.287814-1-thorsten.blum@linux.dev>
On 2/22/26 13:40, Thorsten Blum wrote:
> Replace sprintf() with sysfs_emit() in param_get_audit() and
> param_get_mode(). sysfs_emit() is preferred for formatting sysfs output
> because it provides safer bounds checking. Add terminating newlines as
> suggested by checkpatch.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Acked-by: John Johansen <john.johansen@canoncal.com>
I have pulled this into my tree
> ---
> security/apparmor/lsm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index a87cd60ed206..1250192f7b12 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -17,6 +17,7 @@
> #include <linux/ptrace.h>
> #include <linux/ctype.h>
> #include <linux/sysctl.h>
> +#include <linux/sysfs.h>
> #include <linux/audit.h>
> #include <linux/user_namespace.h>
> #include <linux/netfilter_ipv4.h>
> @@ -2081,7 +2082,7 @@ static int param_get_audit(char *buffer, const struct kernel_param *kp)
> return -EINVAL;
> if (apparmor_initialized && !aa_current_policy_view_capable(NULL))
> return -EPERM;
> - return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
> + return sysfs_emit(buffer, "%s\n", audit_mode_names[aa_g_audit]);
> }
>
> static int param_set_audit(const char *val, const struct kernel_param *kp)
> @@ -2109,8 +2110,7 @@ static int param_get_mode(char *buffer, const struct kernel_param *kp)
> return -EINVAL;
> if (apparmor_initialized && !aa_current_policy_view_capable(NULL))
> return -EPERM;
> -
> - return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
> + return sysfs_emit(buffer, "%s\n", aa_profile_mode_names[aa_g_profile_mode]);
> }
>
> static int param_set_mode(const char *val, const struct kernel_param *kp)
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: Replace memcpy + NUL termination with kmemdup_nul in do_setattr
From: John Johansen @ 2026-03-18 6:31 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260309224150.84575-3-thorsten.blum@linux.dev>
On 3/9/26 15:41, Thorsten Blum wrote:
> Use kmemdup_nul() to copy 'value' instead of using memcpy() followed by
> a manual NUL termination. No functional changes.
>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
sorry this got lost in the mess of the last month, this has
now been pulled into my tree
> ---
> security/apparmor/lsm.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..49aa6ad68838 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -858,12 +858,9 @@ static int do_setattr(u64 attr, void *value, size_t size)
>
> /* AppArmor requires that the buffer must be null terminated atm */
> if (args[size - 1] != '\0') {
> - /* null terminate */
> - largs = args = kmalloc(size + 1, GFP_KERNEL);
> + largs = args = kmemdup_nul(value, size, GFP_KERNEL);
> if (!args)
> return -ENOMEM;
> - memcpy(args, value, size);
> - args[size] = '\0';
> }
>
> error = -EINVAL;
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: Use sysfs_emit in param_get_{audit,mode}
From: John Johansen @ 2026-03-18 6:31 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260318000758.1568-3-thorsten.blum@linux.dev>
On 3/17/26 17:08, Thorsten Blum wrote:
> Replace sprintf() with sysfs_emit() in param_get_audit() and
> param_get_mode(). sysfs_emit() is preferred for formatting sysfs output
> because it provides safer bounds checking. Add terminating newlines as
> suggested by checkpatch.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
this has now been pulled into my tree
> ---
> security/apparmor/lsm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..cdf19a5e7626 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -17,6 +17,7 @@
> #include <linux/ptrace.h>
> #include <linux/ctype.h>
> #include <linux/sysctl.h>
> +#include <linux/sysfs.h>
> #include <linux/audit.h>
> #include <linux/user_namespace.h>
> #include <linux/netfilter_ipv4.h>
> @@ -2073,7 +2074,7 @@ static int param_get_audit(char *buffer, const struct kernel_param *kp)
> return -EINVAL;
> if (apparmor_initialized && !aa_current_policy_view_capable(NULL))
> return -EPERM;
> - return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
> + return sysfs_emit(buffer, "%s\n", audit_mode_names[aa_g_audit]);
> }
>
> static int param_set_audit(const char *val, const struct kernel_param *kp)
> @@ -2101,8 +2102,7 @@ static int param_get_mode(char *buffer, const struct kernel_param *kp)
> return -EINVAL;
> if (apparmor_initialized && !aa_current_policy_view_capable(NULL))
> return -EPERM;
> -
> - return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
> + return sysfs_emit(buffer, "%s\n", aa_profile_mode_names[aa_g_profile_mode]);
> }
>
> static int param_set_mode(const char *val, const struct kernel_param *kp)
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: Remove redundant if check in sk_peer_get_label
From: John Johansen @ 2026-03-18 6:32 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260318002141.3362-3-thorsten.blum@linux.dev>
On 3/17/26 17:21, Thorsten Blum wrote:
> Remove the redundant if check in sk_peer_get_label() and return
> ERR_PTR(-ENOPROTOOPT) directly.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
this has now been pulled into my tree
> ---
> security/apparmor/lsm.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c1d42fc72fdb..f7bcfed40222 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1528,15 +1528,11 @@ static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> static struct aa_label *sk_peer_get_label(struct sock *sk)
> {
> struct aa_sk_ctx *ctx = aa_sock(sk);
> - struct aa_label *label = ERR_PTR(-ENOPROTOOPT);
>
> if (rcu_access_pointer(ctx->peer))
> return aa_get_label_rcu(&ctx->peer);
>
> - if (sk->sk_family != PF_UNIX)
> - return ERR_PTR(-ENOPROTOOPT);
> -
> - return label;
> + return ERR_PTR(-ENOPROTOOPT);
> }
>
> /**
^ permalink raw reply
* Re: [PATCH] apparmor: hide unused get_loaddata_common_ref() function
From: John Johansen @ 2026-03-18 6:33 UTC (permalink / raw)
To: Arnd Bergmann, Paul Moore, James Morris, Serge E. Hallyn,
Maxime Bélair, Cengiz Can, Georgia Garcia
Cc: Arnd Bergmann, Christian Brauner, Jeff Layton, NeilBrown,
apparmor, linux-security-module, linux-kernel
In-Reply-To: <20260316135935.3321551-1-arnd@kernel.org>
On 3/16/26 06:59, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The newly introduced function is only used in an #ifdef block,
> which causes a harmless warning:
>
> security/apparmor/apparmorfs.c:177:28: error: 'get_loaddata_common_ref' defined but not used [-Werror=unused-function]
> 177 | static struct aa_loaddata *get_loaddata_common_ref(struct aa_common_ref *ref)
>
> Move the definition next to the user to avoid the warning.
>
> Fixes: 8e135b8aee5a ("apparmor: fix race between freeing data and fs accessing it")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: John Johansen <john.johansen@canonical.com>
this has now been pulled into my tree
> ---
> Alternatively, the #ifdef checks could be replaced with an
> 'if(IS_ENABLED(CONFIG_SECURITY_APPARMOR_EXPORT_BINARY) return;'
> check in __aa_fs_create_rawdata(), relying on the compiler's
> dead code elimination.
> ---
> security/apparmor/apparmorfs.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index ededaf46f3ca..f762b101d682 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -174,14 +174,6 @@ static struct aa_proxy *get_proxy_common_ref(struct aa_common_ref *ref)
> return NULL;
> }
>
> -static struct aa_loaddata *get_loaddata_common_ref(struct aa_common_ref *ref)
> -{
> - if (ref)
> - return aa_get_i_loaddata(container_of(ref, struct aa_loaddata,
> - count));
> - return NULL;
> -}
> -
> static void aa_put_common_ref(struct aa_common_ref *ref)
> {
> if (!ref)
> @@ -1318,6 +1310,14 @@ static const struct file_operations seq_rawdata_ ##NAME ##_fops = { \
> .release = seq_rawdata_release, \
> } \
>
> +static struct aa_loaddata *get_loaddata_common_ref(struct aa_common_ref *ref)
> +{
> + if (ref)
> + return aa_get_i_loaddata(container_of(ref, struct aa_loaddata,
> + count));
> + return NULL;
> +}
> +
> static int seq_rawdata_open(struct inode *inode, struct file *file,
> int (*show)(struct seq_file *, void *))
> {
^ permalink raw reply
* Re: [PATCH] lsm: Fix the crash issue in xfrm_decode_session
From: Feng Yang @ 2026-03-18 8:37 UTC (permalink / raw)
To: yangfeng59949; +Cc: jmorris, linux-kernel, linux-security-module, paul, serge
In-Reply-To: <20260318061925.134954-1-yangfeng59949@163.com>
> After hooking the following BPF program:
> SEC("lsm/xfrm_decode_session")
> int BPF_PROG(lsm_hook_xfrm_decode_session, struct sk_buff *skb, u32 *secid, int ckall)
> {
> return 1; // Any non-zero value
There is an error here. Any error value between -4095 and -1 will trigger this bug,
while other values in the range [-4095, 0] will be rejected by the validator.
> }
^ permalink raw reply
* Re: [PATCH v6 1/9] lsm: Add LSM hook security_unix_find
From: Mickaël Salaün @ 2026-03-18 8:48 UTC (permalink / raw)
To: John Johansen, Georgia Garcia
Cc: Paul Moore, Günther Noack, James Morris, Serge E . Hallyn,
Tingmao Wang, Justin Suess, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross, Jann Horn,
Tahera Fahimi, Sebastian Andrzej Siewior, Kuniyuki Iwashima,
Simon Horman, netdev, Alexander Viro, Christian Brauner
In-Reply-To: <2697b9f672967b1318630f2ffa21914f@paul-moore.com>
On Tue, Mar 17, 2026 at 05:34:57PM -0400, Paul Moore wrote:
> On Mar 15, 2026 =?UTF-8?q?G=C3=BCnther=20Noack?= <gnoack3000@gmail.com> wrote:
> >
> > Add a LSM hook security_unix_find.
> >
> > This hook is called to check the path of a named unix socket before a
> > connection is initiated. The peer socket may be inspected as well.
> >
> > Why existing hooks are unsuitable:
> >
> > Existing socket hooks, security_unix_stream_connect(),
> > security_unix_may_send(), and security_socket_connect() don't provide
> > TOCTOU-free / namespace independent access to the paths of sockets.
> >
> > (1) We cannot resolve the path from the struct sockaddr in existing hooks.
> > This requires another path lookup. A change in the path between the
> > two lookups will cause a TOCTOU bug.
> >
> > (2) We cannot use the struct path from the listening socket, because it
> > may be bound to a path in a different namespace than the caller,
> > resulting in a path that cannot be referenced at policy creation time.
> >
> > Cc: Günther Noack <gnoack3000@gmail.com>
> > Cc: Tingmao Wang <m@maowtm.org>
> > Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> > ---
> > include/linux/lsm_hook_defs.h | 5 +++++
> > include/linux/security.h | 11 +++++++++++
> > net/unix/af_unix.c | 13 ++++++++++---
> > security/security.c | 20 ++++++++++++++++++++
> > 4 files changed, 46 insertions(+), 3 deletions(-)
>
> Some really minor nitpicky things (below), but nothing critical.
> However, as we discussed, I would like to see the AppArmor folks comment
> on the new hook before we merge anything as I know they have an interest
> here.
John, Georgia, we've been discussing this new hook for a few months now
but didn't hear from you yet. We plan to merge this patch series with
the 7.1 merge window (in a few weeks), so before that I'd like to merge
it in -next in a few days to get a broader coverage. I'm pretty sure
this hook will work well with AppArmor too, but could you please take
look to confirm?
^ permalink raw reply
* [PATCH] lsm: export security_mmap_backing_file
From: Arnd Bergmann @ 2026-03-18 10:42 UTC (permalink / raw)
To: Paul Moore, James Morris, Serge E. Hallyn, Amir Goldstein
Cc: Arnd Bergmann, Casey Schaufler, John Johansen, Christian Brauner,
Mimi Zohar, linux-security-module, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
This symbol can now be used from a loadable erofs module, which causes
a build failure when it is not exported:
ERROR: modpost: "security_mmap_backing_file" [fs/erofs/erofs.ko] undefined!
Fixes: 0e2baaf8fed0 ("lsm: add the security_mmap_backing_file() hook")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
security/security.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/security.c b/security/security.c
index 8d10b184ce25..3e426a762864 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2529,6 +2529,7 @@ int security_mmap_backing_file(struct vm_area_struct *vma,
return call_int_hook(mmap_backing_file, vma, backing_file, user_file);
}
+EXPORT_SYMBOL_GPL(security_mmap_backing_file);
/**
* security_mmap_addr() - Check if mmap'ing an address is allowed
--
2.39.5
^ permalink raw reply related
* Re: [PATCH 1/3] backing_file: store user_path_file
From: Christian Brauner @ 2026-03-18 10:56 UTC (permalink / raw)
To: Paul Moore, Amir Goldstein
Cc: linux-security-module, selinux, linux-fsdevel, linux-unionfs,
linux-erofs, Gao Xiang
In-Reply-To: <20260316213606.374109-6-paul@paul-moore.com>
On Mon, Mar 16, 2026 at 05:35:56PM -0400, Paul Moore wrote:
> From: Amir Goldstein <amir73il@gmail.com>
>
> Instead of storing the user_path, store an O_PATH file for the
> user_path with the original user file creds and a security context.
>
> The user_path_file is only exported as a const pointer and its refcnt
> is initialized to FILE_REF_DEAD, because it is not a refcounted object.
>
> The only caller of file_ref_init() is now open coded, so the helper
> is removed.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Tested-by: Paul Moore <paul@paul-moore.com> (SELinux)
> Acked-by: Gao Xiang <hsiangkao@linux.alibaba.com> (EROFS)
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> fs/backing-file.c | 20 ++++++++------
> fs/erofs/ishare.c | 6 ++--
> fs/file_table.c | 53 ++++++++++++++++++++++++++++--------
> fs/fuse/passthrough.c | 3 +-
> fs/internal.h | 5 ++--
> fs/overlayfs/dir.c | 3 +-
> fs/overlayfs/file.c | 1 +
> include/linux/backing-file.h | 29 ++++++++++++++++++--
> include/linux/file_ref.h | 10 -------
> 9 files changed, 90 insertions(+), 40 deletions(-)
>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index 45da8600d564..acabeea7efff 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -11,6 +11,7 @@
> #include <linux/fs.h>
> #include <linux/backing-file.h>
> #include <linux/splice.h>
> +#include <linux/uio.h>
> #include <linux/mm.h>
>
> #include "internal.h"
> @@ -18,9 +19,10 @@
> /**
> * backing_file_open - open a backing file for kernel internal use
> * @user_path: path that the user reuqested to open
> + * @user_cred: credentials that the user used for open
> * @flags: open flags
> * @real_path: path of the backing file
> - * @cred: credentials for open
> + * @cred: credentials for open of the backing file
> *
> * Open a backing file for a stackable filesystem (e.g., overlayfs).
> * @user_path may be on the stackable filesystem and @real_path on the
> @@ -29,19 +31,19 @@
> * returned file into a container structure that also stores the stacked
> * file's path, which can be retrieved using backing_file_user_path().
> */
> -struct file *backing_file_open(const struct path *user_path, int flags,
> +struct file *backing_file_open(const struct path *user_path,
> + const struct cred *user_cred, int flags,
> const struct path *real_path,
> const struct cred *cred)
> {
> struct file *f;
> int error;
>
> - f = alloc_empty_backing_file(flags, cred);
> + f = alloc_empty_backing_file(flags, cred, user_cred);
> if (IS_ERR(f))
> return f;
>
> - path_get(user_path);
> - backing_file_set_user_path(f, user_path);
> + backing_file_open_user_path(f, user_path);
> error = vfs_open(real_path, f);
> if (error) {
> fput(f);
> @@ -52,7 +54,8 @@ struct file *backing_file_open(const struct path *user_path, int flags,
> }
> EXPORT_SYMBOL_GPL(backing_file_open);
>
> -struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> +struct file *backing_tmpfile_open(const struct path *user_path,
> + const struct cred *user_cred, int flags,
> const struct path *real_parentpath,
> umode_t mode, const struct cred *cred)
> {
> @@ -60,12 +63,11 @@ struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> struct file *f;
> int error;
>
> - f = alloc_empty_backing_file(flags, cred);
> + f = alloc_empty_backing_file(flags, cred, user_cred);
> if (IS_ERR(f))
> return f;
>
> - path_get(user_path);
> - backing_file_set_user_path(f, user_path);
> + backing_file_open_user_path(f, user_path);
> error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
> if (error) {
> fput(f);
> diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
> index 829d50d5c717..17a4941d4518 100644
> --- a/fs/erofs/ishare.c
> +++ b/fs/erofs/ishare.c
> @@ -106,15 +106,15 @@ static int erofs_ishare_file_open(struct inode *inode, struct file *file)
>
> if (file->f_flags & O_DIRECT)
> return -EINVAL;
> - realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred());
> + realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred(),
> + file->f_cred);
> if (IS_ERR(realfile))
> return PTR_ERR(realfile);
> ihold(sharedinode);
> realfile->f_op = &erofs_file_fops;
> realfile->f_inode = sharedinode;
> realfile->f_mapping = sharedinode->i_mapping;
> - path_get(&file->f_path);
> - backing_file_set_user_path(realfile, &file->f_path);
> + backing_file_open_user_path(realfile, &file->f_path);
>
> file_ra_state_init(&realfile->f_ra, file->f_mapping);
> realfile->private_data = EROFS_I(inode);
> diff --git a/fs/file_table.c b/fs/file_table.c
> index aaa5faaace1e..b7dc94656c44 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -27,6 +27,7 @@
> #include <linux/task_work.h>
> #include <linux/swap.h>
> #include <linux/kmemleak.h>
> +#include <linux/backing-file.h>
>
> #include <linux/atomic.h>
>
> @@ -43,11 +44,11 @@ static struct kmem_cache *bfilp_cachep __ro_after_init;
>
> static struct percpu_counter nr_files __cacheline_aligned_in_smp;
>
> -/* Container for backing file with optional user path */
> +/* Container for backing file with optional user path file */
> struct backing_file {
> struct file file;
> union {
> - struct path user_path;
> + struct file user_path_file;
> freeptr_t bf_freeptr;
> };
> };
> @@ -56,24 +57,44 @@ struct backing_file {
>
> const struct path *backing_file_user_path(const struct file *f)
> {
> - return &backing_file(f)->user_path;
> + return &backing_file(f)->user_path_file.f_path;
> }
> EXPORT_SYMBOL_GPL(backing_file_user_path);
>
> -void backing_file_set_user_path(struct file *f, const struct path *path)
> +const struct file *backing_file_user_path_file(const struct file *f)
> {
> - backing_file(f)->user_path = *path;
> + return &backing_file(f)->user_path_file;
> +}
> +EXPORT_SYMBOL_GPL(backing_file_user_path_file);
> +
> +void backing_file_open_user_path(struct file *f, const struct path *path)
I think this is a bad idea. This should return an error but still
WARN_ON(). Just make callers handle that error just like we do
everywhere else.
> +{
> + /* open an O_PATH file to reference the user path - cannot fail */
> + WARN_ON(vfs_open(path, &backing_file(f)->user_path_file));
> +}
> +EXPORT_SYMBOL_GPL(backing_file_open_user_path);
> +
> +static void destroy_file(struct file *f)
> +{
> + security_file_free(f);
> + put_cred(f->f_cred);
Note that calling destroy_file() in this way bypasses
security_file_release(). Presumably this doesn't matter because no LSM
does a security_alloc_file() for this but it adds a nother wrinkly into
the cleanup path.
> }
> -EXPORT_SYMBOL_GPL(backing_file_set_user_path);
>
> static inline void file_free(struct file *f)
> {
> - security_file_free(f);
> + destroy_file(f);
> if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
> percpu_counter_dec(&nr_files);
> - put_cred(f->f_cred);
> if (unlikely(f->f_mode & FMODE_BACKING)) {
> - path_put(backing_file_user_path(f));
> + struct file *user_path_file = &backing_file(f)->user_path_file;
> +
> + /*
> + * no refcount on the user_path_file - they die together,
> + * so __fput() is not called for user_path_file. path_put()
> + * is the only relevant cleanup from __fput().
> + */
> + destroy_file(user_path_file);
> + path_put(&user_path_file->__f_path);
> kmem_cache_free(bfilp_cachep, backing_file(f));
> } else {
> kmem_cache_free(filp_cachep, f);
> @@ -201,7 +222,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
> * fget-rcu pattern users need to be able to handle spurious
> * refcount bumps we should reinitialize the reused file first.
> */
> - file_ref_init(&f->f_ref, 1);
> + atomic_long_set(&f->f_ref.refcnt, FILE_REF_ONEREF);
No, please don't open-code this. The point is to stop any open-access to
f_ref. And also below you introduce another atomic_long_set() open-coded
call as well. Simply adapt file_ref_init() to not do the -1 subtraction
and use the constants directly.
> return 0;
> }
>
> @@ -290,7 +311,8 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
> * This is only for kernel internal use, and the allocate file must not be
> * installed into file tables or such.
> */
> -struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> +struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
> + const struct cred *user_cred)
> {
> struct backing_file *ff;
> int error;
> @@ -305,6 +327,15 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> return ERR_PTR(error);
> }
>
> + error = init_file(&ff->user_path_file, O_PATH, user_cred);
> + /* user_path_file is not refcounterd - it dies with the backing file */
> + atomic_long_set(&ff->user_path_file.f_ref.refcnt, FILE_REF_DEAD);
Please massage this and send that patch. I'll stuff it into a stable vfs
branch that both Paul and I can merge. Paul can then send his PR.
^ permalink raw reply
* Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path
From: Sebastian Andrzej Siewior @ 2026-03-18 11:15 UTC (permalink / raw)
To: Günther Noack
Cc: Mickaël Salaün, John Johansen, Tingmao Wang,
Justin Suess, Kuniyuki Iwashima, Jann Horn, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross,
Tahera Fahimi
In-Reply-To: <20260315222150.121952-4-gnoack3000@gmail.com>
On 2026-03-15 23:21:44 [+0100], Günther Noack wrote:
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
…
> @@ -1557,6 +1560,110 @@ static int hook_path_truncate(const struct path *const path)
…
> +static int hook_unix_find(const struct path *const path, struct sock *other,
> + int flags)
> +{
…
> + /* Checks the layers in which we are connecting within the same domain. */
> + unix_state_lock(other);
> + if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket ||
> + !other->sk_socket->file)) {
> + unix_state_unlock(other);
> + return 0;
> + }
> + dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
> + unix_state_unlock(other);
> +
> + unmask_scoped_access(subject->domain, dom_other, &layer_masks,
> + fs_resolve_unix.fs);
This might be obvious but in case it is not: You obtain the domain
pointer from f_cred->security. Within the unix_state_lock() block the fd
can not be closed. Once you drop the lock, the fd can be closed. What
guarantees that the domain/ dom_other point remains valid between
unix_state_unlock() and after unmask_scoped_access()?
Is this invoked within a RCU section which would delay put_cred_rcu() or
is there other magic involved?
Sebastian
^ permalink raw reply
* Re: [PATCH 1/3] backing_file: store user_path_file
From: Amir Goldstein @ 2026-03-18 12:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Paul Moore, linux-security-module, selinux, linux-fsdevel,
linux-unionfs, linux-erofs, Gao Xiang
In-Reply-To: <20260318-einsam-sellerie-2d547dd338ee@brauner>
On Wed, Mar 18, 2026 at 11:57 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 05:35:56PM -0400, Paul Moore wrote:
> > From: Amir Goldstein <amir73il@gmail.com>
> >
> > Instead of storing the user_path, store an O_PATH file for the
> > user_path with the original user file creds and a security context.
> >
> > The user_path_file is only exported as a const pointer and its refcnt
> > is initialized to FILE_REF_DEAD, because it is not a refcounted object.
> >
> > The only caller of file_ref_init() is now open coded, so the helper
> > is removed.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Tested-by: Paul Moore <paul@paul-moore.com> (SELinux)
> > Acked-by: Gao Xiang <hsiangkao@linux.alibaba.com> (EROFS)
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> > fs/backing-file.c | 20 ++++++++------
> > fs/erofs/ishare.c | 6 ++--
> > fs/file_table.c | 53 ++++++++++++++++++++++++++++--------
> > fs/fuse/passthrough.c | 3 +-
> > fs/internal.h | 5 ++--
> > fs/overlayfs/dir.c | 3 +-
> > fs/overlayfs/file.c | 1 +
> > include/linux/backing-file.h | 29 ++++++++++++++++++--
> > include/linux/file_ref.h | 10 -------
> > 9 files changed, 90 insertions(+), 40 deletions(-)
> >
> > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > index 45da8600d564..acabeea7efff 100644
> > --- a/fs/backing-file.c
> > +++ b/fs/backing-file.c
> > @@ -11,6 +11,7 @@
> > #include <linux/fs.h>
> > #include <linux/backing-file.h>
> > #include <linux/splice.h>
> > +#include <linux/uio.h>
> > #include <linux/mm.h>
> >
> > #include "internal.h"
> > @@ -18,9 +19,10 @@
> > /**
> > * backing_file_open - open a backing file for kernel internal use
> > * @user_path: path that the user reuqested to open
> > + * @user_cred: credentials that the user used for open
> > * @flags: open flags
> > * @real_path: path of the backing file
> > - * @cred: credentials for open
> > + * @cred: credentials for open of the backing file
> > *
> > * Open a backing file for a stackable filesystem (e.g., overlayfs).
> > * @user_path may be on the stackable filesystem and @real_path on the
> > @@ -29,19 +31,19 @@
> > * returned file into a container structure that also stores the stacked
> > * file's path, which can be retrieved using backing_file_user_path().
> > */
> > -struct file *backing_file_open(const struct path *user_path, int flags,
> > +struct file *backing_file_open(const struct path *user_path,
> > + const struct cred *user_cred, int flags,
> > const struct path *real_path,
> > const struct cred *cred)
> > {
> > struct file *f;
> > int error;
> >
> > - f = alloc_empty_backing_file(flags, cred);
> > + f = alloc_empty_backing_file(flags, cred, user_cred);
> > if (IS_ERR(f))
> > return f;
> >
> > - path_get(user_path);
> > - backing_file_set_user_path(f, user_path);
> > + backing_file_open_user_path(f, user_path);
> > error = vfs_open(real_path, f);
> > if (error) {
> > fput(f);
> > @@ -52,7 +54,8 @@ struct file *backing_file_open(const struct path *user_path, int flags,
> > }
> > EXPORT_SYMBOL_GPL(backing_file_open);
> >
> > -struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> > +struct file *backing_tmpfile_open(const struct path *user_path,
> > + const struct cred *user_cred, int flags,
> > const struct path *real_parentpath,
> > umode_t mode, const struct cred *cred)
> > {
> > @@ -60,12 +63,11 @@ struct file *backing_tmpfile_open(const struct path *user_path, int flags,
> > struct file *f;
> > int error;
> >
> > - f = alloc_empty_backing_file(flags, cred);
> > + f = alloc_empty_backing_file(flags, cred, user_cred);
> > if (IS_ERR(f))
> > return f;
> >
> > - path_get(user_path);
> > - backing_file_set_user_path(f, user_path);
> > + backing_file_open_user_path(f, user_path);
> > error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
> > if (error) {
> > fput(f);
> > diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
> > index 829d50d5c717..17a4941d4518 100644
> > --- a/fs/erofs/ishare.c
> > +++ b/fs/erofs/ishare.c
> > @@ -106,15 +106,15 @@ static int erofs_ishare_file_open(struct inode *inode, struct file *file)
> >
> > if (file->f_flags & O_DIRECT)
> > return -EINVAL;
> > - realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred());
> > + realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred(),
> > + file->f_cred);
> > if (IS_ERR(realfile))
> > return PTR_ERR(realfile);
> > ihold(sharedinode);
> > realfile->f_op = &erofs_file_fops;
> > realfile->f_inode = sharedinode;
> > realfile->f_mapping = sharedinode->i_mapping;
> > - path_get(&file->f_path);
> > - backing_file_set_user_path(realfile, &file->f_path);
> > + backing_file_open_user_path(realfile, &file->f_path);
> >
> > file_ra_state_init(&realfile->f_ra, file->f_mapping);
> > realfile->private_data = EROFS_I(inode);
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index aaa5faaace1e..b7dc94656c44 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -27,6 +27,7 @@
> > #include <linux/task_work.h>
> > #include <linux/swap.h>
> > #include <linux/kmemleak.h>
> > +#include <linux/backing-file.h>
> >
> > #include <linux/atomic.h>
> >
> > @@ -43,11 +44,11 @@ static struct kmem_cache *bfilp_cachep __ro_after_init;
> >
> > static struct percpu_counter nr_files __cacheline_aligned_in_smp;
> >
> > -/* Container for backing file with optional user path */
> > +/* Container for backing file with optional user path file */
> > struct backing_file {
> > struct file file;
> > union {
> > - struct path user_path;
> > + struct file user_path_file;
> > freeptr_t bf_freeptr;
> > };
> > };
> > @@ -56,24 +57,44 @@ struct backing_file {
> >
> > const struct path *backing_file_user_path(const struct file *f)
> > {
> > - return &backing_file(f)->user_path;
> > + return &backing_file(f)->user_path_file.f_path;
> > }
> > EXPORT_SYMBOL_GPL(backing_file_user_path);
> >
> > -void backing_file_set_user_path(struct file *f, const struct path *path)
> > +const struct file *backing_file_user_path_file(const struct file *f)
> > {
> > - backing_file(f)->user_path = *path;
> > + return &backing_file(f)->user_path_file;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_user_path_file);
> > +
> > +void backing_file_open_user_path(struct file *f, const struct path *path)
>
> I think this is a bad idea. This should return an error but still
> WARN_ON(). Just make callers handle that error just like we do
> everywhere else.
OK.
>
> > +{
> > + /* open an O_PATH file to reference the user path - cannot fail */
> > + WARN_ON(vfs_open(path, &backing_file(f)->user_path_file));
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_open_user_path);
> > +
> > +static void destroy_file(struct file *f)
> > +{
> > + security_file_free(f);
> > + put_cred(f->f_cred);
>
> Note that calling destroy_file() in this way bypasses
> security_file_release(). Presumably this doesn't matter because no LSM
> does a security_alloc_file() for this but it adds a nother wrinkly into
> the cleanup path.
>
This is for Paul to comment on.
The way I see it, security_file_open() was not called on the user path file,
so no reason to call security_file_release()?
It is very much a possibility that LSM would want to allocate security
context for the user path file during backing_file_mmap, when both files
are available in context, so that later mprotect() will have this stored
information in the user path file security context.
But in this case, wouldn't security_file_free() be enough?
>
> > }
> > -EXPORT_SYMBOL_GPL(backing_file_set_user_path);
> >
> > static inline void file_free(struct file *f)
> > {
> > - security_file_free(f);
> > + destroy_file(f);
> > if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
> > percpu_counter_dec(&nr_files);
> > - put_cred(f->f_cred);
> > if (unlikely(f->f_mode & FMODE_BACKING)) {
> > - path_put(backing_file_user_path(f));
> > + struct file *user_path_file = &backing_file(f)->user_path_file;
> > +
> > + /*
> > + * no refcount on the user_path_file - they die together,
> > + * so __fput() is not called for user_path_file. path_put()
> > + * is the only relevant cleanup from __fput().
> > + */
> > + destroy_file(user_path_file);
> > + path_put(&user_path_file->__f_path);
> > kmem_cache_free(bfilp_cachep, backing_file(f));
> > } else {
> > kmem_cache_free(filp_cachep, f);
> > @@ -201,7 +222,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
> > * fget-rcu pattern users need to be able to handle spurious
> > * refcount bumps we should reinitialize the reused file first.
> > */
> > - file_ref_init(&f->f_ref, 1);
> > + atomic_long_set(&f->f_ref.refcnt, FILE_REF_ONEREF);
>
> No, please don't open-code this. The point is to stop any open-access to
> f_ref. And also below you introduce another atomic_long_set() open-coded
> call as well. Simply adapt file_ref_init() to not do the -1 subtraction
> and use the constants directly.
>
OK.
> > return 0;
> > }
> >
> > @@ -290,7 +311,8 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
> > * This is only for kernel internal use, and the allocate file must not be
> > * installed into file tables or such.
> > */
> > -struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> > +struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
> > + const struct cred *user_cred)
> > {
> > struct backing_file *ff;
> > int error;
> > @@ -305,6 +327,15 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> > return ERR_PTR(error);
> > }
> >
> > + error = init_file(&ff->user_path_file, O_PATH, user_cred);
> > + /* user_path_file is not refcounterd - it dies with the backing file */
> > + atomic_long_set(&ff->user_path_file.f_ref.refcnt, FILE_REF_DEAD);
>
> Please massage this and send that patch. I'll stuff it into a stable vfs
> branch that both Paul and I can merge. Paul can then send his PR.
Sure. I'll try to send it later today.
Thanks,
Amir.
^ permalink raw reply
* [PATCH v6] backing_file: store user_path_file
From: Amir Goldstein @ 2026-03-18 13:12 UTC (permalink / raw)
To: Christian Brauner
Cc: Miklos Szeredi, Paul Moore, Gao Xiang, linux-security-module,
selinux, linux-erofs, linux-fsdevel, linux-unionfs
Instead of storing the user_path, store an O_PATH file for the
user_path with the original user file creds and a security context.
The user_path_file is only exported as a const pointer and its refcnt
is initialized to FILE_REF_DEAD, because it is not a refcounted object.
The file_ref_init() helper was changed to accept the FILE_REF_ constant
instead of the fake +1 integer count.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
Christian,
My v5 patch was sent by Paul along with his LSM/selinux pataches [1].
Here are the changes you requested.
I removed the ACKs and Tested-by because of the changes.
Thanks,
Amir.
Changes since v5:
- Restore file_ref_init() helper without refcnt -1 offset
- Future proofing errors from backing_file_open_user_path()
[1] https://lore.kernel.org/r/20260316213606.374109-6-paul@paul-moore.com/
fs/backing-file.c | 26 ++++++++++--------
fs/erofs/ishare.c | 13 +++++++--
fs/file_table.c | 53 ++++++++++++++++++++++++++++--------
fs/fuse/passthrough.c | 3 +-
fs/internal.h | 5 ++--
fs/overlayfs/dir.c | 3 +-
fs/overlayfs/file.c | 1 +
include/linux/backing-file.h | 29 ++++++++++++++++++--
include/linux/file_ref.h | 4 +--
9 files changed, 103 insertions(+), 34 deletions(-)
diff --git a/fs/backing-file.c b/fs/backing-file.c
index 45da8600d5644..271ff27521063 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -11,6 +11,7 @@
#include <linux/fs.h>
#include <linux/backing-file.h>
#include <linux/splice.h>
+#include <linux/uio.h>
#include <linux/mm.h>
#include "internal.h"
@@ -18,9 +19,10 @@
/**
* backing_file_open - open a backing file for kernel internal use
* @user_path: path that the user reuqested to open
+ * @user_cred: credentials that the user used for open
* @flags: open flags
* @real_path: path of the backing file
- * @cred: credentials for open
+ * @cred: credentials for open of the backing file
*
* Open a backing file for a stackable filesystem (e.g., overlayfs).
* @user_path may be on the stackable filesystem and @real_path on the
@@ -29,20 +31,21 @@
* returned file into a container structure that also stores the stacked
* file's path, which can be retrieved using backing_file_user_path().
*/
-struct file *backing_file_open(const struct path *user_path, int flags,
+struct file *backing_file_open(const struct path *user_path,
+ const struct cred *user_cred, int flags,
const struct path *real_path,
const struct cred *cred)
{
struct file *f;
int error;
- f = alloc_empty_backing_file(flags, cred);
+ f = alloc_empty_backing_file(flags, cred, user_cred);
if (IS_ERR(f))
return f;
- path_get(user_path);
- backing_file_set_user_path(f, user_path);
- error = vfs_open(real_path, f);
+ error = backing_file_open_user_path(f, user_path);
+ if (!error)
+ error = vfs_open(real_path, f);
if (error) {
fput(f);
f = ERR_PTR(error);
@@ -52,7 +55,8 @@ struct file *backing_file_open(const struct path *user_path, int flags,
}
EXPORT_SYMBOL_GPL(backing_file_open);
-struct file *backing_tmpfile_open(const struct path *user_path, int flags,
+struct file *backing_tmpfile_open(const struct path *user_path,
+ const struct cred *user_cred, int flags,
const struct path *real_parentpath,
umode_t mode, const struct cred *cred)
{
@@ -60,13 +64,13 @@ struct file *backing_tmpfile_open(const struct path *user_path, int flags,
struct file *f;
int error;
- f = alloc_empty_backing_file(flags, cred);
+ f = alloc_empty_backing_file(flags, cred, user_cred);
if (IS_ERR(f))
return f;
- path_get(user_path);
- backing_file_set_user_path(f, user_path);
- error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
+ error = backing_file_open_user_path(f, user_path);
+ if (!error)
+ error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
if (error) {
fput(f);
f = ERR_PTR(error);
diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
index 829d50d5c717d..f3a5fb0bffaf0 100644
--- a/fs/erofs/ishare.c
+++ b/fs/erofs/ishare.c
@@ -103,18 +103,25 @@ static int erofs_ishare_file_open(struct inode *inode, struct file *file)
{
struct inode *sharedinode = EROFS_I(inode)->sharedinode;
struct file *realfile;
+ int err;
if (file->f_flags & O_DIRECT)
return -EINVAL;
- realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred());
+ realfile = alloc_empty_backing_file(O_RDONLY|O_NOATIME, current_cred(),
+ file->f_cred);
if (IS_ERR(realfile))
return PTR_ERR(realfile);
+
+ err = backing_file_open_user_path(realfile, &file->f_path);
+ if (err) {
+ fput(realfile);
+ return err;
+ }
+
ihold(sharedinode);
realfile->f_op = &erofs_file_fops;
realfile->f_inode = sharedinode;
realfile->f_mapping = sharedinode->i_mapping;
- path_get(&file->f_path);
- backing_file_set_user_path(realfile, &file->f_path);
file_ra_state_init(&realfile->f_ra, file->f_mapping);
realfile->private_data = EROFS_I(inode);
diff --git a/fs/file_table.c b/fs/file_table.c
index aaa5faaace1e9..e8b4eb2bbff85 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -27,6 +27,7 @@
#include <linux/task_work.h>
#include <linux/swap.h>
#include <linux/kmemleak.h>
+#include <linux/backing-file.h>
#include <linux/atomic.h>
@@ -43,11 +44,11 @@ static struct kmem_cache *bfilp_cachep __ro_after_init;
static struct percpu_counter nr_files __cacheline_aligned_in_smp;
-/* Container for backing file with optional user path */
+/* Container for backing file with optional user path file */
struct backing_file {
struct file file;
union {
- struct path user_path;
+ struct file user_path_file;
freeptr_t bf_freeptr;
};
};
@@ -56,24 +57,44 @@ struct backing_file {
const struct path *backing_file_user_path(const struct file *f)
{
- return &backing_file(f)->user_path;
+ return &backing_file(f)->user_path_file.f_path;
}
EXPORT_SYMBOL_GPL(backing_file_user_path);
-void backing_file_set_user_path(struct file *f, const struct path *path)
+const struct file *backing_file_user_path_file(const struct file *f)
{
- backing_file(f)->user_path = *path;
+ return &backing_file(f)->user_path_file;
}
-EXPORT_SYMBOL_GPL(backing_file_set_user_path);
+EXPORT_SYMBOL_GPL(backing_file_user_path_file);
-static inline void file_free(struct file *f)
+int backing_file_open_user_path(struct file *f, const struct path *path)
+{
+ /* open an O_PATH file to reference the user path - should not fail */
+ return WARN_ON(vfs_open(path, &backing_file(f)->user_path_file));
+}
+EXPORT_SYMBOL_GPL(backing_file_open_user_path);
+
+static void destroy_file(struct file *f)
{
security_file_free(f);
+ put_cred(f->f_cred);
+}
+
+static inline void file_free(struct file *f)
+{
+ destroy_file(f);
if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
percpu_counter_dec(&nr_files);
- put_cred(f->f_cred);
if (unlikely(f->f_mode & FMODE_BACKING)) {
- path_put(backing_file_user_path(f));
+ struct file *user_path_file = &backing_file(f)->user_path_file;
+
+ /*
+ * no refcount on the user_path_file - they die together,
+ * so __fput() is not called for user_path_file. path_put()
+ * is the only relevant cleanup from __fput().
+ */
+ destroy_file(user_path_file);
+ path_put(&user_path_file->__f_path);
kmem_cache_free(bfilp_cachep, backing_file(f));
} else {
kmem_cache_free(filp_cachep, f);
@@ -201,7 +222,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
* fget-rcu pattern users need to be able to handle spurious
* refcount bumps we should reinitialize the reused file first.
*/
- file_ref_init(&f->f_ref, 1);
+ file_ref_init(&f->f_ref, FILE_REF_ONEREF);
return 0;
}
@@ -290,7 +311,8 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
* This is only for kernel internal use, and the allocate file must not be
* installed into file tables or such.
*/
-struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
+struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
+ const struct cred *user_cred)
{
struct backing_file *ff;
int error;
@@ -305,6 +327,15 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
return ERR_PTR(error);
}
+ error = init_file(&ff->user_path_file, O_PATH, user_cred);
+ /* user_path_file is not refcounterd - it dies with the backing file */
+ file_ref_init(&ff->user_path_file.f_ref, FILE_REF_DEAD);
+ if (unlikely(error)) {
+ destroy_file(&ff->file);
+ kmem_cache_free(bfilp_cachep, ff);
+ return ERR_PTR(error);
+ }
+
ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
return &ff->file;
}
diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
index 72de97c03d0ee..60018c6359342 100644
--- a/fs/fuse/passthrough.c
+++ b/fs/fuse/passthrough.c
@@ -10,6 +10,7 @@
#include <linux/file.h>
#include <linux/backing-file.h>
#include <linux/splice.h>
+#include <linux/uio.h>
static void fuse_file_accessed(struct file *file)
{
@@ -167,7 +168,7 @@ struct fuse_backing *fuse_passthrough_open(struct file *file, int backing_id)
goto out;
/* Allocate backing file per fuse file to store fuse path */
- backing_file = backing_file_open(&file->f_path, file->f_flags,
+ backing_file = backing_file_open(&file->f_path, file->f_cred, file->f_flags,
&fb->file->f_path, fb->cred);
err = PTR_ERR(backing_file);
if (IS_ERR(backing_file)) {
diff --git a/fs/internal.h b/fs/internal.h
index cbc384a1aa096..7c44a58627ba3 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -106,8 +106,9 @@ extern void chroot_fs_refs(const struct path *, const struct path *);
*/
struct file *alloc_empty_file(int flags, const struct cred *cred);
struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred);
-struct file *alloc_empty_backing_file(int flags, const struct cred *cred);
-void backing_file_set_user_path(struct file *f, const struct path *path);
+struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
+ const struct cred *user_cred);
+int backing_file_open_user_path(struct file *f, const struct path *path);
static inline void file_put_write_access(struct file *file)
{
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 8c0a3d876fef1..5fd32ccc134d2 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1389,7 +1389,8 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
return PTR_ERR(cred);
ovl_path_upper(dentry->d_parent, &realparentpath);
- realfile = backing_tmpfile_open(&file->f_path, flags, &realparentpath,
+ realfile = backing_tmpfile_open(&file->f_path, file->f_cred,
+ flags, &realparentpath,
mode, current_cred());
err = PTR_ERR_OR_ZERO(realfile);
pr_debug("tmpfile/open(%pd2, 0%o) = %i\n", realparentpath.dentry, mode, err);
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 97bed2286030d..767c128407fcc 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -49,6 +49,7 @@ static struct file *ovl_open_realfile(const struct file *file,
flags &= ~O_NOATIME;
realfile = backing_file_open(file_user_path(file),
+ file_user_cred(file),
flags, realpath, current_cred());
}
}
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
index 1476a6ed1bfd7..8afba93f3ce07 100644
--- a/include/linux/backing-file.h
+++ b/include/linux/backing-file.h
@@ -9,19 +9,42 @@
#define _LINUX_BACKING_FILE_H
#include <linux/file.h>
-#include <linux/uio.h>
#include <linux/fs.h>
+/*
+ * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
+ * stored in ->vm_file is a backing file whose f_inode is on the underlying
+ * filesystem.
+ *
+ * LSM can use file_user_path_file() to store context related to the user path
+ * that was opened and mmaped.
+ */
+const struct file *backing_file_user_path_file(const struct file *f);
+
+static inline const struct file *file_user_path_file(const struct file *f)
+{
+ if (f && unlikely(f->f_mode & FMODE_BACKING))
+ return backing_file_user_path_file(f);
+ return f;
+}
+
+static inline const struct cred *file_user_cred(const struct file *f)
+{
+ return file_user_path_file(f)->f_cred;
+}
+
struct backing_file_ctx {
const struct cred *cred;
void (*accessed)(struct file *file);
void (*end_write)(struct kiocb *iocb, ssize_t);
};
-struct file *backing_file_open(const struct path *user_path, int flags,
+struct file *backing_file_open(const struct path *user_path,
+ const struct cred *user_cred, int flags,
const struct path *real_path,
const struct cred *cred);
-struct file *backing_tmpfile_open(const struct path *user_path, int flags,
+struct file *backing_tmpfile_open(const struct path *user_path,
+ const struct cred *user_cred, int flags,
const struct path *real_parentpath,
umode_t mode, const struct cred *cred);
ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h
index 31551e4cb8f34..c7512ce70f9c4 100644
--- a/include/linux/file_ref.h
+++ b/include/linux/file_ref.h
@@ -54,11 +54,11 @@ typedef struct {
/**
* file_ref_init - Initialize a file reference count
* @ref: Pointer to the reference count
- * @cnt: The initial reference count typically '1'
+ * @cnt: The initial reference count typically FILE_REF_ONEREF
*/
static inline void file_ref_init(file_ref_t *ref, unsigned long cnt)
{
- atomic_long_set(&ref->refcnt, cnt - 1);
+ atomic_long_set(&ref->refcnt, cnt);
}
bool __file_ref_put(file_ref_t *ref, unsigned long cnt);
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path
From: Justin Suess @ 2026-03-18 14:14 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Günther Noack, Mickaël Salaün, John Johansen,
Tingmao Wang, Kuniyuki Iwashima, Jann Horn, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross,
Tahera Fahimi
In-Reply-To: <20260318111507.1nr6rAki@linutronix.de>
On Wed, Mar 18, 2026 at 12:15:07PM +0100, Sebastian Andrzej Siewior wrote:
> On 2026-03-15 23:21:44 [+0100], Günther Noack wrote:
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> …
> > @@ -1557,6 +1560,110 @@ static int hook_path_truncate(const struct path *const path)
> …
> > +static int hook_unix_find(const struct path *const path, struct sock *other,
> > + int flags)
> > +{
> …
> > + /* Checks the layers in which we are connecting within the same domain. */
> > + unix_state_lock(other);
> > + if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket ||
> > + !other->sk_socket->file)) {
> > + unix_state_unlock(other);
> > + return 0;
> > + }
> > + dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
> > + unix_state_unlock(other);
> > +
> > + unmask_scoped_access(subject->domain, dom_other, &layer_masks,
> > + fs_resolve_unix.fs);
>
> This might be obvious but in case it is not: You obtain the domain
> pointer from f_cred->security. Within the unix_state_lock() block the fd
> can not be closed. Once you drop the lock, the fd can be closed. What
> guarantees that the domain/ dom_other point remains valid between
> unix_state_unlock() and after unmask_scoped_access()?
Sebastian,
In short: dom_other is a pointer to a landlock-owned refcounted struct.
There are two cases, one where there is a correspoinding landlock domain
with the other socket, and one where there is not.
We lookup the domain under lock:
dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
The '->domain' part is important.
Thus dom_other isn't a pointer to the actual tasks credentials,
it's a pointer to a landlock controlled/refcounted domain.
This is the function which returns a pointer to the creds.
static inline struct landlock_cred_security *
landlock_cred(const struct cred *cred)
{
return cred->security + landlock_blob_sizes.lbs_cred;
}
If we were storing the landlock_cred_securit, that could potentially
be a problem.
But we copy the domain pointer, which points to a landlock allocated
and controlled object.
If it is a domain, dom_other points to a landlock controlled, refcounted
struct landlock_ruleset object. So even if the f_cred is freed
afterwards, that object is still valid.
Justin
> Is this invoked within a RCU section which would delay put_cred_rcu() or
> is there other magic involved?
>
> Sebastian
^ permalink raw reply
* Re: [PATCH v6 1/9] lsm: Add LSM hook security_unix_find
From: Paul Moore @ 2026-03-18 14:44 UTC (permalink / raw)
To: Mickaël Salaün
Cc: John Johansen, Georgia Garcia, Günther Noack, James Morris,
Serge E . Hallyn, Tingmao Wang, Justin Suess,
linux-security-module, Samasth Norway Ananda, Matthieu Buffet,
Mikhail Ivanov, konstantin.meskhidze, Demi Marie Obenour,
Alyssa Ross, Jann Horn, Tahera Fahimi, Sebastian Andrzej Siewior,
Kuniyuki Iwashima, Simon Horman, netdev, Alexander Viro,
Christian Brauner
In-Reply-To: <20260318.In1aekohyivu@digikod.net>
On Wed, Mar 18, 2026 at 4:48 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Tue, Mar 17, 2026 at 05:34:57PM -0400, Paul Moore wrote:
> > On Mar 15, 2026 =?UTF-8?q?G=C3=BCnther=20Noack?= <gnoack3000@gmail.com> wrote:
> > >
> > > Add a LSM hook security_unix_find.
> > >
> > > This hook is called to check the path of a named unix socket before a
> > > connection is initiated. The peer socket may be inspected as well.
> > >
> > > Why existing hooks are unsuitable:
> > >
> > > Existing socket hooks, security_unix_stream_connect(),
> > > security_unix_may_send(), and security_socket_connect() don't provide
> > > TOCTOU-free / namespace independent access to the paths of sockets.
> > >
> > > (1) We cannot resolve the path from the struct sockaddr in existing hooks.
> > > This requires another path lookup. A change in the path between the
> > > two lookups will cause a TOCTOU bug.
> > >
> > > (2) We cannot use the struct path from the listening socket, because it
> > > may be bound to a path in a different namespace than the caller,
> > > resulting in a path that cannot be referenced at policy creation time.
> > >
> > > Cc: Günther Noack <gnoack3000@gmail.com>
> > > Cc: Tingmao Wang <m@maowtm.org>
> > > Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> > > ---
> > > include/linux/lsm_hook_defs.h | 5 +++++
> > > include/linux/security.h | 11 +++++++++++
> > > net/unix/af_unix.c | 13 ++++++++++---
> > > security/security.c | 20 ++++++++++++++++++++
> > > 4 files changed, 46 insertions(+), 3 deletions(-)
> >
> > Some really minor nitpicky things (below), but nothing critical.
> > However, as we discussed, I would like to see the AppArmor folks comment
> > on the new hook before we merge anything as I know they have an interest
> > here.
>
> John, Georgia, we've been discussing this new hook for a few months now
> but didn't hear from you yet. We plan to merge this patch series with
> the 7.1 merge window (in a few weeks), so before that I'd like to merge
> it in -next in a few days to get a broader coverage. I'm pretty sure
> this hook will work well with AppArmor too, but could you please take
> look to confirm?
I probably wasn't as clear as I should have been, my apologies. The
major reason I held back my ACK on this patch was that I wanted to
hear from the AA folks regarding the hook's suitability for their
needs. While I don't expect they will have an issue with this hook
as-is, they have expressed interest in the hook, and I would just
assume make sure it is okay for everyone before we send it to Linus.
Since this is a feature addition and not a critical bug fix, I will be
quite upset if this is sent to Linus without review by the AA
developers and my ACK.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path
From: Sebastian Andrzej Siewior @ 2026-03-18 15:05 UTC (permalink / raw)
To: Justin Suess
Cc: Günther Noack, Mickaël Salaün, John Johansen,
Tingmao Wang, Kuniyuki Iwashima, Jann Horn, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross,
Tahera Fahimi
In-Reply-To: <abqzXO3MmMXKqXOy@suesslenovo>
On 2026-03-18 10:14:52 [-0400], Justin Suess wrote:
> Sebastian,
Justin,
> In short: dom_other is a pointer to a landlock-owned refcounted struct.
…
>
> But we copy the domain pointer, which points to a landlock allocated
> and controlled object.
and this is not going away while we are here and preempted after
dropping the lock? (if the landlock policy is updated/ changed/ …)
>
> Justin
Sebastian
^ permalink raw reply
* Re: [PATCH v6 1/9] lsm: Add LSM hook security_unix_find
From: Mickaël Salaün @ 2026-03-18 16:22 UTC (permalink / raw)
To: Paul Moore
Cc: John Johansen, Georgia Garcia, Günther Noack, James Morris,
Serge E . Hallyn, Tingmao Wang, Justin Suess,
linux-security-module, Samasth Norway Ananda, Matthieu Buffet,
Mikhail Ivanov, konstantin.meskhidze, Demi Marie Obenour,
Alyssa Ross, Jann Horn, Tahera Fahimi, Sebastian Andrzej Siewior,
Kuniyuki Iwashima, Simon Horman, netdev, Alexander Viro,
Christian Brauner
In-Reply-To: <CAHC9VhRy2j3nYHHnyjsDf9Rk6=5DAzCSrmXgHCpo4arVnfBjRw@mail.gmail.com>
On Wed, Mar 18, 2026 at 10:44:26AM -0400, Paul Moore wrote:
> On Wed, Mar 18, 2026 at 4:48 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > On Tue, Mar 17, 2026 at 05:34:57PM -0400, Paul Moore wrote:
> > > On Mar 15, 2026 =?UTF-8?q?G=C3=BCnther=20Noack?= <gnoack3000@gmail.com> wrote:
> > > >
> > > > Add a LSM hook security_unix_find.
> > > >
> > > > This hook is called to check the path of a named unix socket before a
> > > > connection is initiated. The peer socket may be inspected as well.
> > > >
> > > > Why existing hooks are unsuitable:
> > > >
> > > > Existing socket hooks, security_unix_stream_connect(),
> > > > security_unix_may_send(), and security_socket_connect() don't provide
> > > > TOCTOU-free / namespace independent access to the paths of sockets.
> > > >
> > > > (1) We cannot resolve the path from the struct sockaddr in existing hooks.
> > > > This requires another path lookup. A change in the path between the
> > > > two lookups will cause a TOCTOU bug.
> > > >
> > > > (2) We cannot use the struct path from the listening socket, because it
> > > > may be bound to a path in a different namespace than the caller,
> > > > resulting in a path that cannot be referenced at policy creation time.
> > > >
> > > > Cc: Günther Noack <gnoack3000@gmail.com>
> > > > Cc: Tingmao Wang <m@maowtm.org>
> > > > Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> > > > ---
> > > > include/linux/lsm_hook_defs.h | 5 +++++
> > > > include/linux/security.h | 11 +++++++++++
> > > > net/unix/af_unix.c | 13 ++++++++++---
> > > > security/security.c | 20 ++++++++++++++++++++
> > > > 4 files changed, 46 insertions(+), 3 deletions(-)
> > >
> > > Some really minor nitpicky things (below), but nothing critical.
> > > However, as we discussed, I would like to see the AppArmor folks comment
> > > on the new hook before we merge anything as I know they have an interest
> > > here.
> >
> > John, Georgia, we've been discussing this new hook for a few months now
> > but didn't hear from you yet. We plan to merge this patch series with
> > the 7.1 merge window (in a few weeks), so before that I'd like to merge
> > it in -next in a few days to get a broader coverage. I'm pretty sure
> > this hook will work well with AppArmor too, but could you please take
> > look to confirm?
>
> I probably wasn't as clear as I should have been, my apologies. The
> major reason I held back my ACK on this patch was that I wanted to
> hear from the AA folks regarding the hook's suitability for their
> needs. While I don't expect they will have an issue with this hook
> as-is, they have expressed interest in the hook, and I would just
> assume make sure it is okay for everyone before we send it to Linus.
>
> Since this is a feature addition and not a critical bug fix, I will be
> quite upset if this is sent to Linus without review by the AA
> developers and my ACK.
I definitely understand and that makes sense, but even if it is not a
strict fix, please consider that this feature still fixes an important
gap in practice (e.g. run anything outside a sandbox with the help of
systemd; see cover letter and reported issues [1]). I don't want to
bypass anyone, but given the importance of this change, I don't want to
postpone it either (except if there is a major issue of course, but I
think the review was pretty good). That's why we'd like to get some
feedback sooner than later. While waiting for AA folks feedback, would
you still be OK for me to push it to -next to improve test and review
coverage?
[1] https://lore.kernel.org/all/cover.1767115163.git.m@maowtm.org/
^ permalink raw reply
* Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path
From: Mickaël Salaün @ 2026-03-18 16:26 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Justin Suess, Günther Noack, John Johansen, Tingmao Wang,
Kuniyuki Iwashima, Jann Horn, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross,
Tahera Fahimi
In-Reply-To: <20260318150559.j04YNDtV@linutronix.de>
On Wed, Mar 18, 2026 at 04:05:59PM +0100, Sebastian Andrzej Siewior wrote:
> On 2026-03-18 10:14:52 [-0400], Justin Suess wrote:
> > Sebastian,
> Justin,
>
> > In short: dom_other is a pointer to a landlock-owned refcounted struct.
> …
> >
> > But we copy the domain pointer, which points to a landlock allocated
> > and controlled object.
>
> and this is not going away while we are here and preempted after
> dropping the lock? (if the landlock policy is updated/ changed/ …)
I agree with Sebastian, this is a bug, see my original proposal:
https://lore.kernel.org/all/20260217.lievaS8eeng8@digikod.net/
^ permalink raw reply
* Re: [PATCH] lsm: export security_mmap_backing_file
From: Paul Moore @ 2026-03-18 16:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: James Morris, Serge E. Hallyn, Amir Goldstein, Arnd Bergmann,
Casey Schaufler, John Johansen, Christian Brauner, Mimi Zohar,
linux-security-module, linux-kernel
In-Reply-To: <20260318104322.1643085-1-arnd@kernel.org>
On Wed, Mar 18, 2026 at 6:43 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> This symbol can now be used from a loadable erofs module, which causes
> a build failure when it is not exported:
>
> ERROR: modpost: "security_mmap_backing_file" [fs/erofs/erofs.ko] undefined!
>
> Fixes: 0e2baaf8fed0 ("lsm: add the security_mmap_backing_file() hook")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> security/security.c | 1 +
> 1 file changed, 1 insertion(+)
Thanks Arnd. It looks like I'm going to need to respin this patchset
due to other changes, do you mind if I fold this patch into the
associated LSM patch when I do that, or would you prefer this as a
standalone fix? No worries either way, fixes are always appreciated
:)
> diff --git a/security/security.c b/security/security.c
> index 8d10b184ce25..3e426a762864 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2529,6 +2529,7 @@ int security_mmap_backing_file(struct vm_area_struct *vma,
>
> return call_int_hook(mmap_backing_file, vma, backing_file, user_file);
> }
> +EXPORT_SYMBOL_GPL(security_mmap_backing_file);
>
> /**
> * security_mmap_addr() - Check if mmap'ing an address is allowed
> --
> 2.39.5
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v6 1/9] lsm: Add LSM hook security_unix_find
From: Paul Moore @ 2026-03-18 16:43 UTC (permalink / raw)
To: Mickaël Salaün
Cc: John Johansen, Georgia Garcia, Günther Noack, James Morris,
Serge E . Hallyn, Tingmao Wang, Justin Suess,
linux-security-module, Samasth Norway Ananda, Matthieu Buffet,
Mikhail Ivanov, konstantin.meskhidze, Demi Marie Obenour,
Alyssa Ross, Jann Horn, Tahera Fahimi, Sebastian Andrzej Siewior,
Kuniyuki Iwashima, Simon Horman, netdev, Alexander Viro,
Christian Brauner
In-Reply-To: <20260318.wa0eef4tei6I@digikod.net>
On Wed, Mar 18, 2026 at 12:22 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Wed, Mar 18, 2026 at 10:44:26AM -0400, Paul Moore wrote:
> > On Wed, Mar 18, 2026 at 4:48 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > On Tue, Mar 17, 2026 at 05:34:57PM -0400, Paul Moore wrote:
> > > > On Mar 15, 2026 =?UTF-8?q?G=C3=BCnther=20Noack?= <gnoack3000@gmail.com> wrote:
> > > > >
> > > > > Add a LSM hook security_unix_find.
> > > > >
> > > > > This hook is called to check the path of a named unix socket before a
> > > > > connection is initiated. The peer socket may be inspected as well.
> > > > >
> > > > > Why existing hooks are unsuitable:
> > > > >
> > > > > Existing socket hooks, security_unix_stream_connect(),
> > > > > security_unix_may_send(), and security_socket_connect() don't provide
> > > > > TOCTOU-free / namespace independent access to the paths of sockets.
> > > > >
> > > > > (1) We cannot resolve the path from the struct sockaddr in existing hooks.
> > > > > This requires another path lookup. A change in the path between the
> > > > > two lookups will cause a TOCTOU bug.
> > > > >
> > > > > (2) We cannot use the struct path from the listening socket, because it
> > > > > may be bound to a path in a different namespace than the caller,
> > > > > resulting in a path that cannot be referenced at policy creation time.
> > > > >
> > > > > Cc: Günther Noack <gnoack3000@gmail.com>
> > > > > Cc: Tingmao Wang <m@maowtm.org>
> > > > > Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> > > > > ---
> > > > > include/linux/lsm_hook_defs.h | 5 +++++
> > > > > include/linux/security.h | 11 +++++++++++
> > > > > net/unix/af_unix.c | 13 ++++++++++---
> > > > > security/security.c | 20 ++++++++++++++++++++
> > > > > 4 files changed, 46 insertions(+), 3 deletions(-)
> > > >
> > > > Some really minor nitpicky things (below), but nothing critical.
> > > > However, as we discussed, I would like to see the AppArmor folks comment
> > > > on the new hook before we merge anything as I know they have an interest
> > > > here.
> > >
> > > John, Georgia, we've been discussing this new hook for a few months now
> > > but didn't hear from you yet. We plan to merge this patch series with
> > > the 7.1 merge window (in a few weeks), so before that I'd like to merge
> > > it in -next in a few days to get a broader coverage. I'm pretty sure
> > > this hook will work well with AppArmor too, but could you please take
> > > look to confirm?
> >
> > I probably wasn't as clear as I should have been, my apologies. The
> > major reason I held back my ACK on this patch was that I wanted to
> > hear from the AA folks regarding the hook's suitability for their
> > needs. While I don't expect they will have an issue with this hook
> > as-is, they have expressed interest in the hook, and I would just
> > assume make sure it is okay for everyone before we send it to Linus.
> >
> > Since this is a feature addition and not a critical bug fix, I will be
> > quite upset if this is sent to Linus without review by the AA
> > developers and my ACK.
>
> I definitely understand and that makes sense, but even if it is not a
> strict fix, please consider that this feature still fixes an important
> gap in practice (e.g. run anything outside a sandbox with the help of
> systemd; see cover letter and reported issues [1]). I don't want to
> bypass anyone, but given the importance of this change, I don't want to
> postpone it either (except if there is a major issue of course, but I
> think the review was pretty good).
I understand the desire to close the gap, but as this is a known
limitation of the existing Landlock implementation, I don't think this
is a MUST (in the RFC sense) for the next merge window. It's
definitely a want, and a *very* nice to have, but if they AA folks
need some more time to sort things out after what has happened
recently I think we can afford them that in this case.
> That's why we'd like to get some
> feedback sooner than later. While waiting for AA folks feedback, would
> you still be OK for me to push it to -next to improve test and review
> coverage?
That's fine, my comment about "sent to Linus" was deliberate. Feel
free to do what is appropriate to gather additional testing.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path
From: Justin Suess @ 2026-03-18 16:43 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Sebastian Andrzej Siewior, Günther Noack, John Johansen,
Tingmao Wang, Kuniyuki Iwashima, Jann Horn, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross,
Tahera Fahimi
In-Reply-To: <20260318.Jeph6loh9uSa@digikod.net>
On Wed, Mar 18, 2026 at 05:26:20PM +0100, Mickaël Salaün wrote:
> On Wed, Mar 18, 2026 at 04:05:59PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2026-03-18 10:14:52 [-0400], Justin Suess wrote:
> > > Sebastian,
> > Justin,
> >
> > > In short: dom_other is a pointer to a landlock-owned refcounted struct.
> > …
> > >
> > > But we copy the domain pointer, which points to a landlock allocated
> > > and controlled object.
> >
> > and this is not going away while we are here and preempted after
> > dropping the lock? (if the landlock policy is updated/ changed/ …)
>
> I agree with Sebastian, this is a bug, see my original proposal:
> https://lore.kernel.org/all/20260217.lievaS8eeng8@digikod.net/
Mickaël,
Just to make sure we're speaking of the same thing (I spotted a bug
shortly after replying to Sebastian).
This is a potential UAF if the dom_other is freed before the access
check takes place correct?
dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
unix_state_unlock(other);
unmask_scoped_access(subject->domain, dom_other, &layer_masks,
fs_resolve_unix.fs);
If the dom_other->usage reaches zero, then the domain could be
freed after the unix_state_unlock while we're checking access??
(I guess I assumed the sock_hold on the @other would prevent the task
@other belongs to from being freed.)
Would it be better to move the access check under the unix_state_lock or
to acquire another reference to the ruleset (or something else)?
(Good catch Sebastian sorry for the confusion.)
Justin
^ permalink raw reply
* Re: [PATCH v6 1/9] lsm: Add LSM hook security_unix_find
From: Mickaël Salaün @ 2026-03-18 16:51 UTC (permalink / raw)
To: Günther Noack
Cc: John Johansen, Paul Moore, James Morris, Serge E . Hallyn,
Tingmao Wang, Justin Suess, linux-security-module,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Demi Marie Obenour, Alyssa Ross, Jann Horn,
Tahera Fahimi, Sebastian Andrzej Siewior, Kuniyuki Iwashima,
Simon Horman, netdev, Alexander Viro, Christian Brauner
In-Reply-To: <20260315222150.121952-2-gnoack3000@gmail.com>
On Sun, Mar 15, 2026 at 11:21:42PM +0100, Günther Noack wrote:
> From: Justin Suess <utilityemal77@gmail.com>
>
> Add a LSM hook security_unix_find.
Add an LSM hook...
>
> This hook is called to check the path of a named unix socket before a
UNIX socket
> connection is initiated. The peer socket may be inspected as well.
>
> Why existing hooks are unsuitable:
>
> Existing socket hooks, security_unix_stream_connect(),
> security_unix_may_send(), and security_socket_connect() don't provide
> TOCTOU-free / namespace independent access to the paths of sockets.
>
> (1) We cannot resolve the path from the struct sockaddr in existing hooks.
> This requires another path lookup. A change in the path between the
> two lookups will cause a TOCTOU bug.
>
> (2) We cannot use the struct path from the listening socket, because it
> may be bound to a path in a different namespace than the caller,
> resulting in a path that cannot be referenced at policy creation time.
>
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Tingmao Wang <m@maowtm.org>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> ---
> include/linux/lsm_hook_defs.h | 5 +++++
> include/linux/security.h | 11 +++++++++++
> net/unix/af_unix.c | 13 ++++++++++---
> security/security.c | 20 ++++++++++++++++++++
> 4 files changed, 46 insertions(+), 3 deletions(-)
^ 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