* Re: [PATCH 2/2] security: Fix call security_backing_file_free second time
From: Paul Moore @ 2026-07-02 21:09 UTC (permalink / raw)
To: Cai Xinchen, jmorris, serge, stephen.smalley.work, omosnace,
amir73il, brauner
Cc: linux-security-module, linux-kernel, selinux, caixinchen1,
lujialin4
In-Reply-To: <20260626011720.1144213-3-caixinchen1@huawei.com>
On Jun 25, 2026 Cai Xinchen <caixinchen1@huawei.com> wrote:
>
> I found the following path:
>
> alloc_empty_backing-file
> init_file(&ff->file, xxx)
> -> file_ref_init(&f->f_ref, 1); // only 1
> error = init_backing_file
> -> security_backing_file_alloc
> -> rc = call_int_hook(backing_file_alloc, ...)
The good news is that as you mentioned, only SELinux defines a
backing_file_alloc hook and it always returns success/0.
> if (unlikely(rc))
> security_backing_file_free(backing_file); // first call
> if (unlikely(error)) {
> fput(&ff->file);
> -> if (unlikely(file_ref_put(&file->f_ref))) // zero
> __fput_deferred(file);
> -> ____fput -> __fput -> file_free(file);
> -> backing_file_free(backing_file(f));
> -> security_backing_file_free(&ff->file); // second call
>
> Currently, only SELinux has the lsm backing_file_alloc hook, and the
> backing_file_free hook is not set. When security_backing_file_free is
> called for the first time, the blobs pointer is set to NULL. Therefore,
> double free will not occur in the code.
>
> Fixes: 6af36aeb147a ("lsm: add backing_file LSM hooks")
> Signed-off-by: Cai Xinchen <caixinchen1@huawei.com>
> ---
> security/security.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 71aea8fdf014..595d3c73253e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2468,11 +2468,8 @@ int security_backing_file_alloc(struct file *backing_file,
> rc = lsm_backing_file_alloc(backing_file);
> if (rc)
> return rc;
> - rc = call_int_hook(backing_file_alloc, backing_file, user_file);
> - if (unlikely(rc))
> - security_backing_file_free(backing_file);
>
> - return rc;
> + return call_int_hook(backing_file_alloc, backing_file, user_file);
> }
I think the better option would be to move the
call_void_hook(backing_file_free, ...) call in security_backing_file_free()
into the if-statment true block before we set the backing file's LSM blob
pointer to NULL and free the LSM blob.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v4 0/2] Delete task_euid()
From: Paul Moore @ 2026-07-02 21:12 UTC (permalink / raw)
To: Alice Ryhl
Cc: Serge Hallyn, Jonathan Corbet, Greg Kroah-Hartman, Shuah Khan,
Alex Shi, Yanteng Si, Dongliang Mu, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Jann Horn, linux-security-module,
linux-doc, linux-kernel, rust-for-linux
In-Reply-To: <CAH5fLghJaFXheAZqKyM9Cdo6iqTw1W=haA79POhmZWEfuKujRA@mail.gmail.com>
On Wed, Jun 3, 2026 at 1:05 PM Alice Ryhl <aliceryhl@google.com> wrote:
> On Wed, Jun 3, 2026 at 6:05 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Tue, Jun 2, 2026 at 2:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > On Mon, Jun 01, 2026 at 07:13:37PM -0400, Paul Moore wrote:
> > > > On Fri, May 29, 2026 at 5:33 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > >
> > > > > The task_euid() method is a very weird method, and Binder was the only
> > > > > user. As of commit 65b672152289 ("binder: use current_euid() for
> > > > > transaction sender identity") Binder doesn't use task_euid() anymore,
> > > > > so we can delete this method.
> > > >
> > > > Given the problems from last time, it seems like it might be prudent
> > > > to let the commit have some time to "breathe" in a proper release, I'd
> > > > suggest merging this not for the upcoming v7.2 merge window but
> > > > instead waiting for v7.3.
> > >
> > > Sure, that makes sense. I'll resend after the merge window.
> >
> > No need to resend if there are no changes (see below), it's in
> > patchwork and I'm tracking it so you're all set. I'll send another
> > notice when I merge it.
> >
> > > > > My suggestion would be to merge this through the LSM tree.
> > > >
> > > > That's fine with me. I'd also suggest updating the commit description
> > > > in patch 1/2 to indicate that binder is no longer using task_euid();
> > > > it currently reads like it is still being used.
> > >
> > > I guess this occurred because when patch 1 was written, it really *was*
> > > still being used.
> >
> > Yeah, I understand the world has changed since patch 1/2 was written,
> > which is okay, we just need to update the commit description ... which
> > should be a trivial task.
> >
> > > Perhaps we could pick up only patch 1 now since even
> > > if we run into problems and Binder has to go back to using task_euid(),
> > > clarifying the docs is still useful.
> >
> > I assumed that was one of the reasons for splitting the changes across
> > two patches (reverting patch 2/2 leaves patch 1/2 intact).
> > Regardless, we're at -rc6 and with patch 1/2 being purely a comment
> > update I don't see an urgent rush on this, especially considering that
> > if I did pick it up now, it would be for the v7.2 merge window and the
> > binder/current_euid() change will ship in v7.1.
> >
> > Let's update the commit description - you've got a couple of weeks to
> > do that - and then we'll merge everything once the v7.2 merge window
> > closes.
>
> Sounds good, thanks!
Just wanted to check in and see if this is still on your todo list?
No rush, we are only at -rc1, but I was reminded of this while merging
other patches and just wanted to check ;)
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] lsm: cleanup repeated lsm_blob_size_update() calls in lsm_prepare()
From: Paul Moore @ 2026-07-02 21:15 UTC (permalink / raw)
To: Matt Bobrowski; +Cc: linux-security-module, James Morris, Serge E . Hallyn
In-Reply-To: <akbHgKxaNmRQEfqs@google.com>
On Thu, Jul 2, 2026 at 4:18 PM Matt Bobrowski <mattbobrowski@google.com> wrote:
> On Thu, Jul 02, 2026 at 03:15:02PM -0400, Paul Moore wrote:
> > On Jun 29, 2026 Matt Bobrowski <mattbobrowski@google.com> wrote:
> > >
> > > Centralize the definition of LSM security blob fields using an X-macro
> > > (LSM_BLOBS_LIST). This reduces repetitive boilerplate code across
> > > struct lsm_blob_sizes, blob size registration in lsm_prepare(), and
> > > debug log printing in security_init().
> > >
> > > Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> > > ---
> > > include/linux/lsm_hooks.h | 42 ++++++++++++++++------------
> > > security/lsm_init.c | 59 ++++++++++-----------------------------
> > > 2 files changed, 38 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index b4f8cad53ddb..0e73b22bdeea 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -98,28 +98,34 @@ struct security_hook_list {
> > > const struct lsm_id *lsmid;
> > > } __randomize_layout;
> > >
> > > +#define LSM_BLOBS_LIST(X) \
> > > + X(cred) \
> > > + X(file) \
> > > + X(backing_file) \
> > > + X(ib) \
> > > + X(inode) \
> > > + X(sock) \
> > > + X(superblock) \
> > > + X(ipc) \
> > > + X(key) \
> > > + X(msg_msg) \
> > > + X(perf_event) \
> > > + X(task) \
> > > + X(tun_dev) \
> > > + X(xattr_count) \
> > > + X(bdev) \
> > > + X(bpf_map) \
> > > + X(bpf_prog) \
> > > + X(bpf_token)
> > > +
> > > /*
> > > * Security blob size or offset data.
> > > + * Note: lbs_xattr_count is the number of xattr slots in new_xattrs array.
> > > */
> > > struct lsm_blob_sizes {
> > > - unsigned int lbs_cred;
> > > - unsigned int lbs_file;
> > > - unsigned int lbs_backing_file;
> > > - unsigned int lbs_ib;
> > > - unsigned int lbs_inode;
> > > - unsigned int lbs_sock;
> > > - unsigned int lbs_superblock;
> > > - unsigned int lbs_ipc;
> > > - unsigned int lbs_key;
> > > - unsigned int lbs_msg_msg;
> > > - unsigned int lbs_perf_event;
> > > - unsigned int lbs_task;
> > > - unsigned int lbs_xattr_count; /* num xattr slots in new_xattrs array */
> > > - unsigned int lbs_tun_dev;
> > > - unsigned int lbs_bdev;
> > > - unsigned int lbs_bpf_map;
> > > - unsigned int lbs_bpf_prog;
> > > - unsigned int lbs_bpf_token;
> > > +#define LSM_BLOB_SIZE(name) unsigned int lbs_##name;
> > > + LSM_BLOBS_LIST(LSM_BLOB_SIZE);
> > > +#undef LSM_BLOB_SIZE
> > > };
> >
> > Generally speaking I greatly prefer to see the structure fields typed out
> > as it makes it easier to find them using grep, code indexers, etc. There
> > is a similar argument for the security_init() changes (below).
> >
> > > diff --git a/security/lsm_init.c b/security/lsm_init.c
> > > index 7c0fd17f1601..c256f1c33efa 100644
> > > --- a/security/lsm_init.c
> > > +++ b/security/lsm_init.c
> > > @@ -282,40 +282,24 @@ static void __init lsm_blob_size_update(unsigned int *sz_req,
> > > * lsm_prepare - Prepare the LSM framework for a new LSM
> > > * @lsm: LSM definition
> > > */
> > > -static void __init lsm_prepare(struct lsm_info *lsm)
> > > +static void __init lsm_prepare(const struct lsm_info *lsm)
> > > {
> > > struct lsm_blob_sizes *blobs = lsm->blobs;
> > >
> > > if (!blobs)
> > > return;
> > >
> > > - /* Register the LSM blob sizes. */
> > > - blobs = lsm->blobs;
> > > - lsm_blob_size_update(&blobs->lbs_cred, &blob_sizes.lbs_cred);
> > > - lsm_blob_size_update(&blobs->lbs_file, &blob_sizes.lbs_file);
> > > - lsm_blob_size_update(&blobs->lbs_backing_file,
> > > - &blob_sizes.lbs_backing_file);
> > > - lsm_blob_size_update(&blobs->lbs_ib, &blob_sizes.lbs_ib);
> > > - /* inode blob gets an rcu_head in addition to LSM blobs. */
> > > + /* The inode blob (inode->i_security) gets an rcu_head in addition to
> > > + * LSM blobs.
> > > + */
> > > if (blobs->lbs_inode && blob_sizes.lbs_inode == 0)
> > > blob_sizes.lbs_inode = sizeof(struct rcu_head);
> > > - lsm_blob_size_update(&blobs->lbs_inode, &blob_sizes.lbs_inode);
> > > - lsm_blob_size_update(&blobs->lbs_ipc, &blob_sizes.lbs_ipc);
> > > - lsm_blob_size_update(&blobs->lbs_key, &blob_sizes.lbs_key);
> > > - lsm_blob_size_update(&blobs->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> > > - lsm_blob_size_update(&blobs->lbs_perf_event,
> > > - &blob_sizes.lbs_perf_event);
> > > - lsm_blob_size_update(&blobs->lbs_sock, &blob_sizes.lbs_sock);
> > > - lsm_blob_size_update(&blobs->lbs_superblock,
> > > - &blob_sizes.lbs_superblock);
> > > - lsm_blob_size_update(&blobs->lbs_task, &blob_sizes.lbs_task);
> > > - lsm_blob_size_update(&blobs->lbs_tun_dev, &blob_sizes.lbs_tun_dev);
> > > - lsm_blob_size_update(&blobs->lbs_xattr_count,
> > > - &blob_sizes.lbs_xattr_count);
> > > - lsm_blob_size_update(&blobs->lbs_bdev, &blob_sizes.lbs_bdev);
> > > - lsm_blob_size_update(&blobs->lbs_bpf_map, &blob_sizes.lbs_bpf_map);
> > > - lsm_blob_size_update(&blobs->lbs_bpf_prog, &blob_sizes.lbs_bpf_prog);
> > > - lsm_blob_size_update(&blobs->lbs_bpf_token, &blob_sizes.lbs_bpf_token);
> > > +
> > > + /* Register the LSM blob sizes. */
> > > +#define UPDATE_LSM_BLOB_SIZE(name) \
> > > + lsm_blob_size_update(&blobs->lbs_##name, &blob_sizes.lbs_##name);
> > > + LSM_BLOBS_LIST(UPDATE_LSM_BLOB_SIZE);
> > > +#undef UPDATE_LSM_BLOB_SIZE
> > > }
> >
> > This is one case where the macro approach *might* have some value, but as
> > it is the one instance, and multiple sequential blob operations like this
> > are going to be *very* limited, I'm not sure there is enough value to
> > be worth the change.
> >
> > I appreciate the initiative to search out areas that you think could use
> > improvement, but for the reasons mentioned, I'm not going to merge this
> > patch.
>
> That's not a problem. Perhaps you may want to consider only taking the
> minor cleanup targets the redundant lsm->blobs access within
> lsm_prepare().
I did think about that, but I ever *so* slightly prefer it the way it
is now, although as I said this is the one case where there may be
some value. If there had been other areas where it was a clear cut
win I likely would have just accepted this change too, but there
wasn't (at least not now, and with this revision) so I just decided to
leave it well enough alone.
However, as I said, thank you taking the initiative on this.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next v3 1/6] bpf: Resolve and cache fd_array objects at load time
From: Daniel Borkmann @ 2026-07-02 21:16 UTC (permalink / raw)
To: Anton Protopopov
Cc: ast, kpsingh, James.Bottomley, paul, bboscaccy, memxor, torvalds,
bpf, linux-security-module
In-Reply-To: <aka2yR6IUZMAOUwJ@mail.gmail.com>
On 7/2/26 9:06 PM, Anton Protopopov wrote:
> On 26/07/02 04:36PM, Daniel Borkmann wrote:
>> The fd_array passed to BPF_PROG_LOAD carries the map and module BTF file
>> descriptors a program binds. The verifier reads it more than once during
>> a load: process_fd_array() walks it to bind the maps and BTFs, and
>> check_and_resolve_insns() and the kfunc BTF resolver later read it again
>> to resolve the program's BPF_PSEUDO_MAP_IDX and module kfunc refs.
>>
>> For signed BPF, we need these upfront in memory, thus resolve each fd to
>> its object once and cache it by fd_array index, then bind that cached
>> object for the rest of the load. env->fd_array becomes a small per-slot
>> {map, btf} cache rather than a bpfptr_t; every later reference is then
>> an in-bounds lookup of an already-resolved object, and an index outside
>> the cache is rejected instead of read from user memory:
>>
>> - continuous (fd_array_cnt given): the caller declares the length and
>> every entry is resolved and bound up front (used also by signed loader)
>>
>> - sparse (no fd_array_cnt): left as the legacy path with no fd_array
>> cache; each reference reads its fd from the caller's fd_array and
>> resolves it on the spot. Deduplication in used_maps and the kfunc BTF
>> table keeps this correct, and only unsigned programs use this shape.
>>
>> UAPI-wise nothing changes, its just the internals on how BPF processes
>> and caches the fd_array favors. Split these into separate helpers for
>> continuous versus sparse to make it easier to follow.
>
> Overall looks ok. A few comments below.
>
[...]
>> - /* Check for integer overflow */
>> - if (attr->fd_array_cnt >= (U32_MAX / size)) {
>> - verbose(env, "fd_array_cnt is too big (%u)\n", attr->fd_array_cnt);
>> - return -EINVAL;
>> + if (cnt > MAX_FD_ARRAY_CNT) {
>
> So, I _think_ I've done the "unlimited" thing because there can be duplicates
> in fd_array. The limit is actually tracked by __add_{map_btf} So here we hard
> limit on the size of the fd_array itself. (Even without duplicates, this
> fd_array can contain, say, MAX_KFUNC_BTFS different maps, which will be in any
> case rejected by __add_used_map.)
>
>> + verbose(env, "fd_array has too many entries (%u, max %u)\n",
>> + cnt, MAX_FD_ARRAY_CNT);
>> + return -E2BIG;
>> }
>>
[...]
>>
>> +static int process_fd_array(struct bpf_verifier_env *env,
>> + union bpf_attr *attr, bpfptr_t uattr)
>> +{
>> + bpfptr_t fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
>> +
>> + if (bpfptr_is_null(fd_array))
>> + return 0;
>> + /*
>> + * New API: the caller passes fd_array_cnt and a continuous array that
>> + * is resolved and bound up front. Legacy API (no fd_array_cnt): keep
>> + * the caller's array and resolve entries lazily, on first reference.
>> + */
>> + if (attr->fd_array_cnt)
>> + return process_fd_array_continuous(env, fd_array,
>> + attr->fd_array_cnt);
>
> Looks like this returns success in case (!fd_array && attr->fd_array_cnt),
> which is a misconfiguration and should be rejected.
Thanks for the review, addressing both in v4!
^ permalink raw reply
* Re: [PATCH bpf-next v3 2/6] bpf: Verify signed loader metadata at load time
From: Paul Moore @ 2026-07-02 22:05 UTC (permalink / raw)
To: Daniel Borkmann
Cc: ast, kpsingh, James.Bottomley, bboscaccy, memxor, torvalds,
a.s.protopopov, bpf, linux-security-module
In-Reply-To: <20260702143605.252797-3-daniel@iogearbox.net>
On Thu, Jul 2, 2026 at 10:36 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> A signed gen_loader program carries the programs, maps and relocations it
> installs in a metadata array map. The loader instructions are covered by
> the PKCS#7 signature, but the metadata map is not: Today the loader
> compares the map contents from within BPF against a hash baked into its
> (signed) instructions, using the kernel-cached map hash. The kernel itself
> never actually attests that the metadata the loader installs is the
> metadata that was signed.
>
> This split is the core of the long-standing objection to the BPF signing
> scheme from the LSM / integrity side: the integrity check of a light
> skeleton only completes once the loader program runs, that is, after the
> security_bpf_prog_load() hook, so at admission time an LSM observes a
> program whose payload has not yet been verified [0]. Auditing the chain
> link is also not a purely cryptographic operation: whoever signs or reviews
> an lskel has to disassemble the loader's preamble to convince themselves
> that the embedded hash check is present and correct [1][2]. Two acceptable
> fixes were identified in those threads: Complete the integrity check
> before the admission hook fires, or add a second hook that collects the
> verification result after the loader ran [3]. Let's implement the former,
> without growing the UAPI.
While you've documented some of the objections from the LSM side,
you've missed the largest objection to the existing BPF signing
scheme: KP's scheme runs the PKCS7 signature over the lskel loader,
while the scheme that Blaise proposed (in multiple forms, not just
Hornet), runs the PKCS7 signature over both the lskel loader and the
associated maps, just as you are doing in this patchset. Moving
towards a scheme where the PKCS7 signature encompasses both the lskel
loader and the maps is a good thing, I'm happy to see that. However,
I do have other issues with this patch that I've mentioned previously
(more below).
> A signed loader binds its metadata map(s) through the existing fd_array,
> and an exclusive map is already bound to a program digest (excl_prog_hash).
> So when a signature is present, collect the exclusive maps from fd_array
> and append their frozen contents to the instructions before verification:
> the signature now covers insns || metadata_0 || metadata_1 || [...] in the
> fd_array order, and verification completes in bpf_check(), once the
> fd_array maps are resolved into used_maps, before the LSM admission hook
> and the rest of verification.
>
> A program is either BPF_SIG_UNSIGNED or BPF_SIG_VERIFIED, with nothing in
> between. While folding the fd_array maps, a non-exclusive map bound to
> a signed program is rejected, so every map folded into the signature is
> exclusive. A signed loader that fails to cover its metadata thus does not
> load, and BPF_SIG_VERIFIED always means the instructions and every
> exclusive map are authentic.
>
> The maps must be frozen so the hashed bytes cannot change before the
> loader runs; the map <-> program digest binding is enforced by the
> verifier for every used map. Binding maps through fd_array_cnt makes the
> verifier resolve and excl-check them (excl_prog_sha vs prog->digest)
> before it would otherwise compute the digest, so compute prog->digest
> up front in bpf_check(), over the unmodified instructions the
> signature covers, for a load that folds metadata.
>
> Unsigned programs are not affected. Note, signed loaders generated by
> older libbpf/bpftool versions need to be regenerated; some of the recent
> fixes we've had on the signed loader side require the latter already to
> close gaps.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/bpf/CAHC9VhSDkwGgPfrBUh7EgBKEJj_JjnY68c0YAmuuLT_i--GskQ@mail.gmail.com [0]
> Link: https://lore.kernel.org/bpf/2f71d6c03698eb17d51f7247efde777627ee578a.camel@HansenPartnership.com [1]
> Link: https://lore.kernel.org/lkml/ecf0521ed302db672672ebfbc670ecfba36a6e00.camel@HansenPartnership.com [2]
> Link: https://lore.kernel.org/bpf/88703f00d5b7a779728451008626efa45e42db3d.camel@HansenPartnership.com [3]
> ---
> include/linux/bpf_verifier.h | 1 +
> kernel/bpf/syscall.c | 76 +--------------
> kernel/bpf/verifier.c | 175 +++++++++++++++++++++++++++++++++++
> 3 files changed, 178 insertions(+), 74 deletions(-)
...
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6db306d23b47..e898fad01aaf 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3189,10 +3121,6 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, struct bpf_log_at
> if (err < 0)
> goto free_prog;
>
> - err = security_bpf_prog_load(prog, attr, token, uattr.is_kernel);
> - if (err)
> - goto free_prog;
> -
> /* run eBPF verifier */
> err = bpf_check(&prog, attr, uattr, attr_log);
> if (err < 0)
As I mentioned previously, the security_bpf_prog_load() hook should
not be moved. Its placement was deliberate, and moving it as you've
done in this patch creates a security regression. Without this patch,
for processes where SELinux is preventing BPF programs from being
loaded, it is able to do so before the process allocates a potentially
very large chunk of kernel memory, up to one million BPF instructions.
With this patch, even in cases where SELinux will prevent the BPF
program load operation, it is not able to do so before the process
triggers a potential sizable memory allocation, opening the door for a
local DoS attack.
In an effort to work with you on this, I did bring up two alternative
solutions: a new LSM hook for the signature verification, or the use
of the existing security_bpf_prog() hook. The latter option is
conceptually less clean than the former, but it should still work to
control access to the BPF program based on the signature results.
You've mentioned that you are not in favor of adding a new hook, but
you have ignored the other potential solution of using the
security_bpf_prog() hook.
You can (re)read my earlier comments at the lore link below:
https://lore.kernel.org/linux-security-module/CAHC9VhTtaT_8AXSfNKiq4cGq7T=6Dh=huM8niBCwOP8kj+ugjQ@mail.gmail.com/
While I don't like to do this, you haven't given me much of a choice;
the security regression can not be ignored:
Nacked-by: Paul Moore (don't move bpf_prog_load LSM hook) <paul@paul-moore.com>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 44bb6ce17a1c..1609053a0eeb 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19901,11 +20060,27 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
> ret = bpf_vlog_init(&env->log, attr_log->level, attr_log->ubuf, attr_log->size);
> if (ret)
> goto err_unlock;
> + if (env->signature) {
> + ret = bpf_prog_calc_tag(env->prog);
> + if (ret < 0)
> + goto skip_full_check;
> + }
>
> ret = process_fd_array(env, attr, uattr);
> if (ret)
> goto skip_full_check;
>
> + if (env->signature) {
> + ret = bpf_prog_verify_signature(env, attr, uattr.is_kernel);
> + if (ret)
> + goto skip_full_check;
> + }
> +
> + ret = security_bpf_prog_load(env->prog, attr, env->prog->aux->token,
> + uattr.is_kernel);
> + if (ret)
> + goto skip_full_check;
> +
> mark_verifier_state_clean(env);
>
> if (IS_ERR(btf_vmlinux)) {
> --
> 2.43.0
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next v3 2/6] bpf: Verify signed loader metadata at load time
From: Alexei Starovoitov @ 2026-07-02 22:33 UTC (permalink / raw)
To: Paul Moore, Daniel Borkmann
Cc: ast, kpsingh, James.Bottomley, bboscaccy, memxor, torvalds,
a.s.protopopov, bpf, linux-security-module
In-Reply-To: <CAHC9VhQD4RHsGiTMVCCTN+eB71-Ueopke8XghExpNJ2PSNa_jQ@mail.gmail.com>
On Thu Jul 2, 2026 at 3:05 PM PDT, Paul Moore wrote:
>
> As I mentioned previously, the security_bpf_prog_load() hook should
> not be moved. Its placement was deliberate, and moving it as you've
> done in this patch creates a security regression.
not true.
> Without this patch,
> for processes where SELinux is preventing BPF programs from being
> loaded, it is able to do so before the process allocates a potentially
> very large chunk of kernel memory, up to one million BPF instructions.
only when signature verification is requested and the hash over insns
is what your hornet thingy did too.
So no regression whatsoever. Exact same behavior.
> With this patch, even in cases where SELinux will prevent the BPF
> program load operation, it is not able to do so before the process
> triggers a potential sizable memory allocation, opening the door for a
> local DoS attack.
Not true either. Worst case is 1M which is 8Mbyte.
Not even close to anything DoS worthy.
> In an effort to work with you on this, I did bring up two alternative
> solutions: a new LSM hook for the signature verification, or the use
> of the existing security_bpf_prog() hook.
Sorry we're not going to add new hook because Paul has
non-technical grudge against bpf.
> While I don't like to do this, you haven't given me much of a choice;
> the security regression can not be ignored:
>
> Nacked-by: Paul Moore (don't move bpf_prog_load LSM hook) <paul@paul-moore.com>
of course. will keep it and let Linus decide during the merge window.
^ permalink raw reply
* [PATCH] selftests/lsm: Fix memory leak in attr_lsm_count
From: Wang Yan @ 2026-07-03 2:52 UTC (permalink / raw)
To: linux-security-module
Cc: paul, jmorris, serge, shuah, casey, mic, linux-kselftest,
linux-kernel, Wang Yan
The calloc-allocated buffer in attr_lsm_count() is never released on
any exit path, including both the normal return path and the early
return when read_sysfs_lsms fails, resulting in a heap memory leak.
Add free() for the buffer on all return branches to fix the leak.
Fixes: d3d929a8b0cd ("LSM: selftests for Linux Security Module syscalls")
Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
---
tools/testing/selftests/lsm/common.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/lsm/common.c b/tools/testing/selftests/lsm/common.c
index 9ad258912646..4fa8310750a0 100644
--- a/tools/testing/selftests/lsm/common.c
+++ b/tools/testing/selftests/lsm/common.c
@@ -76,6 +76,7 @@ int attr_lsm_count(void)
return 0;
if (read_sysfs_lsms(names, sysconf(_SC_PAGESIZE)))
+ free(names);
return 0;
if (strstr(names, "selinux"))
@@ -85,5 +86,6 @@ int attr_lsm_count(void)
if (strstr(names, "apparmor"))
count++;
+ free(names);
return count;
}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] selftests/lsm: Fix memory leak in attr_lsm_count
From: William Roberts @ 2026-07-03 3:01 UTC (permalink / raw)
To: Wang Yan
Cc: linux-security-module, paul, jmorris, serge, shuah, casey, mic,
linux-kselftest, linux-kernel
In-Reply-To: <20260703025247.940055-1-wangyan01@kylinos.cn>
On Thu, Jul 2, 2026 at 9:53 PM Wang Yan <wangyan01@kylinos.cn> wrote:
>
> The calloc-allocated buffer in attr_lsm_count() is never released on
> any exit path, including both the normal return path and the early
> return when read_sysfs_lsms fails, resulting in a heap memory leak.
>
> Add free() for the buffer on all return branches to fix the leak.
>
> Fixes: d3d929a8b0cd ("LSM: selftests for Linux Security Module syscalls")
> Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
> ---
> tools/testing/selftests/lsm/common.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/lsm/common.c b/tools/testing/selftests/lsm/common.c
> index 9ad258912646..4fa8310750a0 100644
> --- a/tools/testing/selftests/lsm/common.c
> +++ b/tools/testing/selftests/lsm/common.c
> @@ -76,6 +76,7 @@ int attr_lsm_count(void)
> return 0;
>
> if (read_sysfs_lsms(names, sysconf(_SC_PAGESIZE)))
> + free(names);
> return 0;
Did you test this, let alone even compile it? All the C code after
this return is dead code.
Please ensure that you test and compile your patches checking for
warnings. Sending
untested patches is "spammy".
>
> if (strstr(names, "selinux"))
> @@ -85,5 +86,6 @@ int attr_lsm_count(void)
> if (strstr(names, "apparmor"))
> count++;
>
> + free(names);
Do this instead:
if (read_sysfs_lsms(names, sysconf(_SC_PAGESIZE)))
goto out;
<existing code>
out:
free(names);
> return count;
> }
> --
> 2.25.1
>
>
^ permalink raw reply
* [PATCH v5 0/2] Delete task_euid()
From: Alice Ryhl @ 2026-07-03 6:57 UTC (permalink / raw)
To: Paul Moore, Serge Hallyn, Jonathan Corbet, Greg Kroah-Hartman,
Shuah Khan, Alex Shi, Yanteng Si, Dongliang Mu
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Jann Horn, linux-security-module, linux-doc, linux-kernel,
rust-for-linux, Alice Ryhl
The task_euid() method is a very weird method, and Binder was the only
user. As of commit 65b672152289 ("binder: use current_euid() for
transaction sender identity") Binder doesn't use task_euid() anymore,
so we can delete this method.
My suggestion would be to merge this through the LSM tree.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v5:
- Rebase on v7.2-rc1.
- Reword patch 1 commit message to take into account that usage has now
been removed from Binder.
- Pick up Gary's Reviewed-by.
- Link to v4: https://lore.kernel.org/r/20260529-remove-task-euid-v4-0-07cbdf3af980@google.com
Changes in v4:
- Reword 'euid' -> 'effective UID' in 'Kuid::current_euid()' docs.
- Link to v3: https://lore.kernel.org/r/20260507-remove-task-euid-v3-0-27f22f335c2c@google.com
Changes in v3:
- Include 'task' clarification commit in series.
- Rebase and resend.
- Link to v2: https://lore.kernel.org/r/20260227-remove-task-euid-v2-1-9a9c80a82eb6@google.com
Changes in v2:
- Update translation as per Alex Shi.
- Pick up Reviewed-by Gary.
- Update commit title to use cred: prefix.
- Link to v1: https://lore.kernel.org/r/20260219-remove-task-euid-v1-1-904060826e07@google.com
---
Alice Ryhl (1):
cred: delete task_euid()
Jann Horn (1):
rust: task: clarify comments on task UID accessors
Documentation/security/credentials.rst | 6 ++----
Documentation/translations/zh_CN/security/credentials.rst | 4 +---
include/linux/cred.h | 1 -
rust/helpers/task.c | 5 -----
rust/kernel/task.rs | 11 ++---------
5 files changed, 5 insertions(+), 22 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260219-remove-task-euid-19e4b00beebe
Best regards,
--
Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply
* [PATCH v5 1/2] rust: task: clarify comments on task UID accessors
From: Alice Ryhl @ 2026-07-03 6:57 UTC (permalink / raw)
To: Paul Moore, Serge Hallyn, Jonathan Corbet, Greg Kroah-Hartman,
Shuah Khan, Alex Shi, Yanteng Si, Dongliang Mu
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Jann Horn, linux-security-module, linux-doc, linux-kernel,
rust-for-linux, Alice Ryhl
In-Reply-To: <20260703-remove-task-euid-v5-0-c90c7e2ddf54@google.com>
From: Jann Horn <jannh@google.com>
Linux has separate subjective and objective task credentials, see the
comment above `struct cred`. Clarify which accessor functions operate on
which set of credentials.
Also document that Task::euid() is a very weird operation. You can see how
weird it is by grepping for task_euid() in the history - binder was its
only user. Task::euid() obtains the objective effective UID - it looks
at the credentials of the task for purposes of acting on it as an
object, but then accesses the effective UID (which the credentials.7 man
page describes as "[...] used by the kernel to determine the permissions
that the process will have when accessing shared resources [...]").
For context:
Arguably, binder's use of task_euid() is a theoretical security problem,
which only has no impact on Android because Android has no setuid binaries
executable by apps.
commit 29bc22ac5e5b ("binder: use euid from cred instead of using task")
originally fixed that by removing that only user of task_euid(), but the
fix got reverted in commit c21a80ca0684 ("binder: fix test regression
due to sender_euid change") because some Android test started failing.
It was since fixed again by commit 65b672152289 ("binder: use
current_euid() for transaction sender identity"), which uses
current_euid() instead.
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Originally sent as:
https://lore.kernel.org/r/20260212-rust-uid-v1-1-deff4214c766@google.com
---
rust/kernel/task.rs | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 38273f4eedb5..eabd65bfde12 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -210,14 +210,17 @@ pub fn pid(&self) -> Pid {
unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
}
- /// Returns the UID of the given task.
+ /// Returns the objective real UID of the given task.
#[inline]
pub fn uid(&self) -> Kuid {
// SAFETY: It's always safe to call `task_uid` on a valid task.
Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
}
- /// Returns the effective UID of the given task.
+ /// Returns the objective effective UID of the given task.
+ ///
+ /// You should probably not be using this; the effective UID is normally
+ /// only relevant in subjective credentials.
#[inline]
pub fn euid(&self) -> Kuid {
// SAFETY: It's always safe to call `task_euid` on a valid task.
@@ -371,7 +374,7 @@ fn eq(&self, other: &Self) -> bool {
impl Eq for Task {}
impl Kuid {
- /// Get the current euid.
+ /// Get the current subjective effective UID.
#[inline]
pub fn current_euid() -> Kuid {
// SAFETY: Just an FFI call.
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v5 2/2] cred: delete task_euid()
From: Alice Ryhl @ 2026-07-03 6:57 UTC (permalink / raw)
To: Paul Moore, Serge Hallyn, Jonathan Corbet, Greg Kroah-Hartman,
Shuah Khan, Alex Shi, Yanteng Si, Dongliang Mu
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Jann Horn, linux-security-module, linux-doc, linux-kernel,
rust-for-linux, Alice Ryhl
In-Reply-To: <20260703-remove-task-euid-v5-0-c90c7e2ddf54@google.com>
task_euid() is a very weird operation. You can see how weird it is by
grepping for task_euid() - binder is its only user. task_euid() obtains
the objective effective UID - it looks at the credentials of the task
for purposes of acting on it as an object, but then accesses the
effective UID (which the credentials.7 man page describes as "[...] used
by the kernel to determine the permissions that the process will have
when accessing shared resources [...]").
Since usage in Binder has now been removed, get rid of the resulting
dead code.
Changes to the zh_CN translation was carried out with the help of
Gemini and Google Translate, and since adjusted as per Alex Shi's
feedback.
Suggested-by: Jann Horn <jannh@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Documentation/security/credentials.rst | 6 ++----
Documentation/translations/zh_CN/security/credentials.rst | 4 +---
include/linux/cred.h | 1 -
rust/helpers/task.c | 5 -----
rust/kernel/task.rs | 10 ----------
5 files changed, 3 insertions(+), 23 deletions(-)
diff --git a/Documentation/security/credentials.rst b/Documentation/security/credentials.rst
index 4996838491b1..a39a2a2f67aa 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -393,16 +393,14 @@ the credentials so obtained when they're finished with.
The result of ``__task_cred()`` should not be passed directly to
``get_cred()`` as this may race with ``commit_cred()``.
-There are a couple of convenience functions to access bits of another task's
-credentials, hiding the RCU magic from the caller::
+There is a convenience function to access bits of another task's credentials,
+hiding the RCU magic from the caller::
uid_t task_uid(task) Task's real UID
- uid_t task_euid(task) Task's effective UID
If the caller is holding the RCU read lock at the time anyway, then::
__task_cred(task)->uid
- __task_cred(task)->euid
should be used instead. Similarly, if multiple aspects of a task's credentials
need to be accessed, RCU read lock should be used, ``__task_cred()`` called,
diff --git a/Documentation/translations/zh_CN/security/credentials.rst b/Documentation/translations/zh_CN/security/credentials.rst
index 88fcd9152ffe..20c8696f8198 100644
--- a/Documentation/translations/zh_CN/security/credentials.rst
+++ b/Documentation/translations/zh_CN/security/credentials.rst
@@ -337,15 +337,13 @@ const指针上操作，因此不需要进行类型转换，但需要临时放弃
``__task_cred()`` 的结果不应直接传递给 ``get_cred()`` ，
因为这可能与 ``commit_cred()`` 发生竞争条件。
-还有一些方便的函数可以访问另一个任务凭据的特定部分，将RCU操作对调用方隐藏起来::
+有一个方便的函数可用于访问另一个任务凭据的特定部分，从而对调用方隐藏RCU机制::
uid_t task_uid(task) Task's real UID
- uid_t task_euid(task) Task's effective UID
如果调用方在此时已经持有RCU读锁，则应使用::
__task_cred(task)->uid
- __task_cred(task)->euid
类似地，如果需要访问任务凭据的多个方面，应使用RCU读锁，调用 ``__task_cred()``
函数，将结果存储在临时指针中，然后从临时指针中调用凭据的各个方面，最后释放锁。
diff --git a/include/linux/cred.h b/include/linux/cred.h
index c6676265a985..6ef1750c93e2 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -371,7 +371,6 @@ DEFINE_FREE(put_cred, struct cred *, if (!IS_ERR_OR_NULL(_T)) put_cred(_T))
})
#define task_uid(task) (task_cred_xxx((task), uid))
-#define task_euid(task) (task_cred_xxx((task), euid))
#define task_ucounts(task) (task_cred_xxx((task), ucounts))
#define current_cred_xxx(xxx) \
diff --git a/rust/helpers/task.c b/rust/helpers/task.c
index c0e1a06ede78..b46b1433a67e 100644
--- a/rust/helpers/task.c
+++ b/rust/helpers/task.c
@@ -28,11 +28,6 @@ __rust_helper kuid_t rust_helper_task_uid(struct task_struct *task)
return task_uid(task);
}
-__rust_helper kuid_t rust_helper_task_euid(struct task_struct *task)
-{
- return task_euid(task);
-}
-
#ifndef CONFIG_USER_NS
__rust_helper uid_t rust_helper_from_kuid(struct user_namespace *to, kuid_t uid)
{
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index eabd65bfde12..c2b3457b700c 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -217,16 +217,6 @@ pub fn uid(&self) -> Kuid {
Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
}
- /// Returns the objective effective UID of the given task.
- ///
- /// You should probably not be using this; the effective UID is normally
- /// only relevant in subjective credentials.
- #[inline]
- pub fn euid(&self) -> Kuid {
- // SAFETY: It's always safe to call `task_euid` on a valid task.
- Kuid::from_raw(unsafe { bindings::task_euid(self.as_ptr()) })
- }
-
/// Determines whether the given task has pending signals.
#[inline]
pub fn signal_pending(&self) -> bool {
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* Re: [PATCH v4 0/2] Delete task_euid()
From: Alice Ryhl @ 2026-07-03 6:57 UTC (permalink / raw)
To: Paul Moore
Cc: Serge Hallyn, Jonathan Corbet, Greg Kroah-Hartman, Shuah Khan,
Alex Shi, Yanteng Si, Dongliang Mu, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Jann Horn, linux-security-module,
linux-doc, linux-kernel, rust-for-linux
In-Reply-To: <CAHC9VhTfOnOgOzGk9==wJYKx5gAi3Zf3oTwbj183o0_xxrfKag@mail.gmail.com>
On Thu, Jul 02, 2026 at 05:12:52PM -0400, Paul Moore wrote:
> On Wed, Jun 3, 2026 at 1:05 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > On Wed, Jun 3, 2026 at 6:05 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Tue, Jun 2, 2026 at 2:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > On Mon, Jun 01, 2026 at 07:13:37PM -0400, Paul Moore wrote:
> > > > > On Fri, May 29, 2026 at 5:33 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > > >
> > > > > > The task_euid() method is a very weird method, and Binder was the only
> > > > > > user. As of commit 65b672152289 ("binder: use current_euid() for
> > > > > > transaction sender identity") Binder doesn't use task_euid() anymore,
> > > > > > so we can delete this method.
> > > > >
> > > > > Given the problems from last time, it seems like it might be prudent
> > > > > to let the commit have some time to "breathe" in a proper release, I'd
> > > > > suggest merging this not for the upcoming v7.2 merge window but
> > > > > instead waiting for v7.3.
> > > >
> > > > Sure, that makes sense. I'll resend after the merge window.
> > >
> > > No need to resend if there are no changes (see below), it's in
> > > patchwork and I'm tracking it so you're all set. I'll send another
> > > notice when I merge it.
> > >
> > > > > > My suggestion would be to merge this through the LSM tree.
> > > > >
> > > > > That's fine with me. I'd also suggest updating the commit description
> > > > > in patch 1/2 to indicate that binder is no longer using task_euid();
> > > > > it currently reads like it is still being used.
> > > >
> > > > I guess this occurred because when patch 1 was written, it really *was*
> > > > still being used.
> > >
> > > Yeah, I understand the world has changed since patch 1/2 was written,
> > > which is okay, we just need to update the commit description ... which
> > > should be a trivial task.
> > >
> > > > Perhaps we could pick up only patch 1 now since even
> > > > if we run into problems and Binder has to go back to using task_euid(),
> > > > clarifying the docs is still useful.
> > >
> > > I assumed that was one of the reasons for splitting the changes across
> > > two patches (reverting patch 2/2 leaves patch 1/2 intact).
> > > Regardless, we're at -rc6 and with patch 1/2 being purely a comment
> > > update I don't see an urgent rush on this, especially considering that
> > > if I did pick it up now, it would be for the v7.2 merge window and the
> > > binder/current_euid() change will ship in v7.1.
> > >
> > > Let's update the commit description - you've got a couple of weeks to
> > > do that - and then we'll merge everything once the v7.2 merge window
> > > closes.
> >
> > Sounds good, thanks!
>
> Just wanted to check in and see if this is still on your todo list?
> No rush, we are only at -rc1, but I was reminded of this while merging
> other patches and just wanted to check ;)
Resent!
Alice
^ permalink raw reply
* [RFC PATCH 0/3] coredump, net: fix layer violation with direct connection
From: John Ericson @ 2026-07-03 7:39 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: John Ericson, Cong Wang, Kuniyuki Iwashima, Simon Horman,
Christian Brauner, David Rheinsberg, Andy Lutomirski,
Sergei Zimmerman, netdev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module,
linux-kernel
From: John Ericson <mail@JohnEricson.me>
In https://lore.kernel.org/all/akWxrjOl4Up02Bvq@pop-os.localdomain/ Cong
Wang asked about doing things without new syscalls for my fd-based
connect idea. This got me investigating a few things, all of which I hope to
submit as patches.
This is the first one. I stumbled on `SOCK_COREDUMP` in `af_unix.c`, and
I realized that this was --- right in the part of the kernel I was
already looking at --- an excellent example of something that directly
connecting to a socket could do better. This is not because the
filesystem would never be involved (the interface in procfs still
specifies a path) but because the core dumper wants to resolve that path
differently than the usual way.
The first two commits are refactors that expose/create the necessary
functionality, and then the last commit actually does the untangling of
the unix socket implementation and the core dumper. See especially that
third commit message for details.
I hope this is a compelling use-case for you all, that does not touch
the UABI yet, but also does just the sort of thing that would be nice to
expose with a new syscall.
John
John Ericson (3):
af_unix: factor out unix_lookup_bsd_path()
af_unix: factor out kernel_unix_connect_direct()
coredump, net: remove `SOCK_COREDUMP`
fs/coredump.c | 47 +++--
include/linux/lsm_hook_defs.h | 3 +-
include/linux/net.h | 1 -
include/linux/security.h | 4 +-
include/net/af_unix.h | 2 +
net/unix/af_unix.c | 335 +++++++++++++++++++++++++---------
security/landlock/fs.c | 7 +-
security/security.c | 5 +-
8 files changed, 287 insertions(+), 117 deletions(-)
--
2.54.0
^ permalink raw reply
* [RFC PATCH 1/3] af_unix: factor out unix_lookup_bsd_path()
From: John Ericson @ 2026-07-03 7:39 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: John Ericson, Cong Wang, Kuniyuki Iwashima, Simon Horman,
Christian Brauner, David Rheinsberg, Andy Lutomirski,
Sergei Zimmerman, netdev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module,
linux-kernel
In-Reply-To: <20260703073948.2541875-1-John.Ericson@Obsidian.Systems>
From: John Ericson <mail@JohnEricson.me>
Split the inode -> sock mapping out of `unix_find_bsd()` into a new
helper, `unix_lookup_bsd_path()`: given an already-resolved `struct
path`, check it is a socket, look the bound socket up by inode, and
check its type, returning a held `struct sock` (or an `ERR_PTR`).
`unix_find_bsd()` keeps doing the path resolution, the `MAY_WRITE`
permission check, the `security_unix_find()` LSM hook and
`touch_atime()`, and calls the helper for the lookup. No functional
change.
The function documentation anticipates (in an example) the way this will
be used later in the patch series.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: John Ericson <mail@JohnEricson.me>
---
include/net/af_unix.h | 1 +
net/unix/af_unix.c | 50 ++++++++++++++++++++++++++++++++-----------
2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 34f53dde65ce..fe4547508af1 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -14,6 +14,7 @@
#if IS_ENABLED(CONFIG_UNIX)
struct unix_sock *unix_get_socket(struct file *filp);
+struct sock *unix_lookup_bsd_path(const struct path *path, int type);
#else
static inline struct unix_sock *unix_get_socket(struct file *filp)
{
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f7a9d55eee8a..3270299238c4 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1185,10 +1185,43 @@ static int unix_release(struct socket *sock)
return 0;
}
+/**
+ * unix_lookup_bsd_path - find the AF_UNIX socket bound at a resolved path
+ * @path: a path the caller has already resolved under its own policy
+ * @type: required socket type (SOCK_STREAM/SOCK_SEQPACKET/SOCK_DGRAM)
+ *
+ * Unlike the connect(2) lookup, this performs no path resolution and no
+ * DAC or LSM check of its own: the caller is responsible for having
+ * resolved @path with whatever policy is appropriate. Used by kernel
+ * callers (e.g. coredump-to-socket) that must resolve the path under
+ * their own root and credentials rather than the current task's.
+ *
+ * Returns a held sock, or an ERR_PTR.
+ */
+struct sock *unix_lookup_bsd_path(const struct path *path, int type)
+{
+ struct inode *inode = d_backing_inode(path->dentry);
+ struct sock *sk;
+
+ if (!S_ISSOCK(inode->i_mode))
+ return ERR_PTR(-ECONNREFUSED);
+
+ sk = unix_find_socket_byinode(inode);
+ if (!sk)
+ return ERR_PTR(-ECONNREFUSED);
+
+ if (sk->sk_type != type) {
+ sock_put(sk);
+ return ERR_PTR(-EPROTOTYPE);
+ }
+
+ return sk;
+}
+EXPORT_SYMBOL_GPL(unix_lookup_bsd_path);
+
static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
int type, int flags)
{
- struct inode *inode;
struct path path;
struct sock *sk;
int err;
@@ -1219,18 +1252,11 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
goto path_put;
}
- err = -ECONNREFUSED;
- inode = d_backing_inode(path.dentry);
- if (!S_ISSOCK(inode->i_mode))
+ sk = unix_lookup_bsd_path(&path, type);
+ if (IS_ERR(sk)) {
+ err = PTR_ERR(sk);
goto path_put;
-
- sk = unix_find_socket_byinode(inode);
- if (!sk)
- goto path_put;
-
- err = -EPROTOTYPE;
- if (sk->sk_type != type)
- goto sock_put;
+ }
err = security_unix_find(&path, sk, flags);
if (err)
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 2/3] af_unix: factor out kernel_unix_connect_direct()
From: John Ericson @ 2026-07-03 7:39 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: John Ericson, Cong Wang, Kuniyuki Iwashima, Simon Horman,
Christian Brauner, David Rheinsberg, Andy Lutomirski,
Sergei Zimmerman, netdev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module,
linux-kernel
In-Reply-To: <20260703073948.2541875-1-John.Ericson@Obsidian.Systems>
From: John Ericson <mail@JohnEricson.me>
I was hoping this was going to be a simple matter of factoring out the
back half of `unix_stream_connect`. No such luck was had, because
actually instead of `unix_stream_connect` looking up the socket from the
VFS once, it does it repeatedly in the same loop that is used to deal
with full listening queues.
(This behavior is rather surprising to me, because it would allow a
deleted and recreated socket to be picked up on the next loop iteration.
But, I don't want to make any UAPI-visible changes in this patch series,
so I did not consider changing it.)
Seeing that this was going to be more complex, I instead factored out
three helpers (setup, commit, cleanup) on a state struct, so I could
reuse them both in the existing `unix_stream_connect` and also in the
new `kernel_unix_connect_direct`. This allows each caller to implement a
slightly different loop:
- resource management of `struct sock *other`:
- `unix_stream_connect` acquires (and releases) it.
- `kernel_unix_connect_direct` uses the caller-provided one.
- stale `other` behavior:
- `unix_stream_connect` retries, because on the next iteration the
socket may have been replaced by a fresh one.
- `kernel_unix_connect_direct` fails, because no reacquisition means
staleness is permanent.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: John Ericson <mail@JohnEricson.me>
---
include/net/af_unix.h | 1 +
net/unix/af_unix.c | 247 +++++++++++++++++++++++++++++++++---------
2 files changed, 199 insertions(+), 49 deletions(-)
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index fe4547508af1..7d810321efa3 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -15,6 +15,7 @@
#if IS_ENABLED(CONFIG_UNIX)
struct unix_sock *unix_get_socket(struct file *filp);
struct sock *unix_lookup_bsd_path(const struct path *path, int type);
+int kernel_unix_connect_direct(struct sock *other, struct socket *sock, int flags);
#else
static inline struct unix_sock *unix_get_socket(struct file *filp)
{
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 3270299238c4..aa94da1f8c24 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1649,44 +1649,60 @@ static long unix_wait_for_peer(struct sock *other, long timeo)
return timeo;
}
-static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uaddr,
- int addr_len, int flags)
-{
- struct sockaddr_un *sunaddr = (struct sockaddr_un *)uaddr;
- struct sock *sk = sock->sk, *newsk = NULL, *other = NULL;
- struct unix_sock *u = unix_sk(sk), *newu, *otheru;
- struct unix_peercred peercred = {};
- struct net *net = sock_net(sk);
- struct sk_buff *skb = NULL;
- unsigned char state;
- long timeo;
- int err;
+/*
+ * The state a stream connect() builds up before it has a peer: the new
+ * sock and the connection-request skb handed to the listener, the
+ * connecting side's credentials and its send timeout.
+ *
+ * - Built once by unix_stream_connect_setup()
+ * - Used to finish connecting by unix_stream_connect_commit()
+ * - Cleaned up in the failure case by unix_stream_connect_cleanup()
+ */
+struct unix_connect_state {
+ struct sock *newsk;
+ struct sk_buff *skb;
+ struct unix_peercred peercred;
+ long timeo;
+};
- err = unix_validate_addr(sunaddr, addr_len);
- if (err)
- goto out;
+/* Free a connect state that no connection consumed (i.e. on failure). */
+static void unix_stream_connect_cleanup(struct unix_connect_state *st)
+{
+ consume_skb(st->skb);
+ unix_release_sock(st->newsk, 0);
+ drop_peercred(&st->peercred);
+}
- err = BPF_CGROUP_RUN_PROG_UNIX_CONNECT_LOCK(sk, uaddr, &addr_len);
- if (err)
- goto out;
+/*
+ * Build the state a stream connect needs before it looks for a peer:
+ * autobind if required, snapshot the send timeout, and allocate the new
+ * sock, the request skb and the peer credentials. On failure nothing is
+ * left allocated in @st.
+ */
+static int unix_stream_connect_setup(struct socket *sock, int flags,
+ struct unix_connect_state *st)
+{
+ struct sock *sk = sock->sk, *newsk;
+ struct sk_buff *skb;
+ int err;
- if (unix_may_passcred(sk) && !READ_ONCE(u->addr)) {
+ if (unix_may_passcred(sk) && !READ_ONCE(unix_sk(sk)->addr)) {
err = unix_autobind(sk);
if (err)
- goto out;
+ return err;
}
- timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+ st->timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
- err = prepare_peercred(&peercred);
+ err = prepare_peercred(&st->peercred);
if (err)
- goto out;
+ return err;
/* create new sock for complete connection */
- newsk = unix_create1(net, NULL, 0, sock->type);
+ newsk = unix_create1(sock_net(sk), NULL, 0, sock->type);
if (IS_ERR(newsk)) {
err = PTR_ERR(newsk);
- goto out;
+ goto out_drop;
}
/* Allocate skb for sending to listening sock */
@@ -1696,21 +1712,56 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uad
goto out_free_sk;
}
-restart:
- /* Find listening sock. */
- other = unix_find_other(net, sunaddr, addr_len, sk->sk_type, flags);
- if (IS_ERR(other)) {
- err = PTR_ERR(other);
- goto out_free_skb;
- }
+ st->newsk = newsk;
+ st->skb = skb;
+ return 0;
+
+out_free_sk:
+ unix_release_sock(newsk, 0);
+out_drop:
+ drop_peercred(&st->peercred);
+ return err;
+}
+
+/*
+ * Positive returns from unix_stream_connect_commit() ask the caller to
+ * try again. They are distinct only for a caller with a fixed peer
+ * (kernel_unix_connect_direct()): a full backlog can be retried on the
+ * same peer, but a peer found dead cannot -- the by-name path must
+ * re-resolve it, and a fixed peer has no such recourse and fails.
+ */
+#define UNIX_CONNECT_STALE 1 /* peer was found dead */
+#define UNIX_CONNECT_FULL 2 /* backlog was full and we slept */
+
+/*
+ * Try to connect @sk to the listening peer @other, using the connect
+ * state @st built by unix_stream_connect_setup(). Takes and releases
+ * unix_state_lock(@other) itself.
+ *
+ * Returns 0 on success (@st->skb queued to @other, @st->newsk linked to
+ * @sk and @st->peercred consumed), a negative errno on terminal failure,
+ * or a positive value (UNIX_CONNECT_STALE / UNIX_CONNECT_FULL) asking the
+ * caller to re-obtain @other and call again -- because @other was found
+ * dead, or its backlog was full and we slept (updating @st->timeo)
+ * waiting for room.
+ */
+static int unix_stream_connect_commit(struct sock *sk, struct sock *other,
+ struct unix_connect_state *st)
+{
+ struct sock *newsk = st->newsk;
+ struct sk_buff *skb = st->skb;
+ struct unix_peercred *peercred = &st->peercred;
+ long *timeo = &st->timeo;
+ struct unix_sock *newu, *otheru;
+ unsigned char state;
+ int err;
unix_state_lock(other);
- /* Apparently VFS overslept socket death. Retry. */
+ /* Apparently VFS overslept socket death; ask the caller to retry. */
if (sock_flag(other, SOCK_DEAD)) {
unix_state_unlock(other);
- sock_put(other);
- goto restart;
+ return UNIX_CONNECT_STALE;
}
if (other->sk_state != TCP_LISTEN ||
@@ -1720,19 +1771,19 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uad
}
if (unix_recvq_full_lockless(other)) {
- if (!timeo) {
+ if (!*timeo) {
err = -EAGAIN;
goto out_unlock;
}
- timeo = unix_wait_for_peer(other, timeo);
- sock_put(other);
+ /* unix_wait_for_peer() drops unix_state_lock(other). */
+ *timeo = unix_wait_for_peer(other, *timeo);
- err = sock_intr_errno(timeo);
+ err = sock_intr_errno(*timeo);
if (signal_pending(current))
- goto out_free_skb;
+ return err;
- goto restart;
+ return UNIX_CONNECT_FULL;
}
/* self connect and simultaneous connect are eliminated
@@ -1765,7 +1816,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uad
newsk->sk_state = TCP_ESTABLISHED;
newsk->sk_type = sk->sk_type;
newsk->sk_scm_recv_flags = other->sk_scm_recv_flags;
- init_peercred(newsk, &peercred);
+ init_peercred(newsk, peercred);
newu = unix_sk(newsk);
newu->listener = other;
@@ -1813,20 +1864,118 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uad
spin_unlock(&other->sk_receive_queue.lock);
unix_state_unlock(other);
READ_ONCE(other->sk_data_ready)(other);
- sock_put(other);
return 0;
out_unlock:
unix_state_unlock(other);
+ return err;
+}
+
+static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uaddr,
+ int addr_len, int flags)
+{
+ struct sockaddr_un *sunaddr = (struct sockaddr_un *)uaddr;
+ struct sock *sk = sock->sk, *other;
+ struct unix_connect_state st = {};
+ struct net *net = sock_net(sk);
+ int err;
+
+ err = unix_validate_addr(sunaddr, addr_len);
+ if (err)
+ return err;
+
+ err = BPF_CGROUP_RUN_PROG_UNIX_CONNECT_LOCK(sk, uaddr, &addr_len);
+ if (err)
+ return err;
+
+ err = unix_stream_connect_setup(sock, flags, &st);
+ if (err)
+ return err;
+
+restart:
+ /* Find the listening sock. A positive return from
+ * unix_stream_connect_commit() means "retry": the peer had died,
+ * or its backlog was full and we slept -- so re-resolve the name.
+ */
+ other = unix_find_other(net, sunaddr, addr_len, sk->sk_type, flags);
+ if (IS_ERR(other)) {
+ err = PTR_ERR(other);
+ goto out_free;
+ }
+
+ err = unix_stream_connect_commit(sk, other, &st);
sock_put(other);
-out_free_skb:
- consume_skb(skb);
-out_free_sk:
- unix_release_sock(newsk, 0);
-out:
- drop_peercred(&peercred);
+ switch (err) {
+ case 0:
+ return 0;
+ case UNIX_CONNECT_FULL:
+ goto restart;
+ case UNIX_CONNECT_STALE:
+ /* A full backlog or a dead peer: re-resolve and try again. */
+ goto restart;
+ case INT_MIN ... -1:
+ /* terminal errno, propagate as-is */
+ break;
+ default:
+ /* commit() only returns 0, a retry code, or an errno */
+ WARN_ONCE(1, "unix_stream_connect_commit() returned %d\n", err);
+ err = -EINVAL;
+ break;
+ }
+out_free:
+ unix_stream_connect_cleanup(&st);
+ return err;
+}
+
+/**
+ * kernel_unix_connect_direct - connect a socket to a specific AF_UNIX sock
+ * @other: a held listening sock to connect to (e.g. from
+ * unix_lookup_bsd_path())
+ * @sock: the connecting socket, created with sock_create_kern()
+ * @flags: connect flags; without O_NONBLOCK a full listen backlog on
+ * @other is waited on, as for connect(2)
+ *
+ * Connects @sock to @other without any name lookup, address validation
+ * or path-based permission check. For in-kernel callers that have
+ * already located the target under their own policy. The caller
+ * retains its reference on @other.
+ */
+int kernel_unix_connect_direct(struct sock *other, struct socket *sock, int flags)
+{
+ struct sock *sk = sock->sk;
+ struct unix_connect_state st = {};
+ int err;
+
+ err = unix_stream_connect_setup(sock, flags, &st);
+ if (err)
+ return err;
+
+restart:
+ sock_hold(other);
+ err = unix_stream_connect_commit(sk, other, &st);
+ sock_put(other);
+ switch (err) {
+ case 0:
+ return 0;
+ case UNIX_CONNECT_FULL:
+ goto restart;
+ case UNIX_CONNECT_STALE:
+ /* The peer is fixed, so a dead one cannot be re-found. */
+ err = -ECONNREFUSED;
+ break;
+ case INT_MIN ... -1:
+ /* terminal errno, propagate as-is */
+ break;
+ default:
+ /* commit() only returns 0, a retry code, or an errno */
+ WARN_ONCE(1, "unix_stream_connect_commit() returned %d\n", err);
+ err = -EINVAL;
+ break;
+ }
+ unix_stream_connect_cleanup(&st);
return err;
}
+EXPORT_SYMBOL_GPL(kernel_unix_connect_direct);
static int unix_socketpair(struct socket *socka, struct socket *sockb)
{
--
2.54.0
^ permalink raw reply related
* [RFC PATCH 3/3] coredump, net: remove `SOCK_COREDUMP`
From: John Ericson @ 2026-07-03 7:39 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: John Ericson, Cong Wang, Kuniyuki Iwashima, Simon Horman,
Christian Brauner, David Rheinsberg, Andy Lutomirski,
Sergei Zimmerman, netdev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module,
linux-kernel
In-Reply-To: <20260703073948.2541875-1-John.Ericson@Obsidian.Systems>
From: John Ericson <mail@JohnEricson.me>
The utility of the refactors of the last two commits is demonstrated by
fixing this glaring layer violation: the unix socket implementation
knowing about the coredump socket caller.
Before, when the only way to connect to a socket was via the UAPI
`struct sockaddr_un`, the only way to implement the proper logic that
the kernel needs to resolve the coredump socket path was via hacking it
into the socket implementation.
In addition to being quite ugly, this layer violation is not great for
security. The intent is that `SOCK_COREDUMP` can only be used by the
kernel, and to be clear, I have no reason to believe this is not
correctly enforced today. But because of the many functions with flags
arguments, this is not a locally-enforced invariant. Some change, at
some point, could mess up, and allow user-provided flags to sneak in,
and this strikes me as a mistake waiting to happen. At that point, a
user could exploit this to connect to any socket it likes, bypassing
permission checks.
Now, with the two functions we've just previously factored out, we can
fix the layering. The custom path lookup logic lives with the coredump
caller, where it belongs. Once the right `struct path` is found
(actually just the inode is needed), the coredump caller can resolve a
`struct sock *` from it, and then directly connect to it. With this
change, `SOCK_COREDUMP` is no longer needed at all, and can be deleted.
The layer violation is gone, and the security footgun is gone with it.
As an added bonus, remove `flags` parameters from a number of internal
functions that no longer need them. They were just taking flags
parameters for the sake of `SOCK_COREDUMP`, and so with that gone, those
flag parameters are also no longer needed. If or when there is a new
flag that motivates them, they can be added back.
Tested that `coredump_socket_protocol_test` still passes.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: John Ericson <mail@JohnEricson.me>
---
fs/coredump.c | 47 ++++++++++++++++++++++++-----------
include/linux/lsm_hook_defs.h | 3 +--
include/linux/net.h | 1 -
include/linux/security.h | 4 +--
net/unix/af_unix.c | 42 ++++++++++---------------------
security/landlock/fs.c | 7 +-----
security/security.c | 5 ++--
7 files changed, 51 insertions(+), 58 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index e68a76ff92a3..e1452021218e 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -14,6 +14,7 @@
#include <linux/perf_event.h>
#include <linux/highmem.h>
#include <linux/spinlock.h>
+#include <linux/cred.h>
#include <linux/key.h>
#include <linux/personality.h>
#include <linux/binfmts.h>
@@ -21,6 +22,7 @@
#include <linux/sort.h>
#include <linux/sched/coredump.h>
#include <linux/sched/signal.h>
+#include <linux/sched/task.h>
#include <linux/sched/task_stack.h>
#include <linux/utsname.h>
#include <linux/pid_namespace.h>
@@ -50,7 +52,6 @@
#include <net/net_namespace.h>
#include <net/sock.h>
#include <uapi/linux/pidfd.h>
-#include <uapi/linux/un.h>
#include <uapi/linux/coredump.h>
#include <linux/uaccess.h>
@@ -668,17 +669,10 @@ static int umh_coredump_setup(struct subprocess_info *info, struct cred *new)
static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *cprm)
{
struct file *file __free(fput) = NULL;
- struct sockaddr_un addr = {
- .sun_family = AF_UNIX,
- };
- ssize_t addr_len;
- int retval;
+ struct path root, path;
struct socket *socket;
-
- addr_len = strscpy(addr.sun_path, cn->corename);
- if (addr_len < 0)
- return false;
- addr_len += offsetof(struct sockaddr_un, sun_path) + 1;
+ struct sock *sk;
+ int retval;
/*
* It is possible that the userspace process which is supposed
@@ -710,14 +704,37 @@ static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *
*/
pidfs_coredump(cprm);
- retval = kernel_connect(socket, (struct sockaddr_unsized *)(&addr), addr_len,
- O_NONBLOCK | SOCK_COREDUMP);
+ /*
+ * Resolve the socket path relative to init's root and with kernel
+ * credentials, and with symlinks, magic links and escaping the
+ * root all forbidden, so the dumping process cannot use its own
+ * filesystem view to redirect its core to an arbitrary socket.
+ */
+ task_lock(&init_task);
+ get_fs_root(init_task.fs, &root);
+ task_unlock(&init_task);
+
+ scoped_with_kernel_creds()
+ retval = vfs_path_lookup(root.dentry, root.mnt, cn->corename,
+ LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
+ LOOKUP_NO_MAGICLINKS, &path);
+ path_put(&root);
+ if (retval)
+ return false;
+
+ /* Connect directly to the socket bound there, by fd not by name. */
+ sk = unix_lookup_bsd_path(&path, SOCK_STREAM);
+ path_put(&path);
+ if (IS_ERR(sk))
+ return false;
+ retval = kernel_unix_connect_direct(sk, socket, O_NONBLOCK);
+ sock_put(sk);
if (retval) {
if (retval == -EAGAIN)
- coredump_report_failure("Coredump socket %s receive queue full", addr.sun_path);
+ coredump_report_failure("Coredump socket %s receive queue full", cn->corename);
else
- coredump_report_failure("Coredump socket connection %s failed %d", addr.sun_path, retval);
+ coredump_report_failure("Coredump socket connection %s failed %d", cn->corename, retval);
return false;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 65c9609ec207..3d6fbb6d2628 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -323,8 +323,7 @@ LSM_HOOK(int, 0, watch_key, struct key *key)
#endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
-LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
- int flags)
+LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other)
#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
#ifdef CONFIG_SECURITY_NETWORK
diff --git a/include/linux/net.h b/include/linux/net.h
index f268f395ce47..285cb67927f0 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -102,7 +102,6 @@ enum sock_type {
#ifndef SOCK_NONBLOCK
#define SOCK_NONBLOCK O_NONBLOCK
#endif
-#define SOCK_COREDUMP O_NOCTTY
/**
* enum sock_shutdown_cmd - Shutdown types
diff --git a/include/linux/security.h b/include/linux/security.h
index 153e9043058f..9797bd29c916 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1957,10 +1957,10 @@ static inline int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
-int security_unix_find(const struct path *path, struct sock *other, int flags);
+int security_unix_find(const struct path *path, struct sock *other);
#else /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
-static inline int security_unix_find(const struct path *path, struct sock *other, int flags)
+static inline int security_unix_find(const struct path *path, struct sock *other)
{
return 0;
}
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index aa94da1f8c24..7cb537b404cc 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1220,7 +1220,7 @@ struct sock *unix_lookup_bsd_path(const struct path *path, int type)
EXPORT_SYMBOL_GPL(unix_lookup_bsd_path);
static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
- int type, int flags)
+ int type)
{
struct path path;
struct sock *sk;
@@ -1228,29 +1228,13 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
unix_mkname_bsd(sunaddr, addr_len);
- if (flags & SOCK_COREDUMP) {
- struct path root;
-
- task_lock(&init_task);
- get_fs_root(init_task.fs, &root);
- task_unlock(&init_task);
-
- scoped_with_kernel_creds()
- err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
- LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
- LOOKUP_NO_MAGICLINKS, &path);
- path_put(&root);
- if (err)
- goto fail;
- } else {
- err = kern_path(sunaddr->sun_path, LOOKUP_FOLLOW, &path);
- if (err)
- goto fail;
+ err = kern_path(sunaddr->sun_path, LOOKUP_FOLLOW, &path);
+ if (err)
+ goto fail;
- err = path_permission(&path, MAY_WRITE);
- if (err)
- goto path_put;
- }
+ err = path_permission(&path, MAY_WRITE);
+ if (err)
+ goto path_put;
sk = unix_lookup_bsd_path(&path, type);
if (IS_ERR(sk)) {
@@ -1258,7 +1242,7 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
goto path_put;
}
- err = security_unix_find(&path, sk, flags);
+ err = security_unix_find(&path, sk);
if (err)
goto sock_put;
@@ -1297,12 +1281,12 @@ static struct sock *unix_find_abstract(struct net *net,
static struct sock *unix_find_other(struct net *net,
struct sockaddr_un *sunaddr,
- int addr_len, int type, int flags)
+ int addr_len, int type)
{
struct sock *sk;
if (sunaddr->sun_path[0])
- sk = unix_find_bsd(sunaddr, addr_len, type, flags);
+ sk = unix_find_bsd(sunaddr, addr_len, type);
else
sk = unix_find_abstract(net, sunaddr, addr_len, type);
@@ -1558,7 +1542,7 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr_unsized *addr
}
restart:
- other = unix_find_other(sock_net(sk), sunaddr, alen, sock->type, 0);
+ other = unix_find_other(sock_net(sk), sunaddr, alen, sock->type);
if (IS_ERR(other)) {
err = PTR_ERR(other);
goto out;
@@ -1897,7 +1881,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr_unsized *uad
* unix_stream_connect_commit() means "retry": the peer had died,
* or its backlog was full and we slept -- so re-resolve the name.
*/
- other = unix_find_other(net, sunaddr, addr_len, sk->sk_type, flags);
+ other = unix_find_other(net, sunaddr, addr_len, sk->sk_type);
if (IS_ERR(other)) {
err = PTR_ERR(other);
goto out_free;
@@ -2330,7 +2314,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
if (msg->msg_namelen) {
lookup:
other = unix_find_other(sock_net(sk), msg->msg_name,
- msg->msg_namelen, sk->sk_type, 0);
+ msg->msg_namelen, sk->sk_type);
if (IS_ERR(other)) {
err = PTR_ERR(other);
goto out_free;
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index f7e5e4ef9eac..d77080438c01 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1641,8 +1641,7 @@ static void unmask_scoped_access(const struct landlock_ruleset *const client,
}
}
-static int hook_unix_find(const struct path *const path, struct sock *other,
- int flags)
+static int hook_unix_find(const struct path *const path, struct sock *other)
{
const struct landlock_ruleset *dom_other;
const struct landlock_cred_security *subject;
@@ -1652,10 +1651,6 @@ static int hook_unix_find(const struct path *const path, struct sock *other,
.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
};
- /* Lookup for the purpose of saving coredumps is OK. */
- if (unlikely(flags & SOCK_COREDUMP))
- return 0;
-
subject = landlock_get_applicable_subject(current_cred(),
fs_resolve_unix, NULL);
diff --git a/security/security.c b/security/security.c
index 71aea8fdf014..fabb75c88254 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4839,16 +4839,15 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
* security_unix_find() - Check if a named AF_UNIX socket can connect
* @path: path of the socket being connected to
* @other: peer sock
- * @flags: flags associated with the socket
*
* This hook is called to check permissions before connecting to a named
* AF_UNIX socket. The caller does not hold any locks on @other.
*
* Return: Returns 0 if permission is granted.
*/
-int security_unix_find(const struct path *path, struct sock *other, int flags)
+int security_unix_find(const struct path *path, struct sock *other)
{
- return call_int_hook(unix_find, path, other, flags);
+ return call_int_hook(unix_find, path, other);
}
EXPORT_SYMBOL(security_unix_find);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH -next] lockdown: Add break in lockdown_write
From: Nicolas Bouchinet @ 2026-07-03 7:39 UTC (permalink / raw)
To: Paul Moore
Cc: Xiu Jianfeng, Cai Xinchen, jmorris, serge, linux-security-module,
linux-kernel
In-Reply-To: <CAHC9VhTQT05+1qYM-8_YTc84Rx2SdZV1iRixeqAywCXf1mMddQ@mail.gmail.com>
On Thu, Jul 02, 2026 at 12:50:18PM -0400, Paul Moore wrote:
> On Thu, Jan 22, 2026 at 12:41???PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Thu, Jan 22, 2026 at 4:31???AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
> > > 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? :)
> >
> > Assuming Nicolas is okay with that, I can take this through the LSM tree.
>
> Following up on this as I never saw a response on-list and the patch
> doesn't appear to be in Linus' tree.
Your right, my bad,
First of all, thanks Cai for your contribution and I appologize for the
really late reply.
This is pretty much the same kind of patch as [1], while it effectively
cleans up Lockdown code, in my opinion it doesn't directly add much. I'd
personally prefer to keep those small patches on hold for now, waiting
for a bigger review of Lockdown's code where I will pick them both up.
Sadly, I really don't have much time to work on this right now.
Nicolas
---
[1]: https://lore.kernel.org/all/20260501174448.47154-1-kalevi@kolttonen.fi/
^ permalink raw reply
* Re: [RFC PATCH 3/3] coredump, net: remove `SOCK_COREDUMP`
From: Christian Brauner @ 2026-07-03 8:11 UTC (permalink / raw)
To: John Ericson
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
John Ericson, Cong Wang, Kuniyuki Iwashima, Simon Horman,
Christian Brauner, David Rheinsberg, Andy Lutomirski,
Sergei Zimmerman, netdev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module,
linux-kernel
In-Reply-To: <20260703073948.2541875-4-John.Ericson@Obsidian.Systems>
> The utility of the refactors of the last two commits is demonstrated by
> fixing this glaring layer violation: the unix socket implementation
> knowing about the coredump socket caller.
>
> Before, when the only way to connect to a socket was via the UAPI
> `struct sockaddr_un`, the only way to implement the proper logic that
> the kernel needs to resolve the coredump socket path was via hacking it
> into the socket implementation.
>
> In addition to being quite ugly, this layer violation is not great for
> security. The intent is that `SOCK_COREDUMP` can only be used by the
> kernel, and to be clear, I have no reason to believe this is not
> correctly enforced today. But because of the many functions with flags
> arguments, this is not a locally-enforced invariant. Some change, at
> some point, could mess up, and allow user-provided flags to sneak in,
> and this strikes me as a mistake waiting to happen. At that point, a
> user could exploit this to connect to any socket it likes, bypassing
> permission checks.
>
> Now, with the two functions we've just previously factored out, we can
> fix the layering. The custom path lookup logic lives with the coredump
> caller, where it belongs. Once the right `struct path` is found
> (actually just the inode is needed), the coredump caller can resolve a
> `struct sock *` from it, and then directly connect to it. With this
> change, `SOCK_COREDUMP` is no longer needed at all, and can be deleted.
> The layer violation is gone, and the security footgun is gone with it.
>
> As an added bonus, remove `flags` parameters from a number of internal
> functions that no longer need them. They were just taking flags
> parameters for the sake of `SOCK_COREDUMP`, and so with that gone, those
> flag parameters are also no longer needed. If or when there is a new
> flag that motivates them, they can be added back.
>
> Tested that `coredump_socket_protocol_test` still passes.
>
> Signed-off-by: John Ericson <mail@JohnEricson.me>
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index e68a76ff92a3..e1452021218e 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -14,6 +14,7 @@
> #include <linux/perf_event.h>
> #include <linux/highmem.h>
> #include <linux/spinlock.h>
> +#include <linux/cred.h>
> #include <linux/key.h>
> #include <linux/personality.h>
> #include <linux/binfmts.h>
> @@ -21,6 +22,7 @@
> #include <linux/sort.h>
> #include <linux/sched/coredump.h>
> #include <linux/sched/signal.h>
> +#include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/utsname.h>
> #include <linux/pid_namespace.h>
> @@ -50,7 +52,6 @@
> #include <net/net_namespace.h>
> #include <net/sock.h>
> #include <uapi/linux/pidfd.h>
> -#include <uapi/linux/un.h>
> #include <uapi/linux/coredump.h>
>
> #include <linux/uaccess.h>
> @@ -668,17 +669,10 @@ static int umh_coredump_setup(struct subprocess_info *info, struct cred *new)
> static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *cprm)
> {
> struct file *file __free(fput) = NULL;
> - struct sockaddr_un addr = {
> - .sun_family = AF_UNIX,
> - };
> - ssize_t addr_len;
> - int retval;
> + struct path root, path;
> struct socket *socket;
> -
> - addr_len = strscpy(addr.sun_path, cn->corename);
> - if (addr_len < 0)
> - return false;
> - addr_len += offsetof(struct sockaddr_un, sun_path) + 1;
> + struct sock *sk;
> + int retval;
>
> /*
> * It is possible that the userspace process which is supposed
> @@ -710,14 +704,37 @@ static bool coredump_sock_connect(struct core_name *cn, struct coredump_params *
> */
> pidfs_coredump(cprm);
>
> - retval = kernel_connect(socket, (struct sockaddr_unsized *)(&addr), addr_len,
> - O_NONBLOCK | SOCK_COREDUMP);
> + /*
> + * Resolve the socket path relative to init's root and with kernel
> + * credentials, and with symlinks, magic links and escaping the
> + * root all forbidden, so the dumping process cannot use its own
> + * filesystem view to redirect its core to an arbitrary socket.
> + */
> + task_lock(&init_task);
> + get_fs_root(init_task.fs, &root);
> + task_unlock(&init_task);
> +
> + scoped_with_kernel_creds()
> + retval = vfs_path_lookup(root.dentry, root.mnt, cn->corename,
> + LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> + LOOKUP_NO_MAGICLINKS, &path);
> + path_put(&root);
> + if (retval)
> + return false;
> +
> + /* Connect directly to the socket bound there, by fd not by name. */
> + sk = unix_lookup_bsd_path(&path, SOCK_STREAM);
> + path_put(&path);
> + if (IS_ERR(sk))
> + return false;
>
> + retval = kernel_unix_connect_direct(sk, socket, O_NONBLOCK);
I don't think dragging the bowels of net/ into fs/coredump.c just for
the sake of getting a purely internal SOCK_COREDUMP flag out of the way
is the right way to go.
The two helpers also make no sense to me and force a bunch of cleanup on
the caller as well.
I suspect the saner thing to do would be to introduce a primitive for
connecting to an AF_UNIX path-based socket directly instead of this
weird dance here, no?
int kernel_unix_connect(const char *path, struct socket *socket, unsigned int o_flags, int type)
then my kthread changes would collapse this into:
scoped_with_init_fs()
retval = kernel_unix_connect(path, socket, O_NONBLOCK, SOCK_STREAM);
> + sock_put(sk);
> if (retval) {
> if (retval == -EAGAIN)
> - coredump_report_failure("Coredump socket %s receive queue full", addr.sun_path);
> + coredump_report_failure("Coredump socket %s receive queue full", cn->corename);
> else
> - coredump_report_failure("Coredump socket connection %s failed %d", addr.sun_path, retval);
> + coredump_report_failure("Coredump socket connection %s failed %d", cn->corename, retval);
> return false;
> }
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 65c9609ec207..3d6fbb6d2628 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -323,8 +323,7 @@ LSM_HOOK(int, 0, watch_key, struct key *key)
> #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
>
> #if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
> -LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
> - int flags)
> +LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other)
I'm not sure why we would go on altering all kinds of LSM hooks as well.
--
Christian Brauner <brauner@kernel.org>
^ permalink raw reply
* Re: [RFC PATCH 3/3] coredump, net: remove `SOCK_COREDUMP`
From: John Ericson @ 2026-07-03 9:08 UTC (permalink / raw)
To: Christian Brauner, John Ericson
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Cong Wang, Kuniyuki Iwashima, Simon Horman, David Rheinsberg,
Andy Lutomirski, Sergei Zimmerman, network dev, linux-fsdevel,
Mickaël Salaün, Günther Noack, Paul Moore,
linux-security-module, LKML
In-Reply-To: <20260703-umgehen-auftun-anlagen-826bf59dfcb5@brauner>
On Fri, Jul 3, 2026, at 4:11 AM, Christian Brauner wrote:
> I don't think dragging the bowels of net/ into fs/coredump.c just for
> the sake of getting a purely internal SOCK_COREDUMP flag out of the way
> is the right way to go.
>
> I suspect the saner thing to do would be to introduce a primitive for
> connecting to an AF_UNIX path-based socket directly instead of this
> weird dance here, no?
>
> int kernel_unix_connect(const char *path, struct socket *socket, unsigned int o_flags, int type)
>
> then my kthread changes would collapse this into:
>
> scoped_with_init_fs()
> retval = kernel_unix_connect(path, socket, O_NONBLOCK, SOCK_STREAM);
That works for this case, but the destination I am trying to eventually
reach is being allowed (in userspace) to connect to unbound sockets by
their fd --- no path, no abstract socket name.
unix_connectat(int fd, const char *path, struct socket *socket,
unsigned int at_flags, unsigned int o_flags,
int type)
is a mouthful, but it would work. Still, there is a subtlety about the
retry logic. When one does something like:
> connect(..."/dev/fd/N",...)
it will repeatedly re-lookup "/dev/fd/N" until the timeout is reached. I
consider that pretty terrible --- the rest of the program could race to
change what that file descriptor (number) refers to. Therefore, I think
table stakes to make a good `unix_connectat` is to make the
`AT_EMPTY_PATH` case not re-lookup the socket.
(making a sockfs file descriptor work is separate, I already have the
patch for that, I can include it.)
> The two helpers also make no sense to me and force a bunch of cleanup on
> the caller as well.
I am fine with `unix_connectat`, but just for reference, me breaking up
the steps is because I generally like the decomposition of `fverb`
rather than `verbat` system calls, since the latter would typically be
used with `openat` or so. It seems the `kernel_*` "syscalls" generally
take `struct path` rather than strings, which seems good and in the
spirit of that decomposition, but then even `struct path` is overkill to
refer to a socket. So putting all that together, the final composition I
had was:
1. string path (from /proc/...) -> `struct path`
2. `struct path` -> `struct sock *`
3. connect to `struct sock *`
So I quite liked those 3 orthogonal knobs, vs an all-singing-all-dancing
`unix_connectat`. (I suppose making it `struct socket *` would make it
slightly less internals-y?) But again, anything that puts us on track to
being able to connect to an unbound socket without procfs is good enough
for me.
> I'm not sure why we would go on altering all kinds of LSM hooks as well.
That's in the commit message, it is because without `SOCK_COREDUMP`
those are all dead code. Instead of removing them in this commit, I can
just keep those flags, or remove them in a separate commit. Fine
with any of those.
John
^ permalink raw reply
* Re: [RFC PATCH 3/3] coredump, net: remove `SOCK_COREDUMP`
From: Christian Brauner @ 2026-07-03 9:31 UTC (permalink / raw)
To: John Ericson
Cc: Christian Brauner, John Ericson, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Cong Wang, Kuniyuki Iwashima,
Simon Horman, David Rheinsberg, Andy Lutomirski, Sergei Zimmerman,
network dev, linux-fsdevel, Mickaël Salaün,
Günther Noack, Paul Moore, linux-security-module, LKML
In-Reply-To: <7acb650b-a0de-4244-861f-4dda70ca8993@app.fastmail.com>
On 2026-07-03 05:08 -0400, John Ericson wrote:
> On Fri, Jul 3, 2026, at 4:11 AM, Christian Brauner wrote:
> > I don't think dragging the bowels of net/ into fs/coredump.c just for
> > the sake of getting a purely internal SOCK_COREDUMP flag out of the way
> > is the right way to go.
> >
> > I suspect the saner thing to do would be to introduce a primitive for
> > connecting to an AF_UNIX path-based socket directly instead of this
> > weird dance here, no?
> >
> > int kernel_unix_connect(const char *path, struct socket *socket, unsigned int o_flags, int type)
> >
> > then my kthread changes would collapse this into:
> >
> > scoped_with_init_fs()
> > retval = kernel_unix_connect(path, socket, O_NONBLOCK, SOCK_STREAM);
>
> That works for this case, but the destination I am trying to eventually
> reach is being allowed (in userspace) to connect to unbound sockets by
> their fd --- no path, no abstract socket name.
But these are orthogonal changes. It sill doesn't mean we should sprawl
net internals over coredump code anymore than we already have to. I'm
fond of kernel_unix_connect() int dfd, const char *path even would be
fine.
> unix_connectat(int fd, const char *path, struct socket *socket,
> unsigned int at_flags, unsigned int o_flags,
> int type)
>
> is a mouthful, but it would work. Still, there is a subtlety about the
> retry logic. When one does something like:
>
> > connect(..."/dev/fd/N",...)
>
> it will repeatedly re-lookup "/dev/fd/N" until the timeout is reached. I
> consider that pretty terrible --- the rest of the program could race to
> change what that file descriptor (number) refers to. Therefore, I think
> table stakes to make a good `unix_connectat` is to make the
> `AT_EMPTY_PATH` case not re-lookup the socket.
I have no idea how this has any bearong on the three helper split.
> (making a sockfs file descriptor work is separate, I already have the
> patch for that, I can include it.)
>
> > The two helpers also make no sense to me and force a bunch of cleanup on
> > the caller as well.
>
> I am fine with `unix_connectat`, but just for reference, me breaking up
> the steps is because I generally like the decomposition of `fverb`
> rather than `verbat` system calls, since the latter would typically be
> used with `openat` or so. It seems the `kernel_*` "syscalls" generally
> take `struct path` rather than strings, which seems good and in the
There's a time to follow precedence and there's a time to do what the
maintainability demands. I think here clearly we don't want to pass a
struct path. That makes no sense because we don't care about using that
object at all. It's merely an intermediate jumping point. This is
fundamentally about connecting from the kernel to a specific pathname
that functions as a socket address. Intermediary objects don't matter
(yet).
> spirit of that decomposition, but then even `struct path` is overkill to
> refer to a socket. So putting all that together, the final composition I
> had was:
>
> 1. string path (from /proc/...) -> `struct path`
> 2. `struct path` -> `struct sock *`
> 3. connect to `struct sock *`
>
> So I quite liked those 3 orthogonal knobs, vs an all-singing-all-dancing
> `unix_connectat`. (I suppose making it `struct socket *` would make it
> slightly less internals-y?) But again, anything that puts us on track to
> being able to connect to an unbound socket without procfs is good enough
> for me.
To me this reads like you're introducing moving parts for the sake of a
philosophical argument. I couldn't care less about that. What matters is
maintainability. Having three knobs that introduce potentially three
separate cleanup requirements and exposing three different objects when
it's not needed is just a burden.
>
> > I'm not sure why we would go on altering all kinds of LSM hooks as well.
>
> That's in the commit message, it is because without `SOCK_COREDUMP`
> those are all dead code. Instead of removing them in this commit, I can
> just keep those flags, or remove them in a separate commit. Fine
Do it as a separte commit once we've decided to do this, I think.
^ permalink raw reply
* [PATCH v2] selftests/lsm: Fix memory leak in attr_lsm_count
From: Wang Yan @ 2026-07-03 12:09 UTC (permalink / raw)
To: bill.c.roberts
Cc: casey, jmorris, linux-kernel, linux-kselftest,
linux-security-module, mic, paul, serge, shuah, wangyan01
In-Reply-To: <CAFftDdrDgoHCopx9byaupea8GB6P=qSAY==6QQoXSJcJgn_04w@mail.gmail.com>
The calloc-allocated buffer in attr_lsm_count() is never released on
any exit path, including both the normal return path and the early
return when read_sysfs_lsms fails, resulting in a heap memory leak.
Add free() for the buffer on all return branches to fix the leak.
Fixes: d3d929a8b0cd ("LSM: selftests for Linux Security Module syscalls")
Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
---
tools/testing/selftests/lsm/common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/lsm/common.c b/tools/testing/selftests/lsm/common.c
index 9ad258912646..927dce4f04cb 100644
--- a/tools/testing/selftests/lsm/common.c
+++ b/tools/testing/selftests/lsm/common.c
@@ -76,7 +76,7 @@ int attr_lsm_count(void)
return 0;
if (read_sysfs_lsms(names, sysconf(_SC_PAGESIZE)))
- return 0;
+ goto out;
if (strstr(names, "selinux"))
count++;
@@ -85,5 +85,7 @@ int attr_lsm_count(void)
if (strstr(names, "apparmor"))
count++;
+out:
+ free(names);
return count;
}
--
2.25.1
^ permalink raw reply related
* [PATCH v1] landlock: Fix kernel-doc for the nested quiet layer flag
From: Mickaël Salaün @ 2026-07-03 14:17 UTC (permalink / raw)
To: Günther Noack
Cc: Mickaël Salaün, linux-security-module, Tingmao Wang,
Justin Suess
kernel-doc emits "Excess struct member 'quiet' description in
'landlock_layer'" because "quiet" is a bitfield inside the named nested
struct "flags", but its inline comment used the bare member name
"@quiet:", which kernel-doc attributes to the enclosing landlock_layer.
Use the canonical dotted notation "@flags.quiet:" so kernel-doc resolves
the nested member, and include it in the generated documentation.
Cc: Justin Suess <utilityemal77@gmail.com>
Cc: Tingmao Wang <m@maowtm.org>
Fixes: a260c0055665 ("landlock: Add a place for flags to layer rules")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/ruleset.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 61f3c253d5c9..0437adf17428 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -35,8 +35,8 @@ struct landlock_layer {
*/
struct {
/**
- * @quiet: Suppresses denial logs for the object covered by this
- * rule in this domain. For filesystem rules, this inherits
+ * @flags.quiet: Suppresses denial logs for the object covered by
+ * this rule in this domain. For filesystem rules, this inherits
* down the file hierarchy.
*/
u8 quiet : 1;
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
--
2.54.0
^ permalink raw reply related
* [PATCH v1] landlock: Harden sock_is_scoped() against file-less sockets
From: Mickaël Salaün @ 2026-07-03 15:27 UTC (permalink / raw)
To: Günther Noack
Cc: Mickaël Salaün, linux-security-module, Mikhail Ivanov,
Matthieu Buffet
sock_is_scoped() dereferences other->sk_socket->file->f_cred to read the
peer's Landlock domain when evaluating
LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET, without first checking that the
peer has a backing socket and file. hook_unix_find() performs the same
dereference for LANDLOCK_ACCESS_FS_RESOLVE_UNIX and does guard it.
Guard it here too and treat a peer with no backing file, such as a
kernel socket created by sock_create_kern(), as unscoped.
This is defensive hardening, not a fix for a reachable bug. The
unix_stream_connect() and unix_may_send() hooks run with the peer held
under unix_state_lock() and only after the AF_UNIX core has excluded
SOCK_DEAD, and no in-tree code binds a file-less AF_UNIX socket to an
abstract address, so other->sk_socket->file is always valid at these
call sites today.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/task.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 7ddf211f75c3..474642edbd59 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -243,6 +243,17 @@ static bool sock_is_scoped(struct sock *const other,
/* The credentials will not change. */
lockdep_assert_held(&unix_sk(other)->lock);
+
+ /*
+ * A live kernel socket (e.g. from sock_create_kern()) has no backing
+ * file, hence no Landlock domain, so treat it as unscoped. The
+ * sk_socket check only guards that dereference; sk_socket is NULL
+ * solely for a dead peer, which the caller already excludes under the
+ * held lock, so no separate SOCK_DEAD check is needed.
+ */
+ if (unlikely(!other->sk_socket || !other->sk_socket->file))
+ return false;
+
dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
return domain_is_scoped(domain, dom_other,
LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2] selftests/lsm: Fix memory leak in attr_lsm_count
From: William Roberts @ 2026-07-03 17:56 UTC (permalink / raw)
To: Wang Yan
Cc: casey, jmorris, linux-kernel, linux-kselftest,
linux-security-module, mic, paul, serge, shuah
In-Reply-To: <20260703120951.1622491-1-wangyan01@kylinos.cn>
On Fri, Jul 3, 2026 at 7:10 AM Wang Yan <wangyan01@kylinos.cn> wrote:
>
> The calloc-allocated buffer in attr_lsm_count() is never released on
> any exit path, including both the normal return path and the early
> return when read_sysfs_lsms fails, resulting in a heap memory leak.
>
> Add free() for the buffer on all return branches to fix the leak.
>
> Fixes: d3d929a8b0cd ("LSM: selftests for Linux Security Module syscalls")
> Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
> ---
> tools/testing/selftests/lsm/common.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/lsm/common.c b/tools/testing/selftests/lsm/common.c
> index 9ad258912646..927dce4f04cb 100644
> --- a/tools/testing/selftests/lsm/common.c
> +++ b/tools/testing/selftests/lsm/common.c
> @@ -76,7 +76,7 @@ int attr_lsm_count(void)
> return 0;
>
> if (read_sysfs_lsms(names, sysconf(_SC_PAGESIZE)))
> - return 0;
> + goto out;
>
> if (strstr(names, "selinux"))
> count++;
> @@ -85,5 +85,7 @@ int attr_lsm_count(void)
> if (strstr(names, "apparmor"))
> count++;
>
> +out:
> + free(names);
> return count;
> }
> --
> 2.25.1
>
Reviewed-by: William Roberts <bill.c.roberts@gmail.com>
Tested-by: William Roberts <bill.c.roberts@gmail.com>
^ permalink raw reply
* Re: [PATCH 01/10] docs/zh_CN: add LSM/index Chinese translation
From: Weijie Yuan @ 2026-07-04 15:20 UTC (permalink / raw)
To: Yan Zhu
Cc: alexs, si.yanteng, corbet, mic, dzm91, skhan, gnoack, linux-doc,
linux-security-module
In-Reply-To: <cover.1781105672.git.zhuyan2015@qq.com>
Hi Zhu Yan,
By the way, I could not find the cover letter (the mail I am replying
to) on lore.kernel.org. It seems to be missing there.
^ 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