* 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: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: 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 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] 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 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
* [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
* [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 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] 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
* 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
* 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 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 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 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
* [GIT PULL] KEYS: trusted: keys-trusted-next-rc1
From: Jarkko Sakkinen @ 2025-11-29 21:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, James Bottomley, Mimi Zohar, keyrings,
linux-integrity, linux-security-module
The following changes since commit e1afacb68573c3cd0a3785c6b0508876cd3423bc:
Merge tag 'ceph-for-6.18-rc8' of https://github.com/ceph/ceph-client (2025-11-27 11:11:03 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git tags/keys-trusted-next-rc1
for you to fetch changes up to 62cd5d480b9762ce70d720a81fa5b373052ae05f:
KEYS: trusted: Fix a memory leak in tpm2_load_cmd (2025-11-29 22:57:30 +0200)
----------------------------------------------------------------
Hi,
This pull request includes couple of updates for trusted keys:
1. Remove duplicate 'tpm2_hash_map' and use the one in the drive via new
function 'tpm2_find_hash_alg'.
2. Fix a memory leak on failure paths of 'tpm2_load_cmd'.
BR, Jarkko
----------------------------------------------------------------
Jarkko Sakkinen (2):
KEYS: trusted: Replace a redundant instance of tpm2_hash_map
KEYS: trusted: Fix a memory leak in tpm2_load_cmd
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++++-
include/linux/tpm.h | 1 +
security/keys/trusted-keys/trusted_tpm2.c | 29 ++++++++---------------------
3 files changed, 22 insertions(+), 22 deletions(-)
^ permalink raw reply
* [GIT PULL] Smack patches for 6.19
From: Casey Schaufler @ 2025-11-29 20:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: LSM List, Linux kernel mailing list, Casey Schaufler,
Konstantin Andreev
In-Reply-To: <80229bac-c3d9-4c99-9cca-dade23ef7421.ref@schaufler-ca.com>
Hello Linus,
Here is the Smack pull request for v6.19.
It fairly large as Smack pulls go. There are fixes for several
cases where labels are treated inconsistently when imported
from user space. The assignment of extended attributes has
been cleaned up. There are also some documentation improvements.
The following changes since commit e04c78d86a9699d136910cfc0bdcf01087e3267e:
Linux 6.16-rc2 (2025-06-15 13:49:41 -0700)
are available in the Git repository at:
https://github.com/cschaufler/smack-next tags/Smack-for-6.19
for you to fetch changes up to 29c701f90b9341f1f9c1854a9c22b71c2318457d:
Smack: function parameter 'gfp' not described (2025-11-11 12:00:18 -0800)
----------------------------------------------------------------
Patches for 6.19
----------------------------------------------------------------
Casey Schaufler (1):
Smack: function parameter 'gfp' not described
Konstantin Andreev (8):
smack: deduplicate "does access rule request transmutation"
smack: fix bug: SMACK64TRANSMUTE set on non-directory
smack: deduplicate xattr setting in smack_inode_init_security()
smack: always "instantiate" inode in smack_inode_init_security()
smack: fix bug: invalid label of unix socket file
smack: fix bug: unprivileged task can create labels
smack: fix bug: setting task label silently ignores input garbage
smack: fix kernel-doc warnings for smk_import_valid_label()
Documentation/admin-guide/LSM/Smack.rst | 16 +-
security/smack/smack.h | 3 +
security/smack/smack_access.c | 96 ++++++++---
security/smack/smack_lsm.c | 279 +++++++++++++++++++++-----------
4 files changed, 275 insertions(+), 119 deletions(-)
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Bernd Edlinger @ 2025-11-29 15:06 UTC (permalink / raw)
To: Oleg Nesterov
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: <aSNTNZxiQ0txISJx@redhat.com>
On 11/23/25 19:32, Oleg Nesterov wrote:
> Hi Bernd,
>
> sorry for delay, I am on PTO, didn't read emails this week...
>
> On 11/17, Bernd Edlinger wrote:
>>
>> On 11/17/25 16:01, Oleg Nesterov wrote:
>>> On 11/17, Bernd Edlinger wrote:
>>>>
>>>> On 11/11/25 10:21, Christian Brauner wrote:
>>>>> On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
>>>>
>>>>>> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
>>>>>> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
>>>>>> unconditionally.
>>>>>>
>>>>
>>>> I would not like to drop the mutex when no absolutely necessary for performance reasons.
>>>
>>> OK, I won't insist... But I don't really understand how this can help to
>>> improve the performance. If nothing else, this adds another for_other_threads()
>>> loop.
>>>
>>
>> If no dead-lock is possible it is better to complete the de_thread without
>> releasing the mutex. For the debugger it is also the better experience,
>> no matter when the ptrace_attack happens it will succeed rather quickly either
>> before the execve or after the execve.
>
> I still disagree, I still don't understand the "performance reasons", but since I can't
> convince you I won't really argue.
>
>>>>>>> + if (unlikely(unsafe_execve_in_progress)) {
>>>>>>> + spin_unlock_irq(lock);
>>>>>>> + sig->exec_bprm = bprm;
>>>>>>> + mutex_unlock(&sig->cred_guard_mutex);
>>>>>>> + spin_lock_irq(lock);
>>>>>>
>>>>>> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
>>>>>>
>>>>
>>>> Since the spin lock was acquired while holding the mutex, both should be
>>>> unlocked in reverse sequence and the spin lock re-acquired after releasing
>>>> the mutex.
>>>
>>> Why?
>>>
>>
>> It is generally more safe when each thread acquires its mutexes in order and
>> releases them in reverse order.
>> Consider this:
>> Thread A:
>> holds spin_lock_irq(siglock);
>> does mutes_unlock(cred_guard_mutex); with irq disabled.
>> task switch happens to Thread B which has irq enabled.
>> and is waiting for cred_guard_mutex.
>> Thrad B:
>> does mutex_lock(cred_guard_mutex);
>> but is interrupted this point and the interrupt handler I executes
>> now iterrupt handler I wants to take siglock and is blocked,
>> because the system one single CPU core.
>
> I don't follow. Do you mean PREEMPT_RT ?
>
> If yes. In this case spin_lock_irq() is rt_spin_lock() which doesn't disable irqs,
> it does rt_lock_lock() (takes rt_mutex) + migrate_disable().
>
> I do think that spin/mutex/whatever_unlock() is always safe. In any order, and
> regardless of RT.
>
Well, based on my experience with other embedded real-time O/S-es, I would
expect that something named spin_lock_irq locks the task-specific IRQ, and
prevents task switches due to time-slicing, while something called
mutes_unlock may cause an explicit task switch, when another task is waiting
for the mutex.
It is hard to follow how linux implements that spin_lock_irq exactly, 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-}
so an explicit task switch while locka_irq_disable looks
very dangerous to me. Do you know other places where such
a code pattern is used?
I do just ask, because a close look at those might reveal
some serious bugs, WDYT?
Thanks
Bernd.
^ permalink raw reply
* [PATCH] rust: security: use `pin_init::zeroed()` for LSM context initialization
From: Atharv Dubey @ 2025-11-29 13:56 UTC (permalink / raw)
To: 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, Atharv Dubey
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();
// SAFETY: Just a C FFI call. The pointer is valid for writes.
to_result(unsafe { bindings::security_secid_to_secctx(secid, &mut ctx) })?;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] net: ipv6: fix spelling typos in comments
From: Jakub Kicinski @ 2025-11-29 3:55 UTC (permalink / raw)
To: Shi Hao
Cc: davem, pabeni, dsahern, edumazet, horms, netdev,
linux-security-module, linux-kernel, steffen.klassert, herbert,
pablo
In-Reply-To: <20251127103133.13877-1-i.shihao.999@gmail.com>
On Thu, 27 Nov 2025 16:01:33 +0530 Shi Hao wrote:
> Correct misspelled typos in comments
>
> - informations -> information
> - wont -> won't
> - upto -> up to
> - destionation -> destination
Your changes does not apply, please rebase on netdev/net-next/main
--
pw-bot: cr
^ permalink raw reply
* [PATCH] tomoyo: Use local kmap in tomoyo_dump_page()
From: Davidlohr Bueso @ 2025-11-28 22:27 UTC (permalink / raw)
To: penguin-kernel, takedakn
Cc: linux-security-module, linux-kernel, Davidlohr Bueso
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>
---
security/tomoyo/domain.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 5f9ccab26e9a..90cf0e2969df 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -934,17 +934,12 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
#endif
if (page != dump->page) {
const unsigned int offset = pos % PAGE_SIZE;
- /*
- * Maybe kmap()/kunmap() should be used here.
- * But remove_arg_zero() uses kmap_atomic()/kunmap_atomic().
- * So do I.
- */
- char *kaddr = kmap_atomic(page);
+ char *kaddr = kmap_local_page(page);
dump->page = page;
memcpy(dump->data + offset, kaddr + offset,
PAGE_SIZE - offset);
- kunmap_atomic(kaddr);
+ kunmap_local(kaddr);
}
/* Same with put_arg_page(page) in fs/exec.c */
#ifdef CONFIG_MMU
--
2.39.5
^ permalink raw reply related
* Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`
From: Daniel Almeida @ 2025-11-28 18:06 UTC (permalink / raw)
To: Oliver Mangold
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: <20251117-unique-ref-v13-4-b5b243df1250@pm.me>
Hi Oliver,
> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> Types implementing one of these traits can safely convert between an
> `ARef<T>` and an `Owned<T>`.
>
> This is useful for types which generally are accessed through an `ARef`
> but have methods which can only safely be called when the reference is
> unique, like e.g. `block::mq::Request::end_ok()`.
>
> 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: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/owned.rs | 138 ++++++++++++++++++++++++++++++++++++++++++++---
> rust/kernel/sync/aref.rs | 11 +++-
> rust/kernel/types.rs | 2 +-
> 3 files changed, 141 insertions(+), 10 deletions(-)
>
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index a26747cbc13b..26ab2b00ada0 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
> @@ -5,6 +5,7 @@
> //! 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 crate::sync::aref::{ARef, RefCounted};
> use core::{
> mem::ManuallyDrop,
> ops::{Deref, DerefMut},
> @@ -14,14 +15,16 @@
>
> /// 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?
>
> /// Consumes the [`Owned`], returning a raw pointer.
> @@ -193,3 +194,124 @@ fn drop(&mut self) {
> unsafe { T::release(self.ptr) };
> }
> }
> +
> +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> +/// [`ARef`].
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`OwnableRefCounted`], [`Ownable`] and its usage with
> +/// [`ARef`] and [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// # use core::cell::Cell;
> +/// # use core::ptr::NonNull;
> +/// # use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// # use kernel::sync::aref::{ARef, RefCounted};
> +/// # use kernel::types::{Owned, Ownable, OwnableRefCounted};
> +///
> +/// // Example internally refcounted struct.
nit: IMHO the wording could improve ^
> +/// //
> +/// // # Invariants
> +/// //
> +/// // - `refcount` is always non-zero for a valid object.
> +/// // - `refcount` is >1 if there are more than 1 Rust reference to it.
> +/// //
> +/// struct Foo {
> +/// refcount: Cell<usize>,
> +/// }
> +///
> +/// 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 {
> +/// refcount: Cell::new(1),
> +/// },
> +/// flags::GFP_KERNEL,
> +/// )?;
> +/// let result = NonNull::new(KBox::into_raw(result))
> +/// .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +/// // 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: We increment and decrement each time the respective function is called and only free
> +/// // the `Foo` when the refcount reaches zero.
> +/// unsafe impl RefCounted for Foo {
> +/// fn inc_ref(&self) {
> +/// self.refcount.replace(self.refcount.get() + 1);
> +/// }
> +///
> +/// unsafe fn dec_ref(this: NonNull<Self>) {
> +/// // SAFETY: By requirement on calling this function, the refcount is non-zero,
> +/// // implying the underlying object is valid.
> +/// let refcount = unsafe { &this.as_ref().refcount };
> +/// let new_refcount = refcount.get() - 1;
> +/// if new_refcount == 0 {
> +/// // The `Foo` will be dropped when `KBox` goes out of scope.
> +/// // SAFETY: The [`KBox<Foo>`] is still alive as the old refcount is 1. 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()) };
> +/// } else {
> +/// refcount.replace(new_refcount);
> +/// }
> +/// }
> +/// }
> +///
> +/// 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.
> +/// // 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.
> +/// let mut foo = ARef::from(foo);
> +/// {
> +/// let bar = foo.clone();
> +/// assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub trait OwnableRefCounted: RefCounted + Ownable + Sized {
> + /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> + /// Otherwise it returns again an [`ARef`] to the same underlying object.
> + fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
Again, this method can go way if we add a method to expose the refcount.
> +
> + /// Converts the [`Owned`] into an [`ARef`].
> + fn into_shared(this: Owned<Self>) -> ARef<Self> {
> + // SAFETY: Safe by the requirements on implementing the trait.
> + unsafe { ARef::from_raw(Owned::into_raw(this)) }
> + }
> +}
> +
> +impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
> + type Error = ARef<T>;
> + /// Tries to convert the [`ARef`] to an [`Owned`] by calling
> + /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
> + /// unique, it returns again an [`ARef`] to the same underlying object.
> + fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
> + T::try_from_shared(b)
> + }
> +}
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 937dcf6ed5de..2dbffe2ed1b8 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -30,7 +30,10 @@
> /// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
> /// internal reference count and provides only shared references. If unique references are required
> /// [`Ownable`](crate::types::Ownable) should be implemented which allows types to be wrapped in an
> -/// [`Owned<Self>`](crate::types::Owned).
> +/// [`Owned<Self>`](crate::types::Owned). Implementing the trait
> +/// [`OwnableRefCounted`](crate::types::OwnableRefCounted) allows to convert between unique and
> +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and
> +/// [`ARef<Self>`](crate::types::Owned)).
> ///
> /// # Safety
> ///
> @@ -180,6 +183,12 @@ fn from(b: &T) -> Self {
> }
> }
>
> +impl<T: crate::types::OwnableRefCounted> From<crate::types::Owned<T>> for ARef<T> {
> + fn from(b: crate::types::Owned<T>) -> Self {
> + T::into_shared(b)
> + }
> +}
> +
Not sure why we’re using fully-qualified names here if we can import them, but your call.
> impl<T: RefCounted> Drop for ARef<T> {
> fn drop(&mut self) {
> // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ef01393352b..a9b72709d0d3 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,7 +11,7 @@
> };
> use pin_init::{PinInit, Wrapper, Zeroable};
>
> -pub use crate::owned::{Ownable, Owned};
> +pub use crate::owned::{Ownable, OwnableRefCounted, Owned};
>
> pub use crate::sync::aref::{ARef, AlwaysRefCounted, RefCounted};
>
>
> --
> 2.51.2
>
>
>
— Daniel
^ permalink raw reply
* Re: [PATCH v13 3/4] rust: Add missing SAFETY documentation for `ARef` example
From: Daniel Almeida @ 2025-11-28 17:50 UTC (permalink / raw)
To: Oliver Mangold
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: <20251117-unique-ref-v13-3-b5b243df1250@pm.me>
> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> SAFETY comment in rustdoc example was just 'TODO'. Fixed.
>
> 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>
> ---
> rust/kernel/sync/aref.rs | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 4226119d5ac9..937dcf6ed5de 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -129,12 +129,14 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> /// # Examples
> ///
> /// ```
> - /// use core::ptr::NonNull;
> - /// use kernel::sync::aref::{ARef, RefCounted};
> + /// # use core::ptr::NonNull;
> + /// # use kernel::sync::aref::{ARef, RefCounted};
> ///
> /// struct Empty {}
> ///
> - /// # // SAFETY: TODO.
> + /// // SAFETY: The `RefCounted` implementation for `Empty` does not count references and
> + /// // never frees the underlying object. Thus we can act as having a refcount on the object
nit: perhaps saying “an increment on the refcount” is clearer?
> + /// // that we pass to the newly created `ARef`.
> /// unsafe impl RefCounted for Empty {
> /// fn inc_ref(&self) {}
> /// unsafe fn dec_ref(_obj: NonNull<Self>) {}
> @@ -142,7 +144,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> ///
> /// let mut data = Empty {};
> /// let ptr = NonNull::<Empty>::new(&mut data).unwrap();
> - /// # // SAFETY: TODO.
> + /// // SAFETY: We keep `data` around longer than the `ARef`.
> /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
> /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref);
> ///
>
> --
> 2.51.2
>
>
>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply
* Re: [PATCH v13 2/4] rust: `AlwaysRefCounted` is renamed to `RefCounted`.
From: Daniel Almeida @ 2025-11-28 17:46 UTC (permalink / raw)
To: Oliver Mangold
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: <20251117-unique-ref-v13-2-b5b243df1250@pm.me>
> On 17 Nov 2025, at 07:07, 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.
>
> 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(-)
>
> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 7a3b0b9c418e..7f5b16053c11 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -10,6 +10,7 @@
> driver,
> error::{from_result, to_result, Result},
> prelude::*,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::Opaque,
> ThisModule,
> };
> @@ -239,7 +240,7 @@ extern "C" fn release(dev: *mut bindings::device) {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_ref().as_raw()) };
> @@ -258,6 +259,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> index c5f1f6b1ccfb..b6165f96b4ce 100644
> --- a/rust/kernel/block/mq/request.rs
> +++ b/rust/kernel/block/mq/request.rs
> @@ -8,7 +8,7 @@
> bindings,
> block::mq::Operations,
> error::Result,
> - sync::{atomic::Relaxed, Refcount},
> + sync::{aref::RefCounted, atomic::Relaxed, Refcount},
> types::{ARef, AlwaysRefCounted, Opaque},
> };
> use core::{marker::PhantomData, ptr::NonNull};
> @@ -225,11 +225,10 @@ unsafe impl<T: Operations> Send for Request<T> {}
> // mutate `self` are internally synchronized`
> unsafe impl<T: Operations> Sync for Request<T> {}
>
> -// SAFETY: All instances of `Request<T>` are reference counted. This
> -// implementation of `AlwaysRefCounted` ensure that increments to the ref count
> -// keeps the object alive in memory at least until a matching reference count
> -// decrement is executed.
> -unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
> +// SAFETY: All instances of `Request<T>` are reference counted. This implementation of `RefCounted`
> +// ensure that increments to the ref count keeps the object alive in memory at least until a
> +// matching reference count decrement is executed.
> +unsafe impl<T: Operations> RefCounted for Request<T> {
> fn inc_ref(&self) {
> self.wrapper_ref().refcount().inc();
> }
> @@ -251,3 +250,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
> }
> }
> }
> +
> +// SAFETY: We currently do not implement `Ownable`, thus it is okay to obtain an `ARef<Request>`
> +// from a `&Request` (but this will change in the future).
> +unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {}
> diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs
> index ffa156b9df37..20ef0144094b 100644
> --- a/rust/kernel/cred.rs
> +++ b/rust/kernel/cred.rs
> @@ -8,7 +8,12 @@
> //!
> //! Reference: <https://www.kernel.org/doc/html/latest/security/credentials.html>
>
> -use crate::{bindings, sync::aref::AlwaysRefCounted, task::Kuid, types::Opaque};
> +use crate::{
> + bindings,
> + sync::aref::RefCounted,
> + task::Kuid,
> + types::{AlwaysRefCounted, Opaque},
> +};
>
> /// Wraps the kernel's `struct cred`.
> ///
> @@ -76,7 +81,7 @@ pub fn euid(&self) -> Kuid {
> }
>
> // SAFETY: The type invariants guarantee that `Credential` is always ref-counted.
> -unsafe impl AlwaysRefCounted for Credential {
> +unsafe impl RefCounted for Credential {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -90,3 +95,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Credential>) {
> unsafe { bindings::put_cred(obj.cast().as_ptr()) };
> }
> }
> +
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Credential>` from a
> +// `&Credential`.
> +unsafe impl AlwaysRefCounted for Credential {}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index a849b7dde2fd..a69ee32997c1 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,9 +5,10 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> - bindings, fmt,
> - sync::aref::ARef,
> - types::{ForeignOwnable, Opaque},
> + bindings,
> + fmt,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
> };
> use core::{marker::PhantomData, ptr};
>
> @@ -407,7 +408,7 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_raw()) };
> @@ -419,6 +420,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> // SAFETY: As by the type invariant `Device` can be sent to any thread.
> unsafe impl Send for Device {}
>
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 3a332a8c53a9..a8bb824ad0ec 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -14,6 +14,7 @@
> fmt,
> prelude::*,
> str::{CStr, CString},
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::{ARef, Opaque},
> };
>
> @@ -359,7 +360,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> }
>
> // SAFETY: Instances of `FwNode` are always reference-counted.
> -unsafe impl crate::types::AlwaysRefCounted for FwNode {
> +unsafe impl RefCounted for FwNode {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the
> // refcount is non-zero.
> @@ -373,6 +374,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<FwNode>` from a
> +// `&FwNode`.
> +unsafe impl AlwaysRefCounted for FwNode {}
> +
> enum Node<'a> {
> Borrowed(&'a FwNode),
> Owned(ARef<FwNode>),
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3ce8f62a0056..38ce7f389ed0 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -11,8 +11,8 @@
> error::from_err_ptr,
> error::Result,
> prelude::*,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, Opaque},
> };
> use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>
> @@ -198,7 +198,7 @@ fn deref(&self) -> &Self::Target {
>
> // SAFETY: DRM device objects are always reference counted and the get/put functions
> // satisfy the requirements.
> -unsafe impl<T: drm::Driver> AlwaysRefCounted for Device<T> {
> +unsafe impl<T: drm::Driver> RefCounted for Device<T> {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::drm_dev_get(self.as_raw()) };
> @@ -213,6 +213,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl<T: drm::Driver> AlwaysRefCounted for Device<T> {}
> +
> impl<T: drm::Driver> AsRef<device::Device> for Device<T> {
> fn as_ref(&self) -> &device::Device {
> // SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid,
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 30c853988b94..4afd36e49205 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -10,8 +10,8 @@
> drm::driver::{AllocImpl, AllocOps},
> error::{to_result, Result},
> prelude::*,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, Opaque},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -56,7 +56,7 @@ pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted {
> }
>
> // SAFETY: All gem objects are refcounted.
> -unsafe impl<T: IntoGEMObject> AlwaysRefCounted for T {
> +unsafe impl<T: IntoGEMObject> RefCounted for T {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::drm_gem_object_get(self.as_raw()) };
> @@ -75,6 +75,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<T>` from a
> +// `&T`.
> +unsafe impl<T: IntoGEMObject> crate::types::AlwaysRefCounted for T {}
> +
> extern "C" fn open_callback<T: DriverObject>(
> raw_obj: *mut bindings::drm_gem_object,
> raw_file: *mut bindings::drm_file,
> diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
> index cd6987850332..86309ca5dad0 100644
> --- a/rust/kernel/fs/file.rs
> +++ b/rust/kernel/fs/file.rs
> @@ -12,8 +12,8 @@
> cred::Credential,
> error::{code::*, to_result, Error, Result},
> fmt,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::{NotThreadSafe, Opaque},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> };
> use core::ptr;
>
> @@ -192,7 +192,7 @@ unsafe impl Sync for File {}
>
> // SAFETY: The type invariants guarantee that `File` is always ref-counted. This implementation
> // makes `ARef<File>` own a normal refcount.
> -unsafe impl AlwaysRefCounted for File {
> +unsafe impl RefCounted for File {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -207,6 +207,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<File>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<File>` from a
> +// `&File`.
> +unsafe impl AlwaysRefCounted for File {}
> +
> /// Wraps the kernel's `struct file`. Not thread safe.
> ///
> /// This type represents a file that is not known to be safe to transfer across thread boundaries.
> @@ -228,7 +232,7 @@ pub struct LocalFile {
>
> // SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
> // makes `ARef<LocalFile>` own a normal refcount.
> -unsafe impl AlwaysRefCounted for LocalFile {
> +unsafe impl RefCounted for LocalFile {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -244,6 +248,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<LocalFile>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<LocalFile>` from a
> +// `&LocalFile`.
> +unsafe impl AlwaysRefCounted for LocalFile {}
> +
> impl LocalFile {
> /// Constructs a new `struct file` wrapper from a file descriptor.
> ///
> diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> index 4764d7b68f2a..dd9e3969e720 100644
> --- a/rust/kernel/mm.rs
> +++ b/rust/kernel/mm.rs
> @@ -13,8 +13,8 @@
>
> use crate::{
> bindings,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::{NotThreadSafe, Opaque},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -55,7 +55,7 @@ unsafe impl Send for Mm {}
> unsafe impl Sync for Mm {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for Mm {
> +unsafe impl RefCounted for Mm {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -69,6 +69,9 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Mm>` from a `&Mm`.
> +unsafe impl AlwaysRefCounted for Mm {}
> +
> /// A wrapper for the kernel's `struct mm_struct`.
> ///
> /// This type is like [`Mm`], but with non-zero `mm_users`. It can only be used when `mm_users` can
> @@ -91,7 +94,7 @@ unsafe impl Send for MmWithUser {}
> unsafe impl Sync for MmWithUser {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for MmWithUser {
> +unsafe impl RefCounted for MmWithUser {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -105,6 +108,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<MmWithUser>` from a
> +// `&MmWithUser`.
> +unsafe impl AlwaysRefCounted for MmWithUser {}
> +
> // Make all `Mm` methods available on `MmWithUser`.
> impl Deref for MmWithUser {
> type Target = Mm;
> diff --git a/rust/kernel/mm/mmput_async.rs b/rust/kernel/mm/mmput_async.rs
> index b8d2f051225c..aba4ce675c86 100644
> --- a/rust/kernel/mm/mmput_async.rs
> +++ b/rust/kernel/mm/mmput_async.rs
> @@ -10,7 +10,8 @@
> use crate::{
> bindings,
> mm::MmWithUser,
> - sync::aref::{ARef, AlwaysRefCounted},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -34,7 +35,7 @@ unsafe impl Send for MmWithUserAsync {}
> unsafe impl Sync for MmWithUserAsync {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for MmWithUserAsync {
> +unsafe impl RefCounted for MmWithUserAsync {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -48,6 +49,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<MmWithUserAsync>`
> +// from a `&MmWithUserAsync`.
> +unsafe impl AlwaysRefCounted for MmWithUserAsync {}
> +
> // Make all `MmWithUser` methods available on `MmWithUserAsync`.
> impl Deref for MmWithUserAsync {
> type Target = MmWithUser;
> diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
> index 2c763fa9276d..77d1cc89c412 100644
> --- a/rust/kernel/opp.rs
> +++ b/rust/kernel/opp.rs
> @@ -16,8 +16,8 @@
> ffi::c_ulong,
> prelude::*,
> str::CString,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, Opaque},
> };
>
> #[cfg(CONFIG_CPU_FREQ)]
> @@ -1037,7 +1037,7 @@ unsafe impl Send for OPP {}
> unsafe impl Sync for OPP {}
>
> /// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
> -unsafe impl AlwaysRefCounted for OPP {
> +unsafe impl RefCounted for OPP {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> unsafe { bindings::dev_pm_opp_get(self.0.get()) };
> @@ -1049,6 +1049,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<OPP>` from an
> +// `&OPP`.
> +unsafe impl AlwaysRefCounted for OPP {}
> +
> impl OPP {
> /// Creates an owned reference to a [`OPP`] from a valid pointer.
> ///
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index a2cdd2cb8a10..a26747cbc13b 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
> @@ -21,7 +21,7 @@
> ///
> /// 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.
> +/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> ///
> /// # Safety
> ///
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 7fcc5f6022c1..9ac70823fb4d 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -13,8 +13,8 @@
> io::{Io, IoRaw},
> irq::{self, IrqRequest},
> str::CStr,
> - sync::aref::ARef,
> - types::Opaque,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, Opaque},
> ThisModule,
> };
> use core::{
> @@ -601,7 +601,7 @@ pub fn set_master(&self) {
> impl crate::dma::Device for Device<device::Core> {}
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::pci_dev_get(self.as_raw()) };
> @@ -613,6 +613,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/pid_namespace.rs b/rust/kernel/pid_namespace.rs
> index 979a9718f153..4f6a94540e33 100644
> --- a/rust/kernel/pid_namespace.rs
> +++ b/rust/kernel/pid_namespace.rs
> @@ -7,7 +7,11 @@
> //! C header: [`include/linux/pid_namespace.h`](srctree/include/linux/pid_namespace.h) and
> //! [`include/linux/pid.h`](srctree/include/linux/pid.h)
>
> -use crate::{bindings, sync::aref::AlwaysRefCounted, types::Opaque};
> +use crate::{
> + bindings,
> + sync::aref::RefCounted,
> + types::{AlwaysRefCounted, Opaque},
> +};
> use core::ptr;
>
> /// Wraps the kernel's `struct pid_namespace`. Thread safe.
> @@ -41,7 +45,7 @@ pub unsafe fn from_ptr<'a>(ptr: *const bindings::pid_namespace) -> &'a Self {
> }
>
> // SAFETY: Instances of `PidNamespace` are always reference-counted.
> -unsafe impl AlwaysRefCounted for PidNamespace {
> +unsafe impl RefCounted for PidNamespace {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -55,6 +59,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<PidNamespace>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<PidNamespace>` from
> +// a `&PidNamespace`.
> +unsafe impl AlwaysRefCounted for PidNamespace {}
> +
> // SAFETY:
> // - `PidNamespace::dec_ref` can be called from any thread.
> // - It is okay to send ownership of `PidNamespace` across thread boundaries.
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 7205fe3416d3..bf2aa496b377 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -13,6 +13,7 @@
> irq::{self, IrqRequest},
> of,
> prelude::*,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::Opaque,
> ThisModule,
> };
> @@ -468,7 +469,7 @@ pub fn optional_irq_by_name(&self, name: &CStr) -> Result<IrqRequest<'_>> {
> impl crate::dma::Device for Device<device::Core> {}
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_ref().as_raw()) };
> @@ -480,6 +481,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index e175aefe8615..4226119d5ac9 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -19,11 +19,9 @@
>
> use core::{marker::PhantomData, mem::ManuallyDrop, ops::Deref, ptr::NonNull};
>
> -/// Types that are _always_ reference counted.
> +/// Types that are internally reference counted.
> ///
> /// It allows such types to define their own custom ref increment and decrement functions.
> -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> -/// [`ARef<T>`].
> ///
> /// This is usually implemented by wrappers to existing structures on the C side of the code. For
> /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> @@ -40,9 +38,8 @@
> /// at least until matching decrements are performed.
> ///
> /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> -/// alive.)
> -pub unsafe trait AlwaysRefCounted {
> +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
> +pub unsafe trait RefCounted {
> /// Increments the reference count on the object.
> fn inc_ref(&self);
>
> @@ -55,11 +52,27 @@ pub unsafe trait AlwaysRefCounted {
> /// Callers must ensure that there was a previous matching increment to the reference count,
> /// and that the object is no longer used after its reference count is decremented (as it may
> /// result in the object being freed), unless the caller owns another increment on the refcount
> - /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> - /// [`AlwaysRefCounted::dec_ref`] once).
> + /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
> unsafe fn dec_ref(obj: NonNull<Self>);
> }
>
> +/// Always reference-counted type.
> +///
> +/// It allows to derive a counted reference [`ARef<T>`] from a `&T`.
> +///
> +/// This provides some convenience, but it allows "escaping" borrow checks on `&T`. As it
> +/// complicates attempts to ensure that a reference to T is unique, it is optional to provide for
> +/// [`RefCounted`] types. See *Safety* below.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
> +/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`crate::types::Ownable`]
> +/// cannot be implemented for the same type, as this would allow to violate the uniqueness guarantee
> +/// of [`crate::types::Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from
> +/// that.
> +pub unsafe trait AlwaysRefCounted: RefCounted {}
> +
> /// An owned reference to an always-reference-counted object.
> ///
> /// The object's reference count is automatically decremented when an instance of [`ARef`] is
> @@ -70,7 +83,7 @@ pub unsafe trait AlwaysRefCounted {
> ///
> /// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> /// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> -pub struct ARef<T: AlwaysRefCounted> {
> +pub struct ARef<T: RefCounted> {
> ptr: NonNull<T>,
> _p: PhantomData<T>,
> }
> @@ -79,16 +92,16 @@ pub struct ARef<T: AlwaysRefCounted> {
> // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> // `T` to be `Send` because any thread that has an `ARef<T>` may ultimately access `T` using a
> // mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> -unsafe impl<T: AlwaysRefCounted + Sync + Send> Send for ARef<T> {}
> +unsafe impl<T: RefCounted + Sync + Send> Send for ARef<T> {}
>
> // SAFETY: It is safe to send `&ARef<T>` to another thread when the underlying `T` is `Sync`
> // because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally,
> // it needs `T` to be `Send` because any thread that has a `&ARef<T>` may clone it and get an
> // `ARef<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
> // example, when the reference count reaches zero and `T` is dropped.
> -unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
> +unsafe impl<T: RefCounted + Sync + Send> Sync for ARef<T> {}
>
> -impl<T: AlwaysRefCounted> ARef<T> {
> +impl<T: RefCounted> ARef<T> {
> /// Creates a new instance of [`ARef`].
> ///
> /// It takes over an increment of the reference count on the underlying object.
> @@ -117,12 +130,12 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> ///
> /// ```
> /// use core::ptr::NonNull;
> - /// use kernel::sync::aref::{ARef, AlwaysRefCounted};
> + /// use kernel::sync::aref::{ARef, RefCounted};
> ///
> /// struct Empty {}
> ///
> /// # // SAFETY: TODO.
> - /// unsafe impl AlwaysRefCounted for Empty {
> + /// unsafe impl RefCounted for Empty {
> /// fn inc_ref(&self) {}
> /// unsafe fn dec_ref(_obj: NonNull<Self>) {}
> /// }
> @@ -140,7 +153,7 @@ pub fn into_raw(me: Self) -> NonNull<T> {
> }
> }
>
> -impl<T: AlwaysRefCounted> Clone for ARef<T> {
> +impl<T: RefCounted> Clone for ARef<T> {
> fn clone(&self) -> Self {
> self.inc_ref();
> // SAFETY: We just incremented the refcount above.
> @@ -148,7 +161,7 @@ fn clone(&self) -> Self {
> }
> }
>
> -impl<T: AlwaysRefCounted> Deref for ARef<T> {
> +impl<T: RefCounted> Deref for ARef<T> {
> type Target = T;
>
> fn deref(&self) -> &Self::Target {
> @@ -165,7 +178,7 @@ fn from(b: &T) -> Self {
> }
> }
>
> -impl<T: AlwaysRefCounted> Drop for ARef<T> {
> +impl<T: RefCounted> Drop for ARef<T> {
> fn drop(&mut self) {
> // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> // decrement.
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 49fad6de0674..0a6e38d98456 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -9,8 +9,8 @@
> ffi::{c_int, c_long, c_uint},
> mm::MmWithUser,
> pid_namespace::PidNamespace,
> - sync::aref::ARef,
> - types::{NotThreadSafe, Opaque},
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, NotThreadSafe, Opaque},
> };
> use core::{
> cmp::{Eq, PartialEq},
> @@ -348,7 +348,7 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
> }
>
> // SAFETY: The type invariants guarantee that `Task` is always refcounted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Task {
> +unsafe impl RefCounted for Task {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -362,6 +362,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Task>` from a
> +// `&Task`.
> +unsafe impl AlwaysRefCounted for Task {}
> +
> impl Kuid {
> /// Get the current euid.
> #[inline]
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 7bc07c38cd6c..8ef01393352b 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -13,7 +13,7 @@
>
> pub use crate::owned::{Ownable, Owned};
>
> -pub use crate::sync::aref::{ARef, AlwaysRefCounted};
> +pub use crate::sync::aref::{ARef, AlwaysRefCounted, RefCounted};
>
> /// Used to transfer ownership to and from foreign (non-Rust) languages.
> ///
>
> --
> 2.51.2
>
>
>
Can you use imperative voice in the title? i.e.:
rust: rename `AlwaysRefCounted` to `RefCounted
…or something similar?
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply
* Re: [PATCH v4 1/4] landlock: Fix handling of disconnected directories
From: Tingmao Wang @ 2025-11-28 17:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Al Viro, Ben Scarlato, Christian Brauner,
Jann Horn, Jeff Xu, Justin Suess, Mikhail Ivanov, Paul Moore,
Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <20251128.oht7Aic8nu9d@digikod.net>
On 11/28/25 16:56, Mickaël Salaün wrote:
> On Fri, Nov 28, 2025 at 01:45:29AM +0000, Tingmao Wang wrote:
>> [...]
>>
>> Stepping back a bit, I also think it is reasonable to leave this issue as
>> is and not mitigate it (maybe warn about it in some way in the docs),
>> given that this can only happen if the policy is already weird (if the
>> intention is to protect some file, setting an allow access rule on its
>> parent, even if that parent is "hidden", is questionable).
>
> I agree.
Some additional bit of reasoning, just to make sure this is sound, and
access gaining can really only happen if the policy deliberately adds rule
above protected hierarchies (i.e. this can't be exploited if the policy is
not "problematic", even if it has other hidden rules):
As far as I can tell, there is no way to exploit a "hidden" rule like this
to e.g. read a file if the file is not already under the "hidden" rule,
since in this case the file must be outside of the bind mount. You can't
move files across mounts, and so the sandboxed application won't be able
to move it into the bind mount and cause the situation described above,
whether the destination is connected or disconnected. (It also can't move
the file into such a mount from the source fs of the bind mount, even if
it has visibility to the source fs, since the refer check would fail
there.)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox