* Re: [PATCH v6 06/15] VFS: introduce start_creating_noperm() and start_removing_noperm()
From: Val Packett @ 2025-11-30 0:01 UTC (permalink / raw)
To: NeilBrown, Alexander Viro, Christian Brauner, Amir Goldstein
Cc: Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <20251113002050.676694-7-neilb@ownmail.net>
Hi,
On 11/12/25 9:18 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> xfs, fuse, ipc/mqueue need variants of start_creating or start_removing
> which do not check permissions.
> This patch adds _noperm versions of these functions.
> [..]
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 316922d5dd13..a0d5b302bcc2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1397,27 +1397,25 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> if (!parent)
> return -ENOENT;
>
> - inode_lock_nested(parent, I_MUTEX_PARENT);
> if (!S_ISDIR(parent->i_mode))
> - goto unlock;
> + goto put_parent;
>
> err = -ENOENT;
> dir = d_find_alias(parent);
> if (!dir)
> - goto unlock;
> + goto put_parent;
>
> - name->hash = full_name_hash(dir, name->name, name->len);
> - entry = d_lookup(dir, name);
> + entry = start_removing_noperm(dir, name);
> dput(dir);
> - if (!entry)
> - goto unlock;
> + if (IS_ERR(entry))
> + goto put_parent;
This broke xdg-document-portal (and potentially other FUSE filesystems)
by introducing a massive deadlock.
❯ doas cat /proc/40751/stack # main thread
[<0>] __fuse_simple_request+0x37c/0x5c0 [fuse]
[<0>] fuse_lookup_name+0x12c/0x2a0 [fuse]
[<0>] fuse_lookup+0x9c/0x1e8 [fuse]
[<0>] lookup_one_qstr_excl+0xd4/0x160
[<0>] start_removing_noperm+0x5c/0x90
[<0>] fuse_reverse_inval_entry+0x64/0x1e0 [fuse]
[<0>] fuse_dev_do_write+0x13a8/0x16a8 [fuse]
[<0>] fuse_dev_write+0x64/0xa8 [fuse]
[<0>] do_iter_readv_writev+0x170/0x1d0
[<0>] vfs_writev+0x100/0x2d0
[<0>] do_writev+0x88/0x130
d_lookup which was previously used here —from what I could understand by
reading it— is cache-only and does not call into the FS's lookup at all.
This new start_removing_noperm calls start_dirop which calls
lookup_one_qstr_excl which according to its own comment is the "one and
only case when ->lookup() gets called on non in-lookup dentries". Well,
->lookup() is the request back to the userspace FUSE server.. but the
FUSE server is waiting for the write() to the FUSE device that invokes
this operation to return! We cannot reenter the FUSE server
from fuse_reverse_inval_entry.
x-d-p issue link: https://github.com/flatpak/xdg-desktop-portal/issues/1871
Reverting the fuse/dir.c changes has fixed that for me.
Thanks,
~val
^ permalink raw reply
* Re: [PATCH v6 06/15] VFS: introduce start_creating_noperm() and start_removing_noperm()
From: Al Viro @ 2025-11-30 0:19 UTC (permalink / raw)
To: Val Packett
Cc: NeilBrown, Christian Brauner, Amir Goldstein, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <6713ea38-b583-4c86-b74a-bea55652851d@packett.cool>
On Sat, Nov 29, 2025 at 09:01:05PM -0300, Val Packett wrote:
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 316922d5dd13..a0d5b302bcc2 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1397,27 +1397,25 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> > if (!parent)
> > return -ENOENT;
> > - inode_lock_nested(parent, I_MUTEX_PARENT);
> > if (!S_ISDIR(parent->i_mode))
> > - goto unlock;
> > + goto put_parent;
> > err = -ENOENT;
> > dir = d_find_alias(parent);
> > if (!dir)
> > - goto unlock;
> > + goto put_parent;
> > - name->hash = full_name_hash(dir, name->name, name->len);
> > - entry = d_lookup(dir, name);
> > + entry = start_removing_noperm(dir, name);
> > dput(dir);
> > - if (!entry)
> > - goto unlock;
> > + if (IS_ERR(entry))
> > + goto put_parent;
>
> This broke xdg-document-portal (and potentially other FUSE filesystems) by
> introducing a massive deadlock.
ACK. That chunk needs to be reverted - this is *not* "remove an object by
parent and name", it's "invalidate stuff under that parent with this
first name component" and I would like to understand what FUSE_EXPIRE_ONLY
thing is about.
Miklos, could you give some details on that thing? This chunk definitely
needs to go, the question is what that code is trying to do other than
d_invalidate()...
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Al Viro @ 2025-11-30 4:23 UTC (permalink / raw)
To: Song Liu
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge
In-Reply-To: <20251127005011.1872209-3-song@kernel.org>
On Wed, Nov 26, 2025 at 04:50:06PM -0800, Song Liu wrote:
> Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
> by BPF LSM programs:
>
> bpf_kern_path():
> - Resolves a pathname string to a struct path
> These kfuncs enable BPF LSM programs to resolve pathnames provided by
> hook arguments (e.g., dev_name from sb_mount) and validate or inspect
> the resolved paths. The verifier enforces proper resource management
> through acquire/release tracking.
Oh, *brilliant*. Thank you for giving a wonderful example of the reasons
why this is fundamentally worthless.
OK, your "BPF LSM" has been called and it got that dev_name. You decide
that you want to know what it resolves to (which, BTW, requries a really
non-trivial amount of parsing other arguments - just to figure out whether
it *is* a pathname of some sort). Thanks to your shiny new kfuncs you
can do that! You are a proud holder of mount/dentry pair. You stare at
those and decide whether it's OK to go on. Then you... drop that pair
and let mount(2) proceed towards the point where it will (if you parsed
the arguments correctly) repeat that pathname resolution and get a mount/dentry
pair of its own, that may very well be different from what you've got the
first time around.
Your primitive is a walking TOCTOU bug - it's impossible to use safely.
NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Song Liu @ 2025-11-30 5:57 UTC (permalink / raw)
To: Al Viro
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge, Shervin Oloumi
In-Reply-To: <20251130042357.GP3538@ZenIV>
On Sat, Nov 29, 2025 at 8:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Nov 26, 2025 at 04:50:06PM -0800, Song Liu wrote:
> > Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
> > by BPF LSM programs:
> >
> > bpf_kern_path():
> > - Resolves a pathname string to a struct path
>
> > These kfuncs enable BPF LSM programs to resolve pathnames provided by
> > hook arguments (e.g., dev_name from sb_mount) and validate or inspect
> > the resolved paths. The verifier enforces proper resource management
> > through acquire/release tracking.
>
> Oh, *brilliant*. Thank you for giving a wonderful example of the reasons
> why this is fundamentally worthless.
>
> OK, your "BPF LSM" has been called and it got that dev_name. You decide
> that you want to know what it resolves to (which, BTW, requries a really
> non-trivial amount of parsing other arguments - just to figure out whether
> it *is* a pathname of some sort). Thanks to your shiny new kfuncs you
> can do that! You are a proud holder of mount/dentry pair. You stare at
> those and decide whether it's OK to go on. Then you... drop that pair
> and let mount(2) proceed towards the point where it will (if you parsed
> the arguments correctly) repeat that pathname resolution and get a mount/dentry
> pair of its own, that may very well be different from what you've got the
> first time around.
>
> Your primitive is a walking TOCTOU bug - it's impossible to use safely.
Good point. AFAICT, the sample TOCTOU bug applies to other LSMs that
care about dev_name in sb_mount, namely, aa_bind_mount() for apparmor
and tomoyo_mount_acl() for tomoyo.
What would you recommend to do this properly? How about we add a new
LSM hook that works on the actual mount/dentry pair? Something like:
diff --git i/fs/namespace.c w/fs/namespace.c
index d82910f33dc4..3d5dc167f15f 100644
--- i/fs/namespace.c
+++ w/fs/namespace.c
@@ -2984,6 +2984,10 @@ static int do_loopback(const struct path *path,
const char *old_name,
if (err)
return err;
+ err = security_mount_loopback(old_path, path, recurse);
+ if (err)
+ return err;
+
if (mnt_ns_loop(old_path.dentry))
return -EINVAL;
(Or s/security_mount_loopback/some_other_name).
In other words, do you think we should go [1] by Shervin Oloumi?
CCing Shervin here.
We will also need something similar to cover move mount operation
via path_mount()=>do_move_mount_old()=>do_move_mount().
Thanks,
Song
[1] https://lore.kernel.org/linux-security-module/20250110021008.2704246-1-enlightened@chromium.org/
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Al Viro @ 2025-11-30 6:46 UTC (permalink / raw)
To: Song Liu
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge, Shervin Oloumi
In-Reply-To: <CAPhsuW69nUeMf+89vwsBrwo4sv3P8xOypSfhafEu12HJKqAb+w@mail.gmail.com>
On Sat, Nov 29, 2025 at 09:57:43PM -0800, Song Liu wrote:
> > Your primitive is a walking TOCTOU bug - it's impossible to use safely.
>
> Good point. AFAICT, the sample TOCTOU bug applies to other LSMs that
> care about dev_name in sb_mount, namely, aa_bind_mount() for apparmor
> and tomoyo_mount_acl() for tomoyo.
sb_mount needs to be taken out of its misery; it makes very little sense
and it's certainly rife with TOCTOU issues.
What to replace it with is an interesting question, especially considering
how easy it is to bypass the damn thing with fsopen(), open_tree() and friends.
It certainly won't be a single hook; multiplexing thing aside, if
you look at e.g. loopback you'll see that there are two separate
operations involved - one is cloning a tree (that's where dev_name is
parsed in old API; the corresponding spot in the new one is open_tree()
with OPEN_TREE_CLONE in flags) and another - attaching that tree to
destination (move_mount(2) in the new API).
The former is "what", the latter - "where". And in open_tree()/move_mount()
it literally could be done by different processes - there's no problem
with open_tree() in one process, passing the resulting descriptor to
another process that will attach it.
Any checks you do sb_mount (or in your mount_loopback) would have
to have equivalent counterparts in those, or you get an easy way to
bypass them.
That's a very unpleasant can of worms; if you want to open it, be my
guest, but I would seriously suggest doing that after the end of merge
window - and going over the existing LSMs to see what they are trying to
do in that area before starting that thread. And yes, that's an example
of the reasons why I'm very sceptical about out-of-tree modules in
that area - with API in that state, we have no realistic way to promise
any kind of stability, with obvious consequences for everyone we can't
even see.
^ permalink raw reply
* [PATCH] tpm2-sessions: address out-of-range indexing
From: Jarkko Sakkinen @ 2025-11-30 21:18 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, stable, Peter Huewe, Jason Gunthorpe,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel, linux-kernel,
keyrings, linux-security-module
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID.
Address the issue by:
1. Rename 'name_size' as 'tpm2_name_size' so that it is bit easier to
recognize and make it a fallible function.
2. Check for only known algorithms in 'tpm2_name_size'. Return -EINVAL for
unrecognized algorithms.
3. In order to correctly propagate possible errors make also
'tpm2_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible and
address their possible errors at the call sites.
Cc: stable@vger.kernel.org # v6.10+
Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
drivers/char/tpm/tpm2-cmd.c | 23 ++++-
drivers/char/tpm/tpm2-sessions.c | 108 ++++++++++++++--------
include/linux/tpm.h | 7 +-
security/keys/trusted-keys/trusted_tpm2.c | 38 ++++++--
4 files changed, 125 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 5b6ccf901623..e63254135a74 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,7 +187,12 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (!disable_pcr_integrity) {
- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -202,8 +207,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
chip->allocated_banks[i].digest_size);
}
- if (!disable_pcr_integrity)
- tpm_buf_fill_hmac_session(chip, &buf);
+ if (!disable_pcr_integrity) {
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
if (!disable_pcr_integrity)
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
@@ -261,7 +272,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
| TPM2_SA_CONTINUE_SESSION,
NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
- tpm_buf_fill_hmac_session(chip, &buf);
+
+ err = tpm_buf_fill_hmac_session(chip, &buf);
+ if (err)
+ goto out;
+
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..82b9d9096fd1 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -141,19 +141,28 @@ struct tpm2_auth {
};
#ifdef CONFIG_TCG_TPM2_HMAC
+
/*
- * Name Size based on TPM algorithm (assumes no hash bigger than 255)
+ * Calculate size of the TPMT_HA payload of TPM2B_NAME.
*/
-static u8 name_size(const u8 *name)
+static int tpm2_name_size(const u8 *name)
{
- static u8 size_map[] = {
- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
- };
- u16 alg = get_unaligned_be16(name);
- return size_map[alg] + 2;
+ u16 hash_alg = get_unaligned_be16(name);
+
+ switch (hash_alg) {
+ case TPM_ALG_SHA1:
+ return SHA1_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA256:
+ return SHA256_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA384:
+ return SHA384_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA512:
+ return SHA512_DIGEST_SIZE + 2;
+ case TPM_ALG_SM3_256:
+ return SM3256_DIGEST_SIZE + 2;
+ }
+
+ return -EINVAL;
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +170,7 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
struct tpm_header *head = (struct tpm_header *)buf->data;
off_t offset = TPM_HEADER_SIZE;
u32 tot_len = be32_to_cpu(head->length);
+ int name_size_alg;
u32 val;
/* we're starting after the header so adjust the length */
@@ -172,9 +182,15 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
return -EINVAL;
offset += val;
/* name */
+
val = tpm_buf_read_u16(buf, &offset);
- if (val != name_size(&buf->data[offset]))
+ name_size_alg = tpm2_name_size(&buf->data[offset]);
+ if (name_size_alg < 0)
+ return name_size_alg;
+
+ if (val != name_size_alg)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -222,46 +238,59 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name)
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name)
{
#ifdef CONFIG_TCG_TPM2_HMAC
enum tpm2_mso_type mso = tpm2_handle_mso(handle);
struct tpm2_auth *auth;
+ int name_size;
int slot;
+ int ret;
#endif
if (!tpm2_chip_auth(chip)) {
tpm_buf_append_handle(chip, buf, handle);
- return;
+ return 0;
}
#ifdef CONFIG_TCG_TPM2_HMAC
slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
if (slot >= AUTH_MAX_NAMES) {
- dev_err(&chip->dev, "TPM: too many handles\n");
- return;
+ dev_err(&chip->dev, "too many handles\n");
+ return -ENOMEM;
}
auth = chip->auth;
- WARN(auth->session != tpm_buf_length(buf),
- "name added in wrong place\n");
+ if (auth->session != tpm_buf_length(buf)) {
+ dev_err(&chip->dev, "session state malformed");
+ return -EIO;
+ }
tpm_buf_append_u32(buf, handle);
auth->session += 4;
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- if (!name)
- tpm2_read_public(chip, handle, auth->name[slot]);
+ if (!name) {
+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
+ if (ret)
+ return tpm_ret_to_err(ret);
+ }
} else {
if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ return -EINVAL;
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ name_size = tpm2_name_size(name);
+ if (name_size < 0)
+ return name_size;
+
+ memcpy(auth->name[slot], name, name_size);
+ }
#endif
+ return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -537,7 +566,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
{
u32 cc, handles, val;
struct tpm2_auth *auth = chip->auth;
@@ -549,9 +578,10 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u8 cphash[SHA256_DIGEST_SIZE];
struct sha256_ctx sctx;
struct hmac_sha256_ctx hctx;
+ int name_size;
if (!auth)
- return;
+ return -EINVAL;
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -559,10 +589,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
cc = be32_to_cpu(head->ordinal);
i = tpm2_find_cc(chip, cc);
- if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
- }
+ if (i < 0)
+ return -EINVAL;
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +605,8 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u32 handle = tpm_buf_read_u32(buf, &offset_s);
if (auth->name_h[i] != handle) {
- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
- i);
- return;
+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
+ return -EINVAL;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +637,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
offset_s += len;
}
if (offset_s != offset_p) {
- dev_err(&chip->dev, "TPM session length is incorrect\n");
- return;
+ dev_err(&chip->dev, "session length is incorrect\n");
+ return -EINVAL;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ return -EINVAL;
}
/* encrypt before HMAC */
@@ -646,8 +674,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- sha256_update(&sctx, auth->name[i],
- name_size(auth->name[i]));
+ name_size = tpm2_name_size(auth->name[i]);
+ if (name_size < 0)
+ return name_size;
+
+ sha256_update(&sctx, auth->name[i], name_size);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +699,7 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
hmac_sha256_update(&hctx, &auth->attrs, 1);
hmac_sha256_final(&hctx, hmac);
+ return 0;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0e9e043f728c..f168c547abae 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -413,6 +413,7 @@ enum tpm2_session_attributes {
struct tpm2_hash {
unsigned int crypto_id;
unsigned int tpm_id;
+ unsigned int hash_size;
};
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
@@ -528,8 +529,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
#endif
}
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name);
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name);
void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
u8 attributes, u8 *passphrase,
int passphraselen);
@@ -562,7 +563,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
#ifdef CONFIG_TCG_TPM2_HMAC
int tpm2_start_auth_session(struct tpm_chip *chip);
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
int rc);
void tpm2_end_auth_session(struct tpm_chip *chip);
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index e165b117bbca..33544b6bc105 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -283,7 +283,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out_put;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(chip);
+ goto out_put;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -331,7 +337,12 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ goto out;
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -438,7 +449,12 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -450,7 +466,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (!rc)
@@ -497,7 +516,11 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -522,7 +545,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
NULL, 0);
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
--
2.52.0
^ permalink raw reply related
* [PATCH v2] tpm2-sessions: address out-of-range indexing
From: Jarkko Sakkinen @ 2025-11-30 21:35 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, stable, Peter Huewe, Jason Gunthorpe,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel, linux-kernel,
keyrings, linux-security-module
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID.
Address the issue by:
1. Rename 'name_size' as 'tpm2_name_size' so that it is bit easier to
recognize and make it a fallible function.
2. Check for only known algorithms in 'tpm2_name_size'. Return -EINVAL for
unrecognized algorithms.
3. In order to correctly propagate possible errors make also
'tpm2_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible and
address their possible errors at the call sites.
Cc: stable@vger.kernel.org # v6.10+
Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
There was spurious extra field added to tpm2_hash by mistake.
drivers/char/tpm/tpm2-cmd.c | 23 ++++-
drivers/char/tpm/tpm2-sessions.c | 108 ++++++++++++++--------
include/linux/tpm.h | 6 +-
security/keys/trusted-keys/trusted_tpm2.c | 38 ++++++--
4 files changed, 124 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 5b6ccf901623..e63254135a74 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,7 +187,12 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (!disable_pcr_integrity) {
- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -202,8 +207,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
chip->allocated_banks[i].digest_size);
}
- if (!disable_pcr_integrity)
- tpm_buf_fill_hmac_session(chip, &buf);
+ if (!disable_pcr_integrity) {
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
if (!disable_pcr_integrity)
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
@@ -261,7 +272,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
| TPM2_SA_CONTINUE_SESSION,
NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
- tpm_buf_fill_hmac_session(chip, &buf);
+
+ err = tpm_buf_fill_hmac_session(chip, &buf);
+ if (err)
+ goto out;
+
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..82b9d9096fd1 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -141,19 +141,28 @@ struct tpm2_auth {
};
#ifdef CONFIG_TCG_TPM2_HMAC
+
/*
- * Name Size based on TPM algorithm (assumes no hash bigger than 255)
+ * Calculate size of the TPMT_HA payload of TPM2B_NAME.
*/
-static u8 name_size(const u8 *name)
+static int tpm2_name_size(const u8 *name)
{
- static u8 size_map[] = {
- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
- };
- u16 alg = get_unaligned_be16(name);
- return size_map[alg] + 2;
+ u16 hash_alg = get_unaligned_be16(name);
+
+ switch (hash_alg) {
+ case TPM_ALG_SHA1:
+ return SHA1_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA256:
+ return SHA256_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA384:
+ return SHA384_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA512:
+ return SHA512_DIGEST_SIZE + 2;
+ case TPM_ALG_SM3_256:
+ return SM3256_DIGEST_SIZE + 2;
+ }
+
+ return -EINVAL;
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +170,7 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
struct tpm_header *head = (struct tpm_header *)buf->data;
off_t offset = TPM_HEADER_SIZE;
u32 tot_len = be32_to_cpu(head->length);
+ int name_size_alg;
u32 val;
/* we're starting after the header so adjust the length */
@@ -172,9 +182,15 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
return -EINVAL;
offset += val;
/* name */
+
val = tpm_buf_read_u16(buf, &offset);
- if (val != name_size(&buf->data[offset]))
+ name_size_alg = tpm2_name_size(&buf->data[offset]);
+ if (name_size_alg < 0)
+ return name_size_alg;
+
+ if (val != name_size_alg)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -222,46 +238,59 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name)
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name)
{
#ifdef CONFIG_TCG_TPM2_HMAC
enum tpm2_mso_type mso = tpm2_handle_mso(handle);
struct tpm2_auth *auth;
+ int name_size;
int slot;
+ int ret;
#endif
if (!tpm2_chip_auth(chip)) {
tpm_buf_append_handle(chip, buf, handle);
- return;
+ return 0;
}
#ifdef CONFIG_TCG_TPM2_HMAC
slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
if (slot >= AUTH_MAX_NAMES) {
- dev_err(&chip->dev, "TPM: too many handles\n");
- return;
+ dev_err(&chip->dev, "too many handles\n");
+ return -ENOMEM;
}
auth = chip->auth;
- WARN(auth->session != tpm_buf_length(buf),
- "name added in wrong place\n");
+ if (auth->session != tpm_buf_length(buf)) {
+ dev_err(&chip->dev, "session state malformed");
+ return -EIO;
+ }
tpm_buf_append_u32(buf, handle);
auth->session += 4;
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- if (!name)
- tpm2_read_public(chip, handle, auth->name[slot]);
+ if (!name) {
+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
+ if (ret)
+ return tpm_ret_to_err(ret);
+ }
} else {
if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ return -EINVAL;
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ name_size = tpm2_name_size(name);
+ if (name_size < 0)
+ return name_size;
+
+ memcpy(auth->name[slot], name, name_size);
+ }
#endif
+ return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -537,7 +566,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
{
u32 cc, handles, val;
struct tpm2_auth *auth = chip->auth;
@@ -549,9 +578,10 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u8 cphash[SHA256_DIGEST_SIZE];
struct sha256_ctx sctx;
struct hmac_sha256_ctx hctx;
+ int name_size;
if (!auth)
- return;
+ return -EINVAL;
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -559,10 +589,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
cc = be32_to_cpu(head->ordinal);
i = tpm2_find_cc(chip, cc);
- if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
- }
+ if (i < 0)
+ return -EINVAL;
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +605,8 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u32 handle = tpm_buf_read_u32(buf, &offset_s);
if (auth->name_h[i] != handle) {
- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
- i);
- return;
+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
+ return -EINVAL;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +637,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
offset_s += len;
}
if (offset_s != offset_p) {
- dev_err(&chip->dev, "TPM session length is incorrect\n");
- return;
+ dev_err(&chip->dev, "session length is incorrect\n");
+ return -EINVAL;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ return -EINVAL;
}
/* encrypt before HMAC */
@@ -646,8 +674,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- sha256_update(&sctx, auth->name[i],
- name_size(auth->name[i]));
+ name_size = tpm2_name_size(auth->name[i]);
+ if (name_size < 0)
+ return name_size;
+
+ sha256_update(&sctx, auth->name[i], name_size);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +699,7 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
hmac_sha256_update(&hctx, &auth->attrs, 1);
hmac_sha256_final(&hctx, hmac);
+ return 0;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0e9e043f728c..1a59f0190eb3 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -528,8 +528,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
#endif
}
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name);
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name);
void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
u8 attributes, u8 *passphrase,
int passphraselen);
@@ -562,7 +562,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
#ifdef CONFIG_TCG_TPM2_HMAC
int tpm2_start_auth_session(struct tpm_chip *chip);
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
int rc);
void tpm2_end_auth_session(struct tpm_chip *chip);
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index e165b117bbca..33544b6bc105 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -283,7 +283,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out_put;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(chip);
+ goto out_put;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -331,7 +337,12 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ goto out;
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -438,7 +449,12 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -450,7 +466,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (!rc)
@@ -497,7 +516,11 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -522,7 +545,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
NULL, 0);
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
--
2.52.0
^ permalink raw reply related
* [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: NeilBrown @ 2025-11-30 22:06 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Val Packett
Cc: Amir Goldstein, Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason,
David Sterba, David Howells, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Tyler Hicks, Miklos Szeredi,
Chuck Lever, Olga Kornievskaia, Dai Ngo, Namjae Jeon,
Steve French, Sergey Senozhatsky, Carlos Maiolino, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Ondrej Mosnacek, Mateusz Guzik, Lorenzo Stoakes, Stefan Berger,
Darrick J. Wong, linux-kernel, netfs, ecryptfs, linux-nfs,
linux-unionfs, linux-cifs, linux-xfs, linux-security-module,
selinux
In-Reply-To: <6713ea38-b583-4c86-b74a-bea55652851d@packett.cool>
From: NeilBrown <neil@brown.name>
The recent conversion of fuse_reverse_inval_entry() to use
start_removing() was wrong.
As Val Packett points out the original code did not call ->lookup
while the new code does. This can lead to a deadlock.
Rather than using full_name_hash() and d_lookup() as the old code
did, we can use try_lookup_noperm() which combines these. Then
the result can be given to start_removing_dentry() to get the required
locks for removal. We then double check that the name hasn't
changed.
As 'dir' needs to be used several times now, we load the dput() until
the end, and initialise to NULL so dput() is always safe.
Reported-by: Val Packett <val@packett.cool>
Closes: https://lore.kernel.org/all/6713ea38-b583-4c86-b74a-bea55652851d@packett.cool
Fixes: c9ba789dad15 ("VFS: introduce start_creating_noperm() and start_removing_noperm()")
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/fuse/dir.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index a0d5b302bcc2..8384fa96cf53 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1390,8 +1390,8 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
{
int err = -ENOTDIR;
struct inode *parent;
- struct dentry *dir;
- struct dentry *entry;
+ struct dentry *dir = NULL;
+ struct dentry *entry = NULL;
parent = fuse_ilookup(fc, parent_nodeid, NULL);
if (!parent)
@@ -1404,11 +1404,19 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
dir = d_find_alias(parent);
if (!dir)
goto put_parent;
-
- entry = start_removing_noperm(dir, name);
- dput(dir);
- if (IS_ERR(entry))
- goto put_parent;
+ while (!entry) {
+ struct dentry *child = try_lookup_noperm(name, dir);
+ if (!child || IS_ERR(child))
+ goto put_parent;
+ entry = start_removing_dentry(dir, child);
+ dput(child);
+ if (IS_ERR(entry))
+ goto put_parent;
+ if (!d_same_name(entry, dir, name)) {
+ end_removing(entry);
+ entry = NULL;
+ }
+ }
fuse_dir_changed(parent);
if (!(flags & FUSE_EXPIRE_ONLY))
@@ -1446,6 +1454,7 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
end_removing(entry);
put_parent:
+ dput(dir);
iput(parent);
return err;
}
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH] selftests/landlock: Remove invalid unix socket bind()
From: Matthieu Buffet @ 2025-12-01 0:36 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, linux-security-module, Matthieu Buffet
Remove bind() call on a client socket that doesn't make sense.
Since strlen(cli_un.sun_path) returns a random value depending on stack
garbage, that many uninitialized bytes are read from the stack as an
unix socket address. This creates random test failures due to the bind
address being invalid or already in use if the same stack value comes up
twice.
Fixes: f83d51a5bdfe ("selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets")
Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
tools/testing/selftests/landlock/fs_test.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index eee814e09dd7..7d378bdf3bce 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4391,9 +4391,6 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
ASSERT_LE(0, cli_fd);
- size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
- ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
-
bzero(&cli_un, sizeof(cli_un));
cli_un.sun_family = AF_UNIX;
strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
base-commit: 54f9baf537b0a091adad860ec92e3e18e0a0754c
--
2.47.3
^ permalink raw reply related
* Re: [PATCH v2 1/2] evm: fix security.evm for a file with IMA signature
From: Coiby Xu @ 2025-12-01 3:15 UTC (permalink / raw)
To: linux-integrity, Mimi Zohar
Cc: Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E. Hallyn, open list,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250930022658.4033410-1-coxu@redhat.com>
On Tue, Sep 30, 2025 at 10:26:56AM +0800, Coiby Xu wrote:
>When both IMA and EVM fix modes are enabled, accessing a file with IMA
>signature but missing EVM HMAC won't cause security.evm to be fixed.
>
>Add a function evm_fix_hmac which will be explicitly called to fix EVM
>HMAC for this case.
>
>Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
>Signed-off-by: Coiby Xu <coxu@redhat.com>
>---
> include/linux/evm.h | 8 ++++++++
> security/integrity/evm/evm_main.c | 28 +++++++++++++++++++++++++++
> security/integrity/ima/ima_appraise.c | 5 +++++
> 3 files changed, 41 insertions(+)
>
>diff --git a/include/linux/evm.h b/include/linux/evm.h
>index ddece4a6b25d..913f4573b203 100644
>--- a/include/linux/evm.h
>+++ b/include/linux/evm.h
>@@ -18,6 +18,8 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
> const char *xattr_name,
> void *xattr_value,
> size_t xattr_value_len);
>+int evm_fix_hmac(struct dentry *dentry, const char *xattr_name,
>+ const char *xattr_value, size_t xattr_value_len);
> int evm_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr, struct xattr *xattrs,
> int *xattr_count);
>@@ -51,6 +53,12 @@ static inline enum integrity_status evm_verifyxattr(struct dentry *dentry,
> {
> return INTEGRITY_UNKNOWN;
> }
>+
>+static inline int evm_fix_hmac(struct dentry *dentry, const char *xattr_name,
>+ const char *xattr_value, size_t xattr_value_len)
>+{
>+ return -EOPNOTSUPP;
>+}
> #endif
>
> static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
>diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
>index 0add782e73ba..1b3edc6d26e9 100644
>--- a/security/integrity/evm/evm_main.c
>+++ b/security/integrity/evm/evm_main.c
>@@ -787,6 +787,34 @@ bool evm_revalidate_status(const char *xattr_name)
> return true;
> }
>
>+/**
>+ * evm_fix_hmac - Calculate the HMAC and add it to security.evm for fix mode
>+ * @dentry: pointer to the affected dentry which doesn't yet have security.evm
>+ * xattr
>+ * @xattr_name: pointer to the affected extended attribute name
>+ * @xattr_value: pointer to the new extended attribute value
>+ * @xattr_value_len: pointer to the new extended attribute value length
>+ *
>+ * Expects to be called with i_mutex locked.
>+ *
>+ * Return: 0 on success, -EPERM/-ENOMEM/-EOPNOTSUPP on failure
>+ */
>+int evm_fix_hmac(struct dentry *dentry, const char *xattr_name,
>+ const char *xattr_value, size_t xattr_value_len)
>+
>+{
>+ if (!evm_fixmode || !evm_revalidate_status((xattr_name)))
>+ return -EPERM;
>+
>+ if (!(evm_initialized & EVM_INIT_HMAC))
>+ return -EPERM;
>+
>+ if (is_unsupported_hmac_fs(dentry))
>+ return -EOPNOTSUPP;
>+
>+ return evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
>+}
>+
> /**
> * evm_inode_post_setxattr - update 'security.evm' to reflect the changes
> * @dentry: pointer to the affected dentry
>diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>index f435eff4667f..f48ef5ec185e 100644
>--- a/security/integrity/ima/ima_appraise.c
>+++ b/security/integrity/ima/ima_appraise.c
>@@ -601,6 +601,11 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> if (!ima_fix_xattr(dentry, iint))
> status = INTEGRITY_PASS;
>+ } else if (status == INTEGRITY_NOLABEL) {
>+ if (!evm_fix_hmac(dentry, XATTR_NAME_IMA,
>+ (const char *)xattr_value,
>+ xattr_len))
>+ status = INTEGRITY_PASS;
> }
>
> /*
>
>base-commit: e129e479f2e444eaccd822717d418119d39d3d5c
>--
>2.51.0
>
Hi Mimi,
I think this patch set just fell off the radar. Can you take a look at
it when time permits? Thanks! Btw, the patch set is still applicable to
current next-integrity tree Linus and main tree.
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] rust: security: use `pin_init::zeroed()` for LSM context initialization
From: Alexandre Courbot @ 2025-12-01 3:39 UTC (permalink / raw)
To: Atharv Dubey, paul, jmorris, serge, ojeda, alex.gaynor
Cc: boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, dakr, linux-security-module, rust-for-linux,
linux-kernel
In-Reply-To: <20251129135657.36144-1-atharvd440@gmail.com>
On Sat Nov 29, 2025 at 10:56 PM JST, Atharv Dubey wrote:
> Replace the previous `unsafe { core::mem::zeroed() }` initialization of
> `bindings::lsm_context` with `pin_init::zeroed()`.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1189
> Signed-off-by: Atharv Dubey <atharvd440@gmail.com>
> ---
> rust/kernel/security.rs | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
> index 9d271695265f..4dc3eba6ce84 100644
> --- a/rust/kernel/security.rs
> +++ b/rust/kernel/security.rs
> @@ -62,8 +62,7 @@ impl SecurityCtx {
> /// Get the security context given its id.
> #[inline]
> pub fn from_secid(secid: u32) -> Result<Self> {
> - // SAFETY: `struct lsm_context` can be initialized to all zeros.
> - let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
> + let mut ctx: bindings::lsm_context = pin_init::zeroed();
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Song Liu @ 2025-12-01 7:32 UTC (permalink / raw)
To: Al Viro
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge, Shervin Oloumi
In-Reply-To: <20251130064609.GR3538@ZenIV>
On Sat, Nov 29, 2025 at 10:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Nov 29, 2025 at 09:57:43PM -0800, Song Liu wrote:
>
> > > Your primitive is a walking TOCTOU bug - it's impossible to use safely.
> >
> > Good point. AFAICT, the sample TOCTOU bug applies to other LSMs that
> > care about dev_name in sb_mount, namely, aa_bind_mount() for apparmor
> > and tomoyo_mount_acl() for tomoyo.
>
> sb_mount needs to be taken out of its misery; it makes very little sense
> and it's certainly rife with TOCTOU issues.
>
> What to replace it with is an interesting question, especially considering
> how easy it is to bypass the damn thing with fsopen(), open_tree() and friends.
>
> It certainly won't be a single hook; multiplexing thing aside, if
> you look at e.g. loopback you'll see that there are two separate
> operations involved - one is cloning a tree (that's where dev_name is
> parsed in old API; the corresponding spot in the new one is open_tree()
> with OPEN_TREE_CLONE in flags) and another - attaching that tree to
> destination (move_mount(2) in the new API).
We currently have security_move_mount(), security_sb_remount(), and
security_sb_kern_mount(), so most things are somewhat covered.
> The former is "what", the latter - "where". And in open_tree()/move_mount()
> it literally could be done by different processes - there's no problem
> with open_tree() in one process, passing the resulting descriptor to
> another process that will attach it.
For open_tree, security_file_open() can cover the "what" part of it.
>
> Any checks you do sb_mount (or in your mount_loopback) would have
> to have equivalent counterparts in those, or you get an easy way to
> bypass them.
>
> That's a very unpleasant can of worms; if you want to open it, be my
> guest, but I would seriously suggest doing that after the end of merge
> window - and going over the existing LSMs to see what they are trying to
> do in that area before starting that thread.
I very much support fixing it properly, and I don't plan to rush into a
half broken workaround. I am not very optimistic about whether
we can bring everyone to the same page, but I think it is worth a try.
> And yes, that's an example
> of the reasons why I'm very sceptical about out-of-tree modules in
> that area - with API in that state, we have no realistic way to promise
> any kind of stability, with obvious consequences for everyone we can't
> even see.
I am not sure what you mean by "with API in that state". Which API
sets are you talking about: the LSM hooks, the BPF kfuncs in
bpf_fs_kfuncs.c, or all the exported symbols that are available to in-tree
and out-of-tree LSMs? And how would you suggest we make these
APIs into a better state?
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: Amir Goldstein @ 2025-12-01 8:22 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Val Packett, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <176454037897.634289.3566631742434963788@noble.neil.brown.name>
On Sun, Nov 30, 2025 at 11:06 PM NeilBrown <neilb@ownmail.net> wrote:
>
>
> From: NeilBrown <neil@brown.name>
>
> The recent conversion of fuse_reverse_inval_entry() to use
> start_removing() was wrong.
> As Val Packett points out the original code did not call ->lookup
> while the new code does. This can lead to a deadlock.
>
> Rather than using full_name_hash() and d_lookup() as the old code
> did, we can use try_lookup_noperm() which combines these. Then
> the result can be given to start_removing_dentry() to get the required
> locks for removal. We then double check that the name hasn't
> changed.
>
> As 'dir' needs to be used several times now, we load the dput() until
> the end, and initialise to NULL so dput() is always safe.
>
> Reported-by: Val Packett <val@packett.cool>
> Closes: https://lore.kernel.org/all/6713ea38-b583-4c86-b74a-bea55652851d@packett.cool
> Fixes: c9ba789dad15 ("VFS: introduce start_creating_noperm() and start_removing_noperm()")
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/fuse/dir.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index a0d5b302bcc2..8384fa96cf53 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1390,8 +1390,8 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> {
> int err = -ENOTDIR;
> struct inode *parent;
> - struct dentry *dir;
> - struct dentry *entry;
> + struct dentry *dir = NULL;
> + struct dentry *entry = NULL;
>
> parent = fuse_ilookup(fc, parent_nodeid, NULL);
> if (!parent)
> @@ -1404,11 +1404,19 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> dir = d_find_alias(parent);
> if (!dir)
> goto put_parent;
> -
> - entry = start_removing_noperm(dir, name);
> - dput(dir);
> - if (IS_ERR(entry))
> - goto put_parent;
> + while (!entry) {
> + struct dentry *child = try_lookup_noperm(name, dir);
> + if (!child || IS_ERR(child))
> + goto put_parent;
> + entry = start_removing_dentry(dir, child);
> + dput(child);
> + if (IS_ERR(entry))
> + goto put_parent;
> + if (!d_same_name(entry, dir, name)) {
> + end_removing(entry);
> + entry = NULL;
> + }
> + }
Can you explain why it is so important to use
start_removing_dentry() around shrink_dcache_parent()?
Is there a problem with reverting the change in this function
instead of accomodating start_removing_dentry()?
I don't think there is a point in optimizing parallel dir operations
with FUSE server cache invalidation, but maybe I am missing
something.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: Al Viro @ 2025-12-01 8:33 UTC (permalink / raw)
To: Amir Goldstein
Cc: NeilBrown, Christian Brauner, Val Packett, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <CAOQ4uxjihcBxJzckbJis8hGcWO61QKhiqeGH+hDkTUkDhu23Ww@mail.gmail.com>
On Mon, Dec 01, 2025 at 09:22:54AM +0100, Amir Goldstein wrote:
> I don't think there is a point in optimizing parallel dir operations
> with FUSE server cache invalidation, but maybe I am missing
> something.
The interesting part is the expected semantics of operation;
d_invalidate() side definitely doesn't need any of that cruft,
but I would really like to understand what that function
is supposed to do.
Miklos, could you post a brain dump on that?
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: NeilBrown @ 2025-12-01 8:50 UTC (permalink / raw)
To: Amir Goldstein
Cc: Alexander Viro, Christian Brauner, Val Packett, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <CAOQ4uxjihcBxJzckbJis8hGcWO61QKhiqeGH+hDkTUkDhu23Ww@mail.gmail.com>
On Mon, 01 Dec 2025, Amir Goldstein wrote:
> On Sun, Nov 30, 2025 at 11:06 PM NeilBrown <neilb@ownmail.net> wrote:
> >
> >
> > From: NeilBrown <neil@brown.name>
> >
> > The recent conversion of fuse_reverse_inval_entry() to use
> > start_removing() was wrong.
> > As Val Packett points out the original code did not call ->lookup
> > while the new code does. This can lead to a deadlock.
> >
> > Rather than using full_name_hash() and d_lookup() as the old code
> > did, we can use try_lookup_noperm() which combines these. Then
> > the result can be given to start_removing_dentry() to get the required
> > locks for removal. We then double check that the name hasn't
> > changed.
> >
> > As 'dir' needs to be used several times now, we load the dput() until
> > the end, and initialise to NULL so dput() is always safe.
> >
> > Reported-by: Val Packett <val@packett.cool>
> > Closes: https://lore.kernel.org/all/6713ea38-b583-4c86-b74a-bea55652851d@packett.cool
> > Fixes: c9ba789dad15 ("VFS: introduce start_creating_noperm() and start_removing_noperm()")
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/fuse/dir.c | 23 ++++++++++++++++-------
> > 1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index a0d5b302bcc2..8384fa96cf53 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1390,8 +1390,8 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> > {
> > int err = -ENOTDIR;
> > struct inode *parent;
> > - struct dentry *dir;
> > - struct dentry *entry;
> > + struct dentry *dir = NULL;
> > + struct dentry *entry = NULL;
> >
> > parent = fuse_ilookup(fc, parent_nodeid, NULL);
> > if (!parent)
> > @@ -1404,11 +1404,19 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> > dir = d_find_alias(parent);
> > if (!dir)
> > goto put_parent;
> > -
> > - entry = start_removing_noperm(dir, name);
> > - dput(dir);
> > - if (IS_ERR(entry))
> > - goto put_parent;
> > + while (!entry) {
> > + struct dentry *child = try_lookup_noperm(name, dir);
> > + if (!child || IS_ERR(child))
> > + goto put_parent;
> > + entry = start_removing_dentry(dir, child);
> > + dput(child);
> > + if (IS_ERR(entry))
> > + goto put_parent;
> > + if (!d_same_name(entry, dir, name)) {
> > + end_removing(entry);
> > + entry = NULL;
> > + }
> > + }
>
> Can you explain why it is so important to use
> start_removing_dentry() around shrink_dcache_parent()?
Is it shrink_dcache_parent() that is being protected? or d_delete()? or
....
Why was the original code locking the parent inode? Whatever that was
protecting, we need to keep protecting it. That is what
start_removing_dentry() is there to do.
>
> Is there a problem with reverting the change in this function
> instead of accomodating start_removing_dentry()?
Yes. I want to change the rules for protecting dentries. Ultimately
the vfs won't take the parent lock except for readdir. Individual
filesystems can take the lock if they want to, but the VFS won't care.
To do that, we need to centralise all locking of the parent so we can
smoothly change it.
The next change - after this current series has all the problems ironed
out - is to switch the order of d_alloc_parallel() and
inode_lock(parent).
Currently d_alloc_parallel() can wait while holding the parent lock. I
need to change that so that the parent lock can be taken while holding
a d_in_lookup() dentry (which will block an conflicting
d_alloc_parallel()).
I guess I don't strictly need to remove inode_lock() from this code for
that as it doesn't do a lookup, but there will be a patch set which will
need to change the locking here. It will be much cleaner if the locking
is centralised.
>
> I don't think there is a point in optimizing parallel dir operations
> with FUSE server cache invalidation, but maybe I am missing
> something.
This isn't about supporting parallel dir ops everywhere. This is about
refactoring code so that we can cleanly support parallel dir ops
anywhere.
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: Al Viro @ 2025-12-01 8:56 UTC (permalink / raw)
To: NeilBrown
Cc: Amir Goldstein, Christian Brauner, Val Packett, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <176457904303.16766.13791656192264803692@noble.neil.brown.name>
On Mon, Dec 01, 2025 at 07:50:43PM +1100, NeilBrown wrote:
> Why was the original code locking the parent inode? Whatever that was
> protecting, we need to keep protecting it. That is what
> start_removing_dentry() is there to do.
We need to find out what it's protecting, rather than cargo-culting it
indefinitely. If nothing else, it's a place with uncommon use of
inode_lock; we need to know which properties of current locking
scheme does it expect there. Thus the question to Miklos...
^ permalink raw reply
* Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`
From: Oliver Mangold @ 2025-12-01 10:23 UTC (permalink / raw)
To: Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Benno Lossin, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <A5A7C4C9-1504-439C-B4FF-C28482AF7444@collabora.com>
On 251128 1506, Daniel Almeida wrote:
> > /// Type allocated and destroyed on the C side, but owned by Rust.
> > ///
> > -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> > -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> > -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> > -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> > +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
> > +/// - This is useful when it is desirable to tie the lifetime of an object reference to an owned
> > +/// object, rather than pass around a bare reference.
> > +/// - [`Ownable`] types can define custom drop logic that is executed when the owned reference
> > +/// of type [`Owned<_>`] pointing to the object is dropped.
> > ///
> > /// Note: The underlying object is not required to provide internal reference counting, because it
> > /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> > -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> > +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
> > +/// between unique and shared (reference counted) ownership is needed.
> > ///
> > /// # Safety
> > ///
> > @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
> > /// mutable reference requirements. That is, the kernel will not mutate or free the underlying
> > /// object and is okay with it being modified by Rust code.
> > pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > - Self {
> > - ptr,
> > - }
> > + Self { ptr }
> > }
>
> Unrelated change?
Ah, yes, rustfmt must I done that, and I missed it. Will fix.
> > +///
> > +/// impl OwnableRefCounted for Foo {
> > +/// fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> > +/// if this.refcount.get() == 1 {
> > +/// // SAFETY: The `Foo` is still alive and has no other Rust references as the refcount
> > +/// // is 1.
> > +/// Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> > +/// } else {
> > +/// Err(this)
> > +/// }
> > +/// }
> > +/// }
> > +///
>
> We wouldn’t need this implementation if we added a “refcount()”
> member to this trait. This lets you abstract away this logic for all
> implementors, which has the massive upside of making sure we hardcode (and thus
> enforce) the refcount == 1 check.
This wouldn't work for the block `Request` use case. There a reference can
be acquired "out of thin air" using a `TagSet`. Thus "check for unique
refcount" + "create an owned reference" needs to be one atomic operation.
Also I think it might be generally problematic to require a refcount()
function. The API of the underlying kernel object we want to wrap might not
offer that, so we would need to access internal data.
> > +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
> > +/// unsafe impl Ownable for Foo {
> > +/// unsafe fn release(this: NonNull<Self>) {
> > +/// // SAFETY: Using `dec_ref()` from [`RefCounted`] to release is okay, as the refcount is
> > +/// // always 1 for an [`Owned<Foo>`].
> > +/// unsafe{ Foo::dec_ref(this) };
> > +/// }
> > +/// }
> > +///
> > +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
>
> All these “expects()” and custom error strings would go away if you
> place this behind a fictional function that returns Result.
Not sure what you mean by fictional function. Do you mean a non-existent
function? We want to compile this code as a unit test.
The rest of your suggested changes make sense, I guess. I will implement
them.
Thanks,
Oliver
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: Miklos Szeredi @ 2025-12-01 14:03 UTC (permalink / raw)
To: Al Viro
Cc: Amir Goldstein, NeilBrown, Christian Brauner, Val Packett,
Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Chuck Lever, Olga Kornievskaia,
Dai Ngo, Namjae Jeon, Steve French, Sergey Senozhatsky,
Carlos Maiolino, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <20251201083324.GA3538@ZenIV>
[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]
On Mon, 1 Dec 2025 at 09:33, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Dec 01, 2025 at 09:22:54AM +0100, Amir Goldstein wrote:
>
> > I don't think there is a point in optimizing parallel dir operations
> > with FUSE server cache invalidation, but maybe I am missing
> > something.
>
> The interesting part is the expected semantics of operation;
> d_invalidate() side definitely doesn't need any of that cruft,
> but I would really like to understand what that function
> is supposed to do.
>
> Miklos, could you post a brain dump on that?
This function is supposed to invalidate a dentry due to remote changes
(FUSE_NOTIFY_INVAL_ENTRY). Originally it was supplied a parent ID and
a name and called d_invalidate() on the looked up dentry.
Then it grew a variant (FUSE_NOTIFY_DELETE) that was also supplied a
child ID, which was matched against the looked up inode. This was
commit 451d0f599934 ("FUSE: Notifying the kernel of deletion."),
Apparently this worked around the fact that at that time
d_invalidate() returned -EBUSY if the target was still in use and
didn't unhash the dentry in that case.
That was later changed by commit bafc9b754f75 ("vfs: More precise
tests in d_invalidate") to unconditionally unhash the target, which
effectively made FUSE_NOTIFY_INVAL_ENTRY and FUSE_NOTIFY_DELETE
equivalent and the code in question unnecessary.
For the future, we could also introduce FUSE_NOTIFY_MOVE, that would
differentiate between a delete and a move, while
FUSE_NOTIFY_INVAL_ENTRY would continue to be the common (deleted or
moved) notification.
Attaching untested patch to remove this cruft.
Thanks,
Miklos
[-- Attachment #2: fuse-notify_inval_entry-and-notify_delete-are-equivalent.patch --]
[-- Type: text/x-patch, Size: 1022 bytes --]
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ecaec0fea3a1..d9dffc326a26 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1417,34 +1417,9 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
d_invalidate(entry);
fuse_invalidate_entry_cache(entry);
- if (child_nodeid != 0 && d_really_is_positive(entry)) {
- inode_lock(d_inode(entry));
- if (get_node_id(d_inode(entry)) != child_nodeid) {
- err = -ENOENT;
- goto badentry;
- }
- if (d_mountpoint(entry)) {
- err = -EBUSY;
- goto badentry;
- }
- if (d_is_dir(entry)) {
- shrink_dcache_parent(entry);
- if (!simple_empty(entry)) {
- err = -ENOTEMPTY;
- goto badentry;
- }
- d_inode(entry)->i_flags |= S_DEAD;
- }
- dont_mount(entry);
- clear_nlink(d_inode(entry));
- err = 0;
- badentry:
- inode_unlock(d_inode(entry));
- if (!err)
- d_delete(entry);
- } else {
- err = 0;
- }
+ err = 0;
+ if (child_nodeid != 0 && get_node_id(d_inode(entry)) != child_nodeid)
+ err = -ENOENT;
dput(entry);
unlock:
^ permalink raw reply related
* Re: [PATCH] tomoyo: Use local kmap in tomoyo_dump_page()
From: Tetsuo Handa @ 2025-12-01 14:21 UTC (permalink / raw)
To: Davidlohr Bueso, takedakn; +Cc: linux-security-module, linux-kernel
In-Reply-To: <20251128222747.2174688-1-dave@stgolabs.net>
On 2025/11/29 7:27, Davidlohr Bueso wrote:
> Replace the now deprecated kmap_atomic() with kmap_local_page().
>
> The memcpy does not need atomic semantics, and the removed comment
> is now stale - this patch now makes it in sync again. Last but not
> least, highmem is going to be removed[0].
>
> [0] https://lore.kernel.org/all/4ff89b72-03ff-4447-9d21-dd6a5fe1550f@app.fastmail.com/
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Thank you. Applied.
https://sourceforge.net/p/tomoyo/tomoyo.git/ci/a9ea3a2e081d29350b7a3c0731729efbc70458b8/
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Oleg Nesterov @ 2025-12-01 15:13 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Christian Brauner, Alexander Viro, Alexey Dobriyan, Kees Cook,
Andy Lutomirski, Will Drewry, Andrew Morton, Michal Hocko,
Serge Hallyn, James Morris, Randy Dunlap, Suren Baghdasaryan,
Yafang Shao, Helge Deller, Eric W. Biederman, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <GV2PPF74270EBEEDD43083BE45C6E26F674E4DDA@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
On 11/29, Bernd Edlinger wrote:
>
> On 11/23/25 19:32, Oleg Nesterov wrote:
> > I don't follow. Do you mean PREEMPT_RT ?
> >
> > If yes. In this case spin_lock_irq() is rt_spin_lock() which doesn't disable irqs,
> > it does rt_lock_lock() (takes rt_mutex) + migrate_disable().
> >
> > I do think that spin/mutex/whatever_unlock() is always safe. In any order, and
> > regardless of RT.
> >
>
> It is hard to follow how linux implements that spin_lock_irq exactly,
Yes ;)
> but
> to me it looks like it is done this way:
>
> include/linux/spinlock_api_smp.h:static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
> include/linux/spinlock_api_smp.h-{
> include/linux/spinlock_api_smp.h- local_irq_disable();
> include/linux/spinlock_api_smp.h- preempt_disable();
> include/linux/spinlock_api_smp.h- spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> include/linux/spinlock_api_smp.h- LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> include/linux/spinlock_api_smp.h-}
Again, I will assume you mean RT.
In this case spinlock_t and raw_spinlock_t are not the same thing.
include/linux/spinlock_types.h:
typedef struct spinlock {
struct rt_mutex_base lock;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
#endif
} spinlock_t;
include/linux/spinlock_rt.h:
static __always_inline void spin_lock_irq(spinlock_t *lock)
{
rt_spin_lock(lock);
}
rt_spin_lock() doesn't disable irqs, it takes "rt_mutex_base lock" and
disables migration.
> so an explicit task switch while locka_irq_disable looks
> very dangerous to me.
raw_spin_lock_irq() disables irqs/preemption regardless of RT, task switch
is not possible.
> Do you know other places where such
> a code pattern is used?
For example, double_lock_irq(). See task_numa_group(),
double_lock_irq(&my_grp->lock, &grp->lock);
....
spin_unlock(&my_grp->lock);
spin_unlock_irq(&grp->lock);
this can unlock the locks in reverse order.
I am sure there are more examples.
> I do just ask, because a close look at those might reveal
> some serious bugs, WDYT?
See above, I don't understand your concerns...
Oleg.
^ permalink raw reply
* Re: [PATCH v13 1/4] rust: types: Add Ownable/Owned types
From: Gary Guo @ 2025-12-01 15:51 UTC (permalink / raw)
To: Oliver Mangold
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Björn Roy Baron,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Benno Lossin,
Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <20251117-unique-ref-v13-1-b5b243df1250@pm.me>
On Mon, 17 Nov 2025 10:07:40 +0000
Oliver Mangold <oliver.mangold@pm.me> wrote:
> From: Asahi Lina <lina+kernel@asahilina.net>
>
> By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
> (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
> `AlwaysRefCounted`, this mechanism expects the reference to be unique
> within Rust, and does not allow cloning.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> [ om:
> - Split code into separate file and `pub use` it from types.rs.
> - Make from_raw() and into_raw() public.
> - Remove OwnableMut, and make DerefMut dependent on Unpin instead.
> - Usage example/doctest for Ownable/Owned.
> - Fixes to documentation and commit message.
> ]
>
> Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> Signed-off-by: Asahi Lina <lina+kernel@asahilina.net>
> Co-developed-by: Oliver Mangold <oliver.mangold@pm.me>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/owned.rs | 195 +++++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/sync/aref.rs | 5 ++
> rust/kernel/types.rs | 2 +
> 4 files changed, 203 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 3dd7bebe7888..e0ee04330dd0 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -112,6 +112,7 @@
> pub mod of;
> #[cfg(CONFIG_PM_OPP)]
> pub mod opp;
> +pub mod owned;
> pub mod page;
> #[cfg(CONFIG_PCI)]
> pub mod pci;
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> new file mode 100644
> index 000000000000..a2cdd2cb8a10
> --- /dev/null
> +++ b/rust/kernel/owned.rs
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Unique owned pointer types for objects with custom drop logic.
> +//!
> +//! These pointer types are useful for C-allocated objects which by API-contract
> +//! are owned by Rust, but need to be freed through the C API.
> +
> +use core::{
> + mem::ManuallyDrop,
> + ops::{Deref, DerefMut},
> + pin::Pin,
> + ptr::NonNull,
> +};
> +
> +/// Type allocated and destroyed on the C side, but owned by Rust.
The example given in the documentation below shows a valid way of
defining a type that's handled on the Rust side, so I think this
message is somewhat inaccurate.
Perhaps something like
Types that specify their own way of performing allocation and
destruction. Typically, this trait is implemented on types from
the C side.
?
> +///
> +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> +/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> +/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> +/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> +///
> +/// Note: The underlying object is not required to provide internal reference counting, because it
> +/// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> +/// [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that the [`release()`](Self::release) function frees the underlying
> +/// object in the correct way for a valid, owned object of this type.
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`Ownable`] and its usage with [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// # use core::cell::Cell;
> +/// # use core::ptr::NonNull;
> +/// # use kernel::sync::global_lock;
> +/// # use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// # use kernel::types::{Owned, Ownable};
> +///
> +/// // Let's count the allocations to see if freeing works.
> +/// kernel::sync::global_lock! {
> +/// // SAFETY: we call `init()` right below, before doing anything else.
> +/// unsafe(uninit) static FOO_ALLOC_COUNT: Mutex<usize> = 0;
> +/// }
> +/// // SAFETY: We call `init()` only once, here.
> +/// unsafe { FOO_ALLOC_COUNT.init() };
> +///
> +/// struct Foo {
> +/// }
> +///
> +/// impl Foo {
> +/// fn new() -> Result<Owned<Self>, AllocError> {
> +/// // We are just using a `KBox` here to handle the actual allocation, as our `Foo` is
> +/// // not actually a C-allocated object.
> +/// let result = KBox::new(
> +/// Foo {},
> +/// flags::GFP_KERNEL,
> +/// )?;
> +/// let result = NonNull::new(KBox::into_raw(result))
> +/// .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +/// // Count new allocation
> +/// *FOO_ALLOC_COUNT.lock() += 1;
> +/// // SAFETY: We just allocated the `Self`, thus it is valid and there cannot be any other
> +/// // Rust references. Calling `into_raw()` makes us responsible for ownership and we won't
> +/// // use the raw pointer anymore. Thus we can transfer ownership to the `Owned`.
> +/// Ok(unsafe { Owned::from_raw(result) })
> +/// }
> +/// }
> +///
> +/// // SAFETY: What out `release()` function does is safe of any valid `Self`.
I can't parse this sentence. Is "out" supposed to be a different word?
> +/// unsafe impl Ownable for Foo {
> +/// unsafe fn release(this: NonNull<Self>) {
> +/// // The `Foo` will be dropped when `KBox` goes out of scope.
I would just write `drop(unsafe { ... })` to make drop explicit instead
of commenting about the implicit drop.
> +/// // SAFETY: The [`KBox<Self>`] is still alive. We can pass ownership to the [`KBox`], as
> +/// // by requirement on calling this function, the `Self` will no longer be used by the
> +/// // caller.
> +/// unsafe { KBox::from_raw(this.as_ptr()) };
> +/// // Count released allocation
> +/// *FOO_ALLOC_COUNT.lock() -= 1;
> +/// }
> +/// }
> +///
> +/// {
> +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
> +/// assert!(*FOO_ALLOC_COUNT.lock() == 1);
> +/// }
> +/// // `foo` is out of scope now, so we expect no live allocations.
> +/// assert!(*FOO_ALLOC_COUNT.lock() == 0);
> +/// ```
> +pub unsafe trait Ownable {
> + /// Releases the object.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that:
> + /// - `this` points to a valid `Self`.
> + /// - `*this` is no longer used after this call.
> + unsafe fn release(this: NonNull<Self>);
> +}
> +
> +/// An owned reference to an owned `T`.
> +///
> +/// The [`Ownable`] is automatically freed or released when an instance of [`Owned`] is
> +/// dropped.
> +///
> +/// # Invariants
> +///
> +/// - The [`Owned<T>`] has exclusive access to the instance of `T`.
> +/// - The instance of `T` will stay alive at least as long as the [`Owned<T>`] is alive.
> +pub struct Owned<T: Ownable> {
> + ptr: NonNull<T>,
> +}
> +
> +// SAFETY: It is safe to send an [`Owned<T>`] to another thread when the underlying `T` is [`Send`],
> +// because of the ownership invariant. Sending an [`Owned<T>`] is equivalent to sending the `T`.
> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send [`&Owned<T>`] to another thread when the underlying `T` is [`Sync`],
> +// because of the ownership invariant. Sending an [`&Owned<T>`] is equivalent to sending the `&T`.
> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +
> +impl<T: Ownable> Owned<T> {
> + /// Creates a new instance of [`Owned`].
> + ///
> + /// It takes over ownership of the underlying object.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that:
> + /// - `ptr` points to a valid instance of `T`.
> + /// - Ownership of the underlying `T` can be transferred to the `Self<T>` (i.e. operations
> + /// which require ownership will be safe).
> + /// - No other Rust references to the underlying object exist. This implies that the underlying
> + /// object is not accessed through `ptr` anymore after the function call (at least until the
> + /// the `Self<T>` is dropped.
Is this correct? If `Self<T>` is dropped then `T::release` is called so
the pointer should also not be accessed further?
> + /// - The C code follows the usual shared reference requirements. That is, the kernel will never
> + /// mutate or free the underlying object (excluding interior mutability that follows the usual
> + /// rules) while Rust owns it.
The concept "interior mutability" doesn't really exist on the C side.
Also, use of interior mutability (by UnsafeCell) would be incorrect if
the type is implemented in the rust side (as this requires a
UnsafePinned).
Interior mutability means things can be mutated behind a shared
reference -- however in this case, we have a mutable reference (either
`Pin<&mut Self>` or `&mut Self`)!
Perhaps together with the next line, they could be just phrased like
this?
- The underlying object must not be accessed (read or mutated) through
any pointer other than the created `Owned<T>`.
Opt-out is still possbile similar to a mutable reference (e.g. by
using p`Opaque`]).
I think we should just tell the user "this is just a unique reference
similar to &mut". They should be able to deduce that all the `!Unpin`
that opts out from uniqueness of mutable reference applies here too.
> + /// - In case `T` implements [`Unpin`] the previous requirement is extended from shared to
> + /// mutable reference requirements. That is, the kernel will not mutate or free the underlying
> + /// object and is okay with it being modified by Rust code.
- If `T` implements [`Unpin`], the structure must not be mutated for
the entire lifetime of `Owned<T>`.
> + pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
This needs a (rather trivial) INVARIANT comment.
> + Self {
> + ptr,
> + }
> + }
> +
> + /// Consumes the [`Owned`], returning a raw pointer.
> + ///
> + /// This function does not actually relinquish ownership of the object. After calling this
Perhaps "relinquish" isn't the best word here? In my mental model
this function is pretty much relinquishing ownership as `Owned<T>` no
longer exists. It just doesn't release the object.
> + /// function, the caller is responsible for ownership previously managed
> + /// by the [`Owned`].
> + pub fn into_raw(me: Self) -> NonNull<T> {
> + ManuallyDrop::new(me).ptr
> + }
> +
> + /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
> + pub fn get_pin_mut(&mut self) -> Pin<&mut T> {
> + // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> + // return a mutable reference to it.
> + let unpinned = unsafe { self.ptr.as_mut() };
> +
> + // SAFETY: We never hand out unpinned mutable references to the data in
> + // `Self`, unless the contained type is `Unpin`.
> + unsafe { Pin::new_unchecked(unpinned) }
> + }
> +}
Best,
Gary
^ permalink raw reply
* Re: [PATCH v13 2/4] rust: `AlwaysRefCounted` is renamed to `RefCounted`.
From: Gary Guo @ 2025-12-01 16:00 UTC (permalink / raw)
To: Oliver Mangold
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Björn Roy Baron,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Benno Lossin,
Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <20251117-unique-ref-v13-2-b5b243df1250@pm.me>
On Mon, 17 Nov 2025 10:07:57 +0000
Oliver Mangold <oliver.mangold@pm.me> wrote:
> `AlwaysRefCounted` will become a marker trait to indicate that it is
> allowed to obtain an `ARef<T>` from a `&T`, which cannot be allowed for
> types which are also Ownable.
The message needs a rationale for making the change rather than relying
on the reader to deduce so.
For example:
There are types where it may both be referenced counted in some
cases and owned in other. In such cases, obtaining `ARef<T>`
from `&T` would be unsound as it allows creation of `ARef<T>`
copy from `&Owned<T>`.
Therefore, we split `AlwaysRefCounted` into `RefCounted` (which
`ARef<T>` would require) and a marker trait to indicate that
the type is always reference counted (and not `Ownable`) so the
`&T` -> `ARef<T>` conversion is possible.
Best,
Gary
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/auxiliary.rs | 7 +++++-
> rust/kernel/block/mq/request.rs | 15 +++++++------
> rust/kernel/cred.rs | 13 ++++++++++--
> rust/kernel/device.rs | 13 ++++++++----
> rust/kernel/device/property.rs | 7 +++++-
> rust/kernel/drm/device.rs | 10 ++++++---
> rust/kernel/drm/gem/mod.rs | 10 ++++++---
> rust/kernel/fs/file.rs | 16 ++++++++++----
> rust/kernel/mm.rs | 15 +++++++++----
> rust/kernel/mm/mmput_async.rs | 9 ++++++--
> rust/kernel/opp.rs | 10 ++++++---
> rust/kernel/owned.rs | 2 +-
> rust/kernel/pci.rs | 10 ++++++---
> rust/kernel/pid_namespace.rs | 12 +++++++++--
> rust/kernel/platform.rs | 7 +++++-
> rust/kernel/sync/aref.rs | 47 ++++++++++++++++++++++++++---------------
> rust/kernel/task.rs | 10 ++++++---
> rust/kernel/types.rs | 2 +-
> 18 files changed, 154 insertions(+), 61 deletions(-)
^ permalink raw reply
* Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: Eric W. Biederman @ 2025-12-01 16:06 UTC (permalink / raw)
To: Roberto Sassu
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <6dc556a0a93c18fffec71322bf97441c74b3134e.camel@huaweicloud.com>
Roberto Sassu <roberto.sassu@huaweicloud.com> writes:
> + Mimi, linux-integrity (would be nice if we are in CC when linux-
> security-module is in CC).
>
> Apologies for not answering earlier, it seems I don't receive the
> emails from the linux-security-module mailing list (thanks Serge for
> letting me know!).
>
> I see two main effects of this patch. First, the bprm_check_security
> hook implementations will not see bprm->cred populated. That was a
> problem before we made this patch:
>
> https://patchew.org/linux/20251008113503.2433343-1-roberto.sassu@huaweicloud.com/
Thanks, that is definitely needed.
Does calling process_measurement(CREDS_CHECK) on only the final file
pass review? Do you know of any cases where that will break things?
As it stands I don't think it should be assumed that any LSM has
computed it's final creds until bprm_creds_from_file. Not just the
uid and gid.
If the patch you posted for review works that helps sort that mess out.
> to work around the problem of not calculating the final DAC credentials
> early enough (well, we actually had to change our CREDS_CHECK hook
> behavior).
>
> The second, I could not check. If I remember well, unlike the
> capability LSM, SELinux/Apparmor/SMACK calculate the final credentials
> based on the first file being executed (thus the script, not the
> interpreter). Is this patch keeping the same behavior despite preparing
> the credentials when the final binary is found?
The patch I posted was.
My brain is still reeling from the realization that our security modules
have the implicit assumption that it is safe to calculate their security
information from shell scripts.
In the first half of the 90's I remember there was lots of effort to try
and make setuid shell scripts and setuid perl scripts work, and the
final conclusion was it was a lost cause.
Now I look at security_bprm_creds_for_exec and security_bprm_check which
both have the implicit assumption that it is indeed safe to compute the
credentials from a shell script.
When passing a file descriptor to execat we have
BINPRM_FLAGS_PATH_INACCESSIBLE and use /dev/fd/NNN as the filename
which reduces some of the races.
However when just plain executing a shell script we pass the filename of
the shell script as a command line argument, and expect the shell to
open the filename again. This has been a time of check to time of use
race for decades, and one of the reasons we don't have setuid shell
scripts.
Yet the IMA implementation (without the above mentioned patch) assumes
the final creds will be calculated before security_bprm_check is called,
and security_bprm_creds_for_exec busily calculate the final creds.
For some of the security modules I believe anyone can set any label they
want on a file and they remain secure (At which point I don't understand
the point of having labels on files). I don't believe that is the case
for selinux, or in general.
So just to remove the TOCTOU race the security_bprm_creds_for_exec
and security_bprm_check hooks need to be removed, after moving their
code into something like security_bprm_creds_from_file.
Or am I missing something and even with the TOCTOU race are setuid shell
scripts somehow safe now?
Eric
^ permalink raw reply
* Re: Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: Roberto Sassu @ 2025-12-01 16:49 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <87v7iqtcev.fsf_-_@email.froward.int.ebiederm.org>
On Mon, 2025-12-01 at 10:06 -0600, Eric W. Biederman wrote:
> Roberto Sassu <roberto.sassu@huaweicloud.com> writes:
>
> > + Mimi, linux-integrity (would be nice if we are in CC when linux-
> > security-module is in CC).
> >
> > Apologies for not answering earlier, it seems I don't receive the
> > emails from the linux-security-module mailing list (thanks Serge for
> > letting me know!).
> >
> > I see two main effects of this patch. First, the bprm_check_security
> > hook implementations will not see bprm->cred populated. That was a
> > problem before we made this patch:
> >
> > https://patchew.org/linux/20251008113503.2433343-1-roberto.sassu@huaweicloud.com/
>
> Thanks, that is definitely needed.
>
> Does calling process_measurement(CREDS_CHECK) on only the final file
> pass review? Do you know of any cases where that will break things?
We intentionally changed the behavior of CREDS_CHECK to be invoked only
for the final file. We are monitoring for bug reports, if we receive
complains from people that the patch breaks their expectation we will
revisit the issue.
Any LSM implementing bprm_check_security looking for brpm->cred would
be affected by recalculating the DAC credentials for the final binary.
> As it stands I don't think it should be assumed that any LSM has
> computed it's final creds until bprm_creds_from_file. Not just the
> uid and gid.
Uhm, I can be wrong, but most LSMs calculate their state change in
bprm_creds_for_exec (git grep bprm_creds_for_exec|grep LSM_HOOK_INIT).
> If the patch you posted for review works that helps sort that mess out.
Well, it works because we changed the expectation :)
> > to work around the problem of not calculating the final DAC credentials
> > early enough (well, we actually had to change our CREDS_CHECK hook
> > behavior).
> >
> > The second, I could not check. If I remember well, unlike the
> > capability LSM, SELinux/Apparmor/SMACK calculate the final credentials
> > based on the first file being executed (thus the script, not the
> > interpreter). Is this patch keeping the same behavior despite preparing
> > the credentials when the final binary is found?
>
> The patch I posted was.
>
> My brain is still reeling from the realization that our security modules
> have the implicit assumption that it is safe to calculate their security
> information from shell scripts.
If I'm interpreting this behavior correctly (please any LSM maintainer
could comment on it), the intent is just to transition to a different
security context where a different set of rules could apply (since we
are executing a script).
Imagine if for every script, the security transition is based on the
interpreter, it would be hard to differentiate between scripts and
associate to the respective processes different security labels.
> In the first half of the 90's I remember there was lots of effort to try
> and make setuid shell scripts and setuid perl scripts work, and the
> final conclusion was it was a lost cause.
Definitely I lack a lot of context...
> Now I look at security_bprm_creds_for_exec and security_bprm_check which
> both have the implicit assumption that it is indeed safe to compute the
> credentials from a shell script.
>
> When passing a file descriptor to execat we have
> BINPRM_FLAGS_PATH_INACCESSIBLE and use /dev/fd/NNN as the filename
> which reduces some of the races.
>
> However when just plain executing a shell script we pass the filename of
> the shell script as a command line argument, and expect the shell to
> open the filename again. This has been a time of check to time of use
> race for decades, and one of the reasons we don't have setuid shell
> scripts.
Yes, it would be really nice to fix it!
> Yet the IMA implementation (without the above mentioned patch) assumes
> the final creds will be calculated before security_bprm_check is called,
> and security_bprm_creds_for_exec busily calculate the final creds.
>
> For some of the security modules I believe anyone can set any label they
> want on a file and they remain secure (At which point I don't understand
> the point of having labels on files). I don't believe that is the case
> for selinux, or in general.
A simple example for SELinux. Suppose that the parent process has type
initrc_t, then the SELinux policy configures the following transitions
based on the label of the first file executed (sesearch -T -s initrc_t
-c process):
type_transition initrc_t NetworkManager_dispatcher_exec_t:process NetworkManager_dispatcher_t;
type_transition initrc_t NetworkManager_exec_t:process NetworkManager_t;
type_transition initrc_t NetworkManager_initrc_exec_t:process initrc_t;
type_transition initrc_t NetworkManager_priv_helper_exec_t:process NetworkManager_priv_helper_t;
type_transition initrc_t abrt_dump_oops_exec_t:process abrt_dump_oops_t;
type_transition initrc_t abrt_exec_t:process abrt_t;
[...]
(there are 747 rules in my system).
If the transition would be based on the interpreter label, it would be
hard to express with rules.
If the transition does not occur for any reason the parent process
policy would still apply, but maybe it would not have the necessary
permissions for the execution of the script.
> So just to remove the TOCTOU race the security_bprm_creds_for_exec
> and security_bprm_check hooks need to be removed, after moving their
> code into something like security_bprm_creds_from_file.
>
> Or am I missing something and even with the TOCTOU race are setuid shell
> scripts somehow safe now?
Take this with a looot of salt, if there is a TOCTOU race, the script
will be executed with a security context that does not belong to it.
But the transition already happened. Not sure if it is safe.
I also don't know how the TOCTOU race could be solved, but I also would
like it to be fixed. I'm available to comment on any proposal!
Roberto
^ permalink raw reply
* Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: Al Viro @ 2025-12-01 17:08 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Amir Goldstein, NeilBrown, Christian Brauner, Val Packett,
Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Chuck Lever, Olga Kornievskaia,
Dai Ngo, Namjae Jeon, Steve French, Sergey Senozhatsky,
Carlos Maiolino, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <CAJfpegs+o01jgY76WsGnk9j41LS5V0JQSk--d6xsJJp4VjTh8Q@mail.gmail.com>
On Mon, Dec 01, 2025 at 03:03:08PM +0100, Miklos Szeredi wrote:
> On Mon, 1 Dec 2025 at 09:33, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Mon, Dec 01, 2025 at 09:22:54AM +0100, Amir Goldstein wrote:
> >
> > > I don't think there is a point in optimizing parallel dir operations
> > > with FUSE server cache invalidation, but maybe I am missing
> > > something.
> >
> > The interesting part is the expected semantics of operation;
> > d_invalidate() side definitely doesn't need any of that cruft,
> > but I would really like to understand what that function
> > is supposed to do.
> >
> > Miklos, could you post a brain dump on that?
>
> This function is supposed to invalidate a dentry due to remote changes
> (FUSE_NOTIFY_INVAL_ENTRY). Originally it was supplied a parent ID and
> a name and called d_invalidate() on the looked up dentry.
>
> Then it grew a variant (FUSE_NOTIFY_DELETE) that was also supplied a
> child ID, which was matched against the looked up inode. This was
> commit 451d0f599934 ("FUSE: Notifying the kernel of deletion."),
> Apparently this worked around the fact that at that time
> d_invalidate() returned -EBUSY if the target was still in use and
> didn't unhash the dentry in that case.
>
> That was later changed by commit bafc9b754f75 ("vfs: More precise
> tests in d_invalidate") to unconditionally unhash the target, which
> effectively made FUSE_NOTIFY_INVAL_ENTRY and FUSE_NOTIFY_DELETE
> equivalent and the code in question unnecessary.
>
> For the future, we could also introduce FUSE_NOTIFY_MOVE, that would
> differentiate between a delete and a move, while
> FUSE_NOTIFY_INVAL_ENTRY would continue to be the common (deleted or
> moved) notification.
Then as far as VFS is concerned, it's an equivalent of "we'd done
a dcache lookup and revalidate told us to bugger off", which does
*not* need locking the parent - the same sequence can very well
happen without touching any inode locks.
IOW, from the point of view of locking protocol changes that's not
a removal at all.
Or do you need them serialized for fuse-internal purposes?
^ 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