* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-30 0:33 UTC (permalink / raw)
To: Mimi Zohar, Roberto Sassu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <497d1608572eec7d3d498967f0bae7d45023635b.camel@linux.ibm.com>
On Mon, Oct 20, 2025 at 09:57:19AM -0400, Mimi Zohar wrote:
>On Mon, 2025-10-20 at 14:45 +0200, Roberto Sassu wrote:
>> On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
[...]
>> >
>> > >
>> > > Another thought is to make CPIO support xattr. Today I realize that
>> > > ima_policy=secure_boot can also cause failure of loading kdump kernel.
>> > > So the issue this patch tries to resolves has much less impact than I
>> > > thought. Maybe we can wait until CPIO xattr support is ready? I'll help
>> > > review and test Roberto's patches if this is the best way forward.
>> >
>> > I'm not sure of the status of the CPIO patch set. Roberto?
>>
>> I haven't had time to look at it recently. I can take the openEuler
>> version, address the remaining comments and repost.
>
>Thank you!
+1, I'm looking forward to the reposted patch set!
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-30 3:01 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <xldwpvz4gzp74kualadf2n2wdlpbo3xorrgvkibhdocjtroipd@dpukmalev4yu>
On Thu, 2025-10-30 at 08:31 +0800, Coiby Xu wrote:
> On Fri, Oct 24, 2025 at 11:16:37AM -0400, Mimi Zohar wrote:
> > On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
> > > On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
> > > > > > > 2. Instead of defining an additional process_measurement() argument to identify
> > > > > > > compressed kernel modules, to simplify the code it might be possible to define a
> > > > > > > new "func" named COMPRESSED_MODULE_CHECK.
> > > > > > >
> > > > > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
> > > > > >
> > > > > > I also thought about this approach. But IMA rule maps kernel module
> > > > > > loading to MODULE_CHECK. If we define a new rule and ask users to use
> > > > > > this new rule, ima_policy=secure_boot still won't work.
> > > > >
> > > > > I don't have a problem with extending the "secure-boot" policy to support
> > > > > uncompressed kernel modules appended signatures, based on whether
> > > > > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
> > > > > MODULE_CHECK rule.
> > > >
> > > > I assume once the new rule get added, we can't remove it for userspace
> > > > backward compatibility, right? And with CPIO xattr supported, it seems
> > > > there is no need to keep this rule. So if this concern is valid, do you
> > > > think we shall switch to another approach i.e. to make IMA support
> > > > verifying decompressed module and then make "secure-boot" to allow
> > > > appended module signature?
> > >
> > > Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
> > > support verifying decompressed module", yes that might be a better solution,
> > > than relying on "sig_enforce" being enabled. IMA already supports verifying the
> > > appended signatures. A new IMA specific or LSM hook would need to be defined
> > > after module_decompress().
> >
> > Looking at the code further, decompressing the kernel module in IMA is
> > redundant. Instead I think the best approach would be to:
> > - define DECOMPRESSED_MODULE, in addition to COMPRESSED_MODULE.
> >
> > id(COMPRESSED_MODULE, compressed-kernel-module) \
> > id(DECOMPRESSED_MODULE, decompressed-kernel-module) \
> >
> > - instead of passing a boolean indicating whether the module is compressed, pass
> > the kernel_read_file_id enumeration to differentiate between the compressed and
> > decompressed module.
> >
> > - define a new IMA hook, probably LSM hook as well, named
> > ima_decompressed_module().
> >
> > - call the new ima_decompressed_module() from init_module_from_file()
> > immediately after decompressing the kernel module. Something along the lines
> > of:
> >
> > err = ima_decompressed_module(f, (char *)info.hdr, info.len,
> > READING_DECOMPRESSED_MODULE);
>
> Thanks for proposing a better solution! Yeah, decompressing the kernel
> module in IMA is unnecessary. Do you think we can further extend your
> idea to call one IMA hook only after kernel module decompression is
> done? If we call two IMA hooks in finit_module, we'll need coordination
> between two IMA hooks i.e. the 1st IMA hook shouldn't fail in order for
> the 2nd IMA hook to be executed. The new IMA hook will always have
> access to the decompressed kernel module buffer so there is no need to
> differentiate between compressed and decompressed module.
Agreed instead of verifying the kernel module signature on the LSM
kernel_post_read_file() hook, define and move it to a new IMA/LSM hook after it
is decompressed, would be much simpler than coordinating two LSM hooks.
>
> Another question is whether we should allow loading a kernel module with
> appended signature but misses IMA signature. Both IMA arch specific policy
> and init_module syscall only require appended signature verification. On
> the other hand, we only allow "appraise_type=imasig|modsig" but not
> appraise_type=modsig. How about we allow loading a kernel module with
> valid appended signature regardless of its IMA signature? We won't call
> set_module_sig_enforced but as long as we know is_module_sig_enforced()
> is true, we allow the module in IMA.
Based on the policy, IMA enforces signature verification. Only if
CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
IMA kernel module appraise rule. However, custom policies could still require
both types of signatures, not necessarily signed by the same entity.
The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
appended signature.
>
> >
> > For testing purposes to see the decompressed appended signature in the
> > measurement list, modify the MODULE_CHECK measure rule to include "template=ima-
> > modsig" in ima_efi.c.
>
> I haven't figured out why to include "template=ima-modsig" for testing
> purpose considering we can simply check if the kernel module has been
> loaded successfully.
That's fine too.
> It it related to the design that "The d-modsig and
> modsig fields are only populated if both the measure and appraise rules
> trigger"? If so, can you also help me understand there is such a design?
>
> [1] https://ima-doc.readthedocs.io/en/latest/event-log-format.html#ima-modsig
The "ima-sig" template contains the file data hash and file signature, allowing
the attestation server to verify the signature based on the file data hash
contained in the measurement list.
In addition to the file data hash and the file signature, the "ima-modsig"
template contains the file data hash without the appended signature, allowing
the attestation server to verify the appended signature against it.
Mimi
^ permalink raw reply
* Re: [PATCH v4 01/14] debugfs: rename end_creating() to debugfs_end_creating()
From: Greg Kroah-Hartman @ 2025-10-30 5:32 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Amir Goldstein, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, 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, apparmor,
linux-security-module, selinux
In-Reply-To: <20251029234353.1321957-2-neilb@ownmail.net>
On Thu, Oct 30, 2025 at 10:31:01AM +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> By not using the generic end_creating() name here we are free to use it
> more globally for a more generic function.
> This should have been done when start_creating() was renamed.
>
> For consistency, also rename failed_creating().
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/debugfs/inode.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH v4 07/14] VFS: introduce start_removing_dentry()
From: Al Viro @ 2025-10-30 6:11 UTC (permalink / raw)
To: NeilBrown
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251029234353.1321957-8-neilb@ownmail.net>
On Thu, Oct 30, 2025 at 10:31:07AM +1100, NeilBrown wrote:
> @@ -428,11 +429,14 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
> if (!old_tmpfile) {
> struct cachefiles_volume *volume = object->volume;
> struct dentry *fan = volume->fanout[(u8)cookie->key_hash];
> -
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - cachefiles_bury_object(volume->cache, object, fan,
> - old_file->f_path.dentry,
> - FSCACHE_OBJECT_INVALIDATED);
> + struct dentry *obj;
> +
> + obj = start_removing_dentry(fan, old_file->f_path.dentry);
> + if (!IS_ERR(obj))
> + cachefiles_bury_object(volume->cache, object,
> + fan, obj,
> + FSCACHE_OBJECT_INVALIDATED);
> + end_removing(obj);
Huh? Where did you change cachefiles_bury_object to *not* unlock the parent?
Not in this commit, AFAICS, and that means at least a bisection hazard around
here...
Confused...
^ permalink raw reply
* Re: [PATCH v4 11/14] Add start_renaming_two_dentries()
From: Al Viro @ 2025-10-30 6:22 UTC (permalink / raw)
To: NeilBrown
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251029234353.1321957-12-neilb@ownmail.net>
On Thu, Oct 30, 2025 at 10:31:11AM +1100, NeilBrown wrote:
> +++ b/fs/debugfs/inode.c
Why does debugfs_change_name() need any of that horror? Seriously, WTF?
This is strictly a name change on a filesystem that never, ever moves
anything from one directory to another.
IMO struct renamedata is a fucking eyesore, but that aside, this:
> @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
> if (ret)
> goto out;
>
> - lock_rename(tmp_parent, fsi->sb->s_root);
> + rd.old_parent = tmp_parent;
> + rd.new_parent = fsi->sb->s_root;
>
> /* booleans */
> - d_exchange(tmp_bool_dir, fsi->bool_dir);
> + ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> + if (!ret) {
> + d_exchange(tmp_bool_dir, fsi->bool_dir);
>
> - swap(fsi->bool_num, bool_num);
> - swap(fsi->bool_pending_names, bool_names);
> - swap(fsi->bool_pending_values, bool_values);
> + swap(fsi->bool_num, bool_num);
> + swap(fsi->bool_pending_names, bool_names);
> + swap(fsi->bool_pending_values, bool_values);
>
> - fsi->bool_dir = tmp_bool_dir;
> + fsi->bool_dir = tmp_bool_dir;
> + end_renaming(&rd);
> + }
>
> /* classes */
> - d_exchange(tmp_class_dir, fsi->class_dir);
> - fsi->class_dir = tmp_class_dir;
> + ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> + if (ret == 0) {
> + d_exchange(tmp_class_dir, fsi->class_dir);
> + fsi->class_dir = tmp_class_dir;
>
> - unlock_rename(tmp_parent, fsi->sb->s_root);
> + end_renaming(&rd);
> + }
>
> out:
> sel_remove_old_bool_data(bool_num, bool_names, bool_values);
is very interesting - suddenly you get two non-overlapping scopes instead of one.
Why is that OK?
^ permalink raw reply
* Re: [PATCH v4 12/14] ecryptfs: use new start_creating/start_removing APIs
From: Al Viro @ 2025-10-30 6:24 UTC (permalink / raw)
To: NeilBrown
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251029234353.1321957-13-neilb@ownmail.net>
On Thu, Oct 30, 2025 at 10:31:12AM +1100, NeilBrown wrote:
> +static struct dentry *ecryptfs_start_creating_dentry(struct dentry *dentry)
> {
> - struct dentry *lower_dir_dentry;
> + struct dentry *parent = dget_parent(dentry->d_parent);
"Grab the reference to grandparent"?
^ permalink raw reply
* Re: [PATCH v4 09/14] VFS/nfsd/ovl: introduce start_renaming() and end_renaming()
From: Chuck Lever @ 2025-10-30 13:22 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, 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251029234353.1321957-10-neilb@ownmail.net>
On 10/29/25 7:31 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> start_renaming() combines name lookup and locking to prepare for rename.
> It is used when two names need to be looked up as in nfsd and overlayfs -
> cases where one or both dentries are already available will be handled
> separately.
>
> __start_renaming() avoids the inode_permission check and hash
> calculation and is suitable after filename_parentat() in do_renameat2().
> It subsumes quite a bit of code from that function.
>
> start_renaming() does calculate the hash and check X permission and is
> suitable elsewhere:
> - nfsd_rename()
> - ovl_rename()
>
> In ovl, ovl_do_rename_rd() is factored out of ovl_do_rename(), which
> itself will be gone by the end of the series.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
>
> --
> Changes since v3:
> - added missig dput() in ovl_rename when "whiteout" is not-NULL.
>
> Changes since v2:
> - in __start_renaming() some label have been renamed, and err
> is always set before a "goto out_foo" rather than passing the
> error in a dentry*.
> - ovl_do_rename() changed to call the new ovl_do_rename_rd() rather
> than keeping duplicate code
> - code around ovl_cleanup() call in ovl_rename() restructured.
> ---
> fs/namei.c | 197 ++++++++++++++++++++++++++++-----------
> fs/nfsd/vfs.c | 73 +++++----------
> fs/overlayfs/dir.c | 74 +++++++--------
> fs/overlayfs/overlayfs.h | 23 +++--
> include/linux/namei.h | 3 +
> 5 files changed, 218 insertions(+), 152 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 04d2819bd351..0ee0a110b088 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3667,6 +3667,129 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
> }
> EXPORT_SYMBOL(unlock_rename);
>
> +/**
> + * __start_renaming - lookup and lock names for rename
> + * @rd: rename data containing parent and flags, and
> + * for receiving found dentries
> + * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
> + * LOOKUP_NO_SYMLINKS etc).
> + * @old_last: name of object in @rd.old_parent
> + * @new_last: name of object in @rd.new_parent
> + *
> + * Look up two names and ensure locks are in place for
> + * rename.
> + *
> + * On success the found dentries are stored in @rd.old_dentry,
> + * @rd.new_dentry. These references and the lock are dropped by
> + * end_renaming().
> + *
> + * The passed in qstrs must have the hash calculated, and no permission
> + * checking is performed.
> + *
> + * Returns: zero or an error.
> + */
> +static int
> +__start_renaming(struct renamedata *rd, int lookup_flags,
> + struct qstr *old_last, struct qstr *new_last)
> +{
> + struct dentry *trap;
> + struct dentry *d1, *d2;
> + int target_flags = LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
> + int err;
> +
> + if (rd->flags & RENAME_EXCHANGE)
> + target_flags = 0;
> + if (rd->flags & RENAME_NOREPLACE)
> + target_flags |= LOOKUP_EXCL;
> +
> + trap = lock_rename(rd->old_parent, rd->new_parent);
> + if (IS_ERR(trap))
> + return PTR_ERR(trap);
> +
> + d1 = lookup_one_qstr_excl(old_last, rd->old_parent,
> + lookup_flags);
> + err = PTR_ERR(d1);
> + if (IS_ERR(d1))
> + goto out_unlock;
> +
> + d2 = lookup_one_qstr_excl(new_last, rd->new_parent,
> + lookup_flags | target_flags);
> + err = PTR_ERR(d2);
> + if (IS_ERR(d2))
> + goto out_dput_d1;
> +
> + if (d1 == trap) {
> + /* source is an ancestor of target */
> + err = -EINVAL;
> + goto out_dput_d2;
> + }
> +
> + if (d2 == trap) {
> + /* target is an ancestor of source */
> + if (rd->flags & RENAME_EXCHANGE)
> + err = -EINVAL;
> + else
> + err = -ENOTEMPTY;
> + goto out_dput_d2;
> + }
> +
> + rd->old_dentry = d1;
> + rd->new_dentry = d2;
> + return 0;
> +
> +out_dput_d2:
> + dput(d2);
> +out_dput_d1:
> + dput(d1);
> +out_unlock:
> + unlock_rename(rd->old_parent, rd->new_parent);
> + return err;
> +}
> +
> +/**
> + * start_renaming - lookup and lock names for rename with permission checking
> + * @rd: rename data containing parent and flags, and
> + * for receiving found dentries
> + * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
> + * LOOKUP_NO_SYMLINKS etc).
> + * @old_last: name of object in @rd.old_parent
> + * @new_last: name of object in @rd.new_parent
> + *
> + * Look up two names and ensure locks are in place for
> + * rename.
> + *
> + * On success the found dentries are stored in @rd.old_dentry,
> + * @rd.new_dentry. These references and the lock are dropped by
> + * end_renaming().
> + *
> + * The passed in qstrs need not have the hash calculated, and basic
> + * eXecute permission checking is performed against @rd.mnt_idmap.
> + *
> + * Returns: zero or an error.
> + */
> +int start_renaming(struct renamedata *rd, int lookup_flags,
> + struct qstr *old_last, struct qstr *new_last)
> +{
> + int err;
> +
> + err = lookup_one_common(rd->mnt_idmap, old_last, rd->old_parent);
> + if (err)
> + return err;
> + err = lookup_one_common(rd->mnt_idmap, new_last, rd->new_parent);
> + if (err)
> + return err;
> + return __start_renaming(rd, lookup_flags, old_last, new_last);
> +}
> +EXPORT_SYMBOL(start_renaming);
> +
> +void end_renaming(struct renamedata *rd)
> +{
> + unlock_rename(rd->old_parent, rd->new_parent);
> + dput(rd->old_dentry);
> + dput(rd->new_dentry);
> +}
> +EXPORT_SYMBOL(end_renaming);
> +
> /**
> * vfs_prepare_mode - prepare the mode to be used for a new inode
> * @idmap: idmap of the mount the inode was found from
> @@ -5504,14 +5627,11 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> struct filename *to, unsigned int flags)
> {
> struct renamedata rd;
> - struct dentry *old_dentry, *new_dentry;
> - struct dentry *trap;
> struct path old_path, new_path;
> struct qstr old_last, new_last;
> int old_type, new_type;
> struct inode *delegated_inode = NULL;
> - unsigned int lookup_flags = 0, target_flags =
> - LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
> + unsigned int lookup_flags = 0;
> bool should_retry = false;
> int error = -EINVAL;
>
> @@ -5522,11 +5642,6 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> (flags & RENAME_EXCHANGE))
> goto put_names;
>
> - if (flags & RENAME_EXCHANGE)
> - target_flags = 0;
> - if (flags & RENAME_NOREPLACE)
> - target_flags |= LOOKUP_EXCL;
> -
> retry:
> error = filename_parentat(olddfd, from, lookup_flags, &old_path,
> &old_last, &old_type);
> @@ -5556,66 +5671,40 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> goto exit2;
>
> retry_deleg:
> - trap = lock_rename(new_path.dentry, old_path.dentry);
> - if (IS_ERR(trap)) {
> - error = PTR_ERR(trap);
> + rd.old_parent = old_path.dentry;
> + rd.mnt_idmap = mnt_idmap(old_path.mnt);
> + rd.new_parent = new_path.dentry;
> + rd.delegated_inode = &delegated_inode;
> + rd.flags = flags;
> +
> + error = __start_renaming(&rd, lookup_flags, &old_last, &new_last);
> + if (error)
> goto exit_lock_rename;
> - }
>
> - old_dentry = lookup_one_qstr_excl(&old_last, old_path.dentry,
> - lookup_flags);
> - error = PTR_ERR(old_dentry);
> - if (IS_ERR(old_dentry))
> - goto exit3;
> - new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
> - lookup_flags | target_flags);
> - error = PTR_ERR(new_dentry);
> - if (IS_ERR(new_dentry))
> - goto exit4;
> if (flags & RENAME_EXCHANGE) {
> - if (!d_is_dir(new_dentry)) {
> + if (!d_is_dir(rd.new_dentry)) {
> error = -ENOTDIR;
> if (new_last.name[new_last.len])
> - goto exit5;
> + goto exit_unlock;
> }
> }
> /* unless the source is a directory trailing slashes give -ENOTDIR */
> - if (!d_is_dir(old_dentry)) {
> + if (!d_is_dir(rd.old_dentry)) {
> error = -ENOTDIR;
> if (old_last.name[old_last.len])
> - goto exit5;
> + goto exit_unlock;
> if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
> - goto exit5;
> - }
> - /* source should not be ancestor of target */
> - error = -EINVAL;
> - if (old_dentry == trap)
> - goto exit5;
> - /* target should not be an ancestor of source */
> - if (!(flags & RENAME_EXCHANGE))
> - error = -ENOTEMPTY;
> - if (new_dentry == trap)
> - goto exit5;
> + goto exit_unlock;
> + }
>
> - error = security_path_rename(&old_path, old_dentry,
> - &new_path, new_dentry, flags);
> + error = security_path_rename(&old_path, rd.old_dentry,
> + &new_path, rd.new_dentry, flags);
> if (error)
> - goto exit5;
> + goto exit_unlock;
>
> - rd.old_parent = old_path.dentry;
> - rd.old_dentry = old_dentry;
> - rd.mnt_idmap = mnt_idmap(old_path.mnt);
> - rd.new_parent = new_path.dentry;
> - rd.new_dentry = new_dentry;
> - rd.delegated_inode = &delegated_inode;
> - rd.flags = flags;
> error = vfs_rename(&rd);
> -exit5:
> - dput(new_dentry);
> -exit4:
> - dput(old_dentry);
> -exit3:
> - unlock_rename(new_path.dentry, old_path.dentry);
> +exit_unlock:
> + end_renaming(&rd);
> exit_lock_rename:
> if (delegated_inode) {
> error = break_deleg_wait(&delegated_inode);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index cd64ffe12e0b..62109885d4db 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1885,11 +1885,12 @@ __be32
> nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> struct svc_fh *tfhp, char *tname, int tlen)
> {
> - struct dentry *fdentry, *tdentry, *odentry, *ndentry, *trap;
> + struct dentry *fdentry, *tdentry;
> int type = S_IFDIR;
> + struct renamedata rd = {};
> __be32 err;
> int host_err;
> - bool close_cached = false;
> + struct dentry *close_cached;
>
> trace_nfsd_vfs_rename(rqstp, ffhp, tfhp, fname, flen, tname, tlen);
>
> @@ -1915,15 +1916,22 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> goto out;
>
> retry:
> + close_cached = NULL;
> host_err = fh_want_write(ffhp);
> if (host_err) {
> err = nfserrno(host_err);
> goto out;
> }
>
> - trap = lock_rename(tdentry, fdentry);
> - if (IS_ERR(trap)) {
> - err = nfserr_xdev;
> + rd.mnt_idmap = &nop_mnt_idmap;
> + rd.old_parent = fdentry;
> + rd.new_parent = tdentry;
> +
> + host_err = start_renaming(&rd, 0, &QSTR_LEN(fname, flen),
> + &QSTR_LEN(tname, tlen));
> +
> + if (host_err) {
> + err = nfserrno(host_err);
> goto out_want_write;
> }
> err = fh_fill_pre_attrs(ffhp);
> @@ -1933,48 +1941,23 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> if (err != nfs_ok)
> goto out_unlock;
>
> - odentry = lookup_one(&nop_mnt_idmap, &QSTR_LEN(fname, flen), fdentry);
> - host_err = PTR_ERR(odentry);
> - if (IS_ERR(odentry))
> - goto out_nfserr;
> + type = d_inode(rd.old_dentry)->i_mode & S_IFMT;
> +
> + if (d_inode(rd.new_dentry))
> + type = d_inode(rd.new_dentry)->i_mode & S_IFMT;
>
> - host_err = -ENOENT;
> - if (d_really_is_negative(odentry))
> - goto out_dput_old;
> - host_err = -EINVAL;
> - if (odentry == trap)
> - goto out_dput_old;
> - type = d_inode(odentry)->i_mode & S_IFMT;
> -
> - ndentry = lookup_one(&nop_mnt_idmap, &QSTR_LEN(tname, tlen), tdentry);
> - host_err = PTR_ERR(ndentry);
> - if (IS_ERR(ndentry))
> - goto out_dput_old;
> - if (d_inode(ndentry))
> - type = d_inode(ndentry)->i_mode & S_IFMT;
> - host_err = -ENOTEMPTY;
> - if (ndentry == trap)
> - goto out_dput_new;
> -
> - if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) &&
> - nfsd_has_cached_files(ndentry)) {
> - close_cached = true;
> - goto out_dput_old;
> + if ((rd.new_dentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) &&
> + nfsd_has_cached_files(rd.new_dentry)) {
> + close_cached = dget(rd.new_dentry);
> + goto out_unlock;
> } else {
> - struct renamedata rd = {
> - .mnt_idmap = &nop_mnt_idmap,
> - .old_parent = fdentry,
> - .old_dentry = odentry,
> - .new_parent = tdentry,
> - .new_dentry = ndentry,
> - };
> int retries;
>
> for (retries = 1;;) {
> host_err = vfs_rename(&rd);
> if (host_err != -EAGAIN || !retries--)
> break;
> - if (!nfsd_wait_for_delegreturn(rqstp, d_inode(odentry)))
> + if (!nfsd_wait_for_delegreturn(rqstp, d_inode(rd.old_dentry)))
> break;
> }
> if (!host_err) {
> @@ -1983,11 +1966,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> host_err = commit_metadata(ffhp);
> }
> }
> - out_dput_new:
> - dput(ndentry);
> - out_dput_old:
> - dput(odentry);
> - out_nfserr:
> if (host_err == -EBUSY) {
> /*
> * See RFC 8881 Section 18.26.4 para 1-3: NFSv4 RENAME
> @@ -2006,7 +1984,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> fh_fill_post_attrs(tfhp);
> }
> out_unlock:
> - unlock_rename(tdentry, fdentry);
> + end_renaming(&rd);
> out_want_write:
> fh_drop_write(ffhp);
>
> @@ -2017,9 +1995,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> * until this point and then reattempt the whole shebang.
> */
> if (close_cached) {
> - close_cached = false;
> - nfsd_close_cached_files(ndentry);
> - dput(ndentry);
> + nfsd_close_cached_files(close_cached);
> + dput(close_cached);
> goto retry;
> }
> out:
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c8d0885ee5e0..0f2c2da68433 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1124,9 +1124,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> int err;
> struct dentry *old_upperdir;
> struct dentry *new_upperdir;
> - struct dentry *olddentry = NULL;
> - struct dentry *newdentry = NULL;
> - struct dentry *trap, *de;
> + struct renamedata rd = {};
> bool old_opaque;
> bool new_opaque;
> bool cleanup_whiteout = false;
> @@ -1136,6 +1134,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> bool new_is_dir = d_is_dir(new);
> bool samedir = olddir == newdir;
> struct dentry *opaquedir = NULL;
> + struct dentry *whiteout = NULL;
> const struct cred *old_cred = NULL;
> struct ovl_fs *ofs = OVL_FS(old->d_sb);
> LIST_HEAD(list);
> @@ -1233,29 +1232,21 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> }
> }
>
> - trap = lock_rename(new_upperdir, old_upperdir);
> - if (IS_ERR(trap)) {
> - err = PTR_ERR(trap);
> - goto out_revert_creds;
> - }
> + rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> + rd.old_parent = old_upperdir;
> + rd.new_parent = new_upperdir;
> + rd.flags = flags;
>
> - de = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
> - old->d_name.len);
> - err = PTR_ERR(de);
> - if (IS_ERR(de))
> - goto out_unlock;
> - olddentry = de;
> + err = start_renaming(&rd, 0,
> + &QSTR_LEN(old->d_name.name, old->d_name.len),
> + &QSTR_LEN(new->d_name.name, new->d_name.len));
>
> - err = -ESTALE;
> - if (!ovl_matches_upper(old, olddentry))
> - goto out_unlock;
> + if (err)
> + goto out_revert_creds;
>
> - de = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
> - new->d_name.len);
> - err = PTR_ERR(de);
> - if (IS_ERR(de))
> + err = -ESTALE;
> + if (!ovl_matches_upper(old, rd.old_dentry))
> goto out_unlock;
> - newdentry = de;
>
> old_opaque = ovl_dentry_is_opaque(old);
> new_opaque = ovl_dentry_is_opaque(new);
> @@ -1263,15 +1254,15 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> err = -ESTALE;
> if (d_inode(new) && ovl_dentry_upper(new)) {
> if (opaquedir) {
> - if (newdentry != opaquedir)
> + if (rd.new_dentry != opaquedir)
> goto out_unlock;
> } else {
> - if (!ovl_matches_upper(new, newdentry))
> + if (!ovl_matches_upper(new, rd.new_dentry))
> goto out_unlock;
> }
> } else {
> - if (!d_is_negative(newdentry)) {
> - if (!new_opaque || !ovl_upper_is_whiteout(ofs, newdentry))
> + if (!d_is_negative(rd.new_dentry)) {
> + if (!new_opaque || !ovl_upper_is_whiteout(ofs, rd.new_dentry))
> goto out_unlock;
> } else {
> if (flags & RENAME_EXCHANGE)
> @@ -1279,19 +1270,14 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> }
> }
>
> - if (olddentry == trap)
> - goto out_unlock;
> - if (newdentry == trap)
> - goto out_unlock;
> -
> - if (olddentry->d_inode == newdentry->d_inode)
> + if (rd.old_dentry->d_inode == rd.new_dentry->d_inode)
> goto out_unlock;
>
> err = 0;
> if (ovl_type_merge_or_lower(old))
> err = ovl_set_redirect(old, samedir);
> else if (is_dir && !old_opaque && ovl_type_merge(new->d_parent))
> - err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
> + err = ovl_set_opaque_xerr(old, rd.old_dentry, -EXDEV);
> if (err)
> goto out_unlock;
>
> @@ -1299,18 +1285,24 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> err = ovl_set_redirect(new, samedir);
> else if (!overwrite && new_is_dir && !new_opaque &&
> ovl_type_merge(old->d_parent))
> - err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
> + err = ovl_set_opaque_xerr(new, rd.new_dentry, -EXDEV);
> if (err)
> goto out_unlock;
>
> - err = ovl_do_rename(ofs, old_upperdir, olddentry,
> - new_upperdir, newdentry, flags);
> - unlock_rename(new_upperdir, old_upperdir);
> + err = ovl_do_rename_rd(&rd);
> +
> + if (!err && cleanup_whiteout)
> + whiteout = dget(rd.new_dentry);
> +
> + end_renaming(&rd);
> +
> if (err)
> goto out_revert_creds;
>
> - if (cleanup_whiteout)
> - ovl_cleanup(ofs, old_upperdir, newdentry);
> + if (whiteout) {
> + ovl_cleanup(ofs, old_upperdir, whiteout);
> + dput(whiteout);
> + }
>
> if (overwrite && d_inode(new)) {
> if (new_is_dir)
> @@ -1336,14 +1328,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> else
> ovl_drop_write(old);
> out:
> - dput(newdentry);
> - dput(olddentry);
> dput(opaquedir);
> ovl_cache_free(&list);
> return err;
>
> out_unlock:
> - unlock_rename(new_upperdir, old_upperdir);
> + end_renaming(&rd);
> goto out_revert_creds;
> }
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 49ad65f829dc..3cc85a893b5c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -355,11 +355,24 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
> }
>
> +static inline int ovl_do_rename_rd(struct renamedata *rd)
> +{
> + int err;
> +
> + pr_debug("rename(%pd2, %pd2, 0x%x)\n", rd->old_dentry, rd->new_dentry,
> + rd->flags);
> + err = vfs_rename(rd);
> + if (err) {
> + pr_debug("...rename(%pd2, %pd2, ...) = %i\n",
> + rd->old_dentry, rd->new_dentry, err);
> + }
> + return err;
> +}
> +
> static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> struct dentry *olddentry, struct dentry *newdir,
> struct dentry *newdentry, unsigned int flags)
> {
> - int err;
> struct renamedata rd = {
> .mnt_idmap = ovl_upper_mnt_idmap(ofs),
> .old_parent = olddir,
> @@ -369,13 +382,7 @@ static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> .flags = flags,
> };
>
> - pr_debug("rename(%pd2, %pd2, 0x%x)\n", olddentry, newdentry, flags);
> - err = vfs_rename(&rd);
> - if (err) {
> - pr_debug("...rename(%pd2, %pd2, ...) = %i\n",
> - olddentry, newdentry, err);
> - }
> - return err;
> + return ovl_do_rename_rd(&rd);
> }
>
> static inline int ovl_do_whiteout(struct ovl_fs *ofs,
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index e5cff89679df..19c3d8e336d5 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -156,6 +156,9 @@ extern int follow_up(struct path *);
> extern struct dentry *lock_rename(struct dentry *, struct dentry *);
> extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
> extern void unlock_rename(struct dentry *, struct dentry *);
> +int start_renaming(struct renamedata *rd, int lookup_flags,
> + struct qstr *old_last, struct qstr *new_last);
> +void end_renaming(struct renamedata *rd);
>
> /**
> * mode_strip_umask - handle vfs umask stripping
For the fs/nfsd/vfs.c hunks:
Acked-by: Chuck Lever <chuck.lever@oracle.com>
--
Chuck Lever
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-30 13:42 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <26cb0926bd707edea6f19ca1bf8f5d5d3d10ff96.camel@linux.ibm.com>
On Wed, Oct 29, 2025 at 11:01:27PM -0400, Mimi Zohar wrote:
>On Thu, 2025-10-30 at 08:31 +0800, Coiby Xu wrote:
>> On Fri, Oct 24, 2025 at 11:16:37AM -0400, Mimi Zohar wrote:
>> > On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
>> > > On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
>> > > > > > > 2. Instead of defining an additional process_measurement() argument to identify
>> > > > > > > compressed kernel modules, to simplify the code it might be possible to define a
>> > > > > > > new "func" named COMPRESSED_MODULE_CHECK.
>> > > > > > >
>> > > > > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
>> > > > > >
>> > > > > > I also thought about this approach. But IMA rule maps kernel module
>> > > > > > loading to MODULE_CHECK. If we define a new rule and ask users to use
>> > > > > > this new rule, ima_policy=secure_boot still won't work.
>> > > > >
>> > > > > I don't have a problem with extending the "secure-boot" policy to support
>> > > > > uncompressed kernel modules appended signatures, based on whether
>> > > > > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
>> > > > > MODULE_CHECK rule.
>> > > >
>> > > > I assume once the new rule get added, we can't remove it for userspace
>> > > > backward compatibility, right? And with CPIO xattr supported, it seems
>> > > > there is no need to keep this rule. So if this concern is valid, do you
>> > > > think we shall switch to another approach i.e. to make IMA support
>> > > > verifying decompressed module and then make "secure-boot" to allow
>> > > > appended module signature?
>> > >
>> > > Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
>> > > support verifying decompressed module", yes that might be a better solution,
>> > > than relying on "sig_enforce" being enabled. IMA already supports verifying the
>> > > appended signatures. A new IMA specific or LSM hook would need to be defined
>> > > after module_decompress().
>> >
>> > Looking at the code further, decompressing the kernel module in IMA is
>> > redundant. Instead I think the best approach would be to:
>> > - define DECOMPRESSED_MODULE, in addition to COMPRESSED_MODULE.
>> >
>> > id(COMPRESSED_MODULE, compressed-kernel-module) \
>> > id(DECOMPRESSED_MODULE, decompressed-kernel-module) \
>> >
>> > - instead of passing a boolean indicating whether the module is compressed, pass
>> > the kernel_read_file_id enumeration to differentiate between the compressed and
>> > decompressed module.
>> >
>> > - define a new IMA hook, probably LSM hook as well, named
>> > ima_decompressed_module().
>> >
>> > - call the new ima_decompressed_module() from init_module_from_file()
>> > immediately after decompressing the kernel module. Something along the lines
>> > of:
>> >
>> > err = ima_decompressed_module(f, (char *)info.hdr, info.len,
>> > READING_DECOMPRESSED_MODULE);
>>
>> Thanks for proposing a better solution! Yeah, decompressing the kernel
>> module in IMA is unnecessary. Do you think we can further extend your
>> idea to call one IMA hook only after kernel module decompression is
>> done? If we call two IMA hooks in finit_module, we'll need coordination
>> between two IMA hooks i.e. the 1st IMA hook shouldn't fail in order for
>> the 2nd IMA hook to be executed. The new IMA hook will always have
>> access to the decompressed kernel module buffer so there is no need to
>> differentiate between compressed and decompressed module.
>
>Agreed instead of verifying the kernel module signature on the LSM
>kernel_post_read_file() hook, define and move it to a new IMA/LSM hook after it
>is decompressed, would be much simpler than coordinating two LSM hooks.
Thanks for confirming it! I'll send a new version once the testing is
finished.
>
>>
>> Another question is whether we should allow loading a kernel module with
>> appended signature but misses IMA signature. Both IMA arch specific policy
>> and init_module syscall only require appended signature verification. On
>> the other hand, we only allow "appraise_type=imasig|modsig" but not
>> appraise_type=modsig. How about we allow loading a kernel module with
>> valid appended signature regardless of its IMA signature? We won't call
>> set_module_sig_enforced but as long as we know is_module_sig_enforced()
>> is true, we allow the module in IMA.
>
>Based on the policy, IMA enforces signature verification. Only if
>CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
>IMA kernel module appraise rule. However, custom policies could still require
>both types of signatures, not necessarily signed by the same entity.
>
>The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
>appended signature.
Thanks for the clarification! If I understand you correctly, some users
may want to enforce IMA signature verification and we should provide
such flexibility. Then do you think it's a good idea to change the kernel
module rule in ima_policy=secure_boot to
"appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
ima_policy=secure_boot can also work for in-kernel decompressing
modules?
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-30 16:50 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <pbkvp4o4m3spjgvctapidfnsswakekxl2vyigqip6yyfzp73z4@rgbohq7h4nnt>
On Thu, 2025-10-30 at 21:42 +0800, Coiby Xu wrote:
> > >
> > > Another question is whether we should allow loading a kernel module with
> > > appended signature but misses IMA signature. Both IMA arch specific policy
> > > and init_module syscall only require appended signature verification. On
> > > the other hand, we only allow "appraise_type=imasig|modsig" but not
> > > appraise_type=modsig. How about we allow loading a kernel module with
> > > valid appended signature regardless of its IMA signature? We won't call
> > > set_module_sig_enforced but as long as we know is_module_sig_enforced()
> > > is true, we allow the module in IMA.
> >
> > Based on the policy, IMA enforces signature verification. Only if
> > CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
> > IMA kernel module appraise rule. However, custom policies could still require
> > both types of signatures, not necessarily signed by the same entity.
> >
> > The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
> > appended signature.
>
> Thanks for the clarification! If I understand you correctly, some users
> may want to enforce IMA signature verification and we should provide
> such flexibility. Then do you think it's a good idea to change the kernel
> module rule in ima_policy=secure_boot to
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
> ima_policy=secure_boot can also work for in-kernel decompressing
> modules?
Yes, that's fine. Unlike the arch specific policy rules and the Kconfig
appraise rules, which persist after loading a custom policy, the builtin secure
boot rules do not persist.
Mimi
^ permalink raw reply
* Re: [PATCH 2/2] ipe: Update documentation for script enforcement
From: Fan Wu @ 2025-10-30 22:08 UTC (permalink / raw)
To: Yanzhu Huang
Cc: wufan, paul, mic, jmorris, serge, corbet, linux-security-module,
linux-doc, linux-kernel
In-Reply-To: <20251023233656.661344-3-yanzhuhuang@linux.microsoft.com>
On Thu, Oct 23, 2025 at 4:37 PM Yanzhu Huang
<yanzhuhuang@linux.microsoft.com> wrote:
>
> This patch adds explanation of script enforcement mechanism in admin
> guide documentation. Describes how IPE supports integrity enforcement
> for indirectly executed scripts through the AT_EXECVE_CHECK flag, and
> how this differs from kernel enforcement for compiled executables.
>
> Signed-off-by: Yanzhu Huang <yanzhuhuang@linux.microsoft.com>
> ---
> Documentation/admin-guide/LSM/ipe.rst | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
> index dc7088451f9d..1063256559a8 100644
> --- a/Documentation/admin-guide/LSM/ipe.rst
> +++ b/Documentation/admin-guide/LSM/ipe.rst
> @@ -95,7 +95,20 @@ languages when these scripts are invoked by passing these program files
> to the interpreter. This is because the way interpreters execute these
> files; the scripts themselves are not evaluated as executable code
> through one of IPE's hooks, but they are merely text files that are read
> -(as opposed to compiled executables) [#interpreters]_.
> +(as opposed to compiled executables) [#interpreters]_. However, with the
All looks good to me, however, we could also update the
[#interpreters] reference to userspace-api/check_exec.
-Fan
> +introduction of the ``AT_EXECVE_CHECK`` flag, interpreters can use it to
> +signal the kernel that a script file will be executed, and request the
> +kernel to perform LSM security checks on it.
> +
> +IPE's EXECUTE operation enforcement differs between compiled executables and
> +interpreted scripts: For compiled executables, enforcement is triggered
> +automatically by the kernel during ``execve()``, ``execveat()``, ``mmap()``
> +and ``mprotect()`` syscalls when loading executable content. For interpreted
> +scripts, enforcement requires explicit interpreter integration using
> +``execveat()`` with ``AT_EXECVE_CHECK`` flag. Unlike exec syscalls that IPE
> +intercepts during the execution process, this mechanism needs the interpreter
> +to take the initiative, and existing interpreters won't be automatically
> +supported unless the signal call is added.
>
> Threat Model
> ------------
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v4 07/14] VFS: introduce start_removing_dentry()
From: NeilBrown @ 2025-10-30 23:22 UTC (permalink / raw)
To: Al Viro
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251030061159.GV2441659@ZenIV>
On Thu, 30 Oct 2025, Al Viro wrote:
> On Thu, Oct 30, 2025 at 10:31:07AM +1100, NeilBrown wrote:
>
> > @@ -428,11 +429,14 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
> > if (!old_tmpfile) {
> > struct cachefiles_volume *volume = object->volume;
> > struct dentry *fan = volume->fanout[(u8)cookie->key_hash];
> > -
> > - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> > - cachefiles_bury_object(volume->cache, object, fan,
> > - old_file->f_path.dentry,
> > - FSCACHE_OBJECT_INVALIDATED);
> > + struct dentry *obj;
> > +
> > + obj = start_removing_dentry(fan, old_file->f_path.dentry);
> > + if (!IS_ERR(obj))
> > + cachefiles_bury_object(volume->cache, object,
> > + fan, obj,
> > + FSCACHE_OBJECT_INVALIDATED);
> > + end_removing(obj);
>
> Huh? Where did you change cachefiles_bury_object to *not* unlock the parent?
> Not in this commit, AFAICS, and that means at least a bisection hazard around
> here...
>
> Confused...
>
Thanks for the review and for catching that error.
This incremental patch should fix it.
Thanks,
NeilBrown
diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index 3f8a6f1a8fc3..a08250d244ea 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -436,7 +436,6 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
cachefiles_bury_object(volume->cache, object,
fan, obj,
FSCACHE_OBJECT_INVALIDATED);
- end_removing(obj);
}
fput(old_file);
}
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index b97a40917a32..0104ac00485d 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -261,6 +261,7 @@ static int cachefiles_unlink(struct cachefiles_cache *cache,
* - Directory backed objects are stuffed into the graveyard for userspace to
* delete
* On entry dir must be locked. It will be unlocked on exit.
+ * On entry there must be at least 2 refs on rep, one will be dropped on exit.
*/
int cachefiles_bury_object(struct cachefiles_cache *cache,
struct cachefiles_object *object,
@@ -275,12 +276,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
_enter(",'%pd','%pd'", dir, rep);
- /* end_removing() will dput() @rep but we need to keep
- * a ref, so take one now. This also stops the dentry
- * being negated when unlinked which we need.
- */
- dget(rep);
-
if (rep->d_parent != dir) {
end_removing(rep);
_leave(" = -ESTALE");
@@ -650,7 +645,6 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
ret = cachefiles_bury_object(volume->cache, object,
fan, de,
FSCACHE_OBJECT_IS_WEIRD);
- end_removing(de);
dput(dentry);
if (ret < 0)
return false;
diff --git a/fs/cachefiles/volume.c b/fs/cachefiles/volume.c
index ddf95ff5daf0..90ba926f488e 100644
--- a/fs/cachefiles/volume.c
+++ b/fs/cachefiles/volume.c
@@ -64,7 +64,6 @@ void cachefiles_acquire_volume(struct fscache_volume *vcookie)
cachefiles_bury_object(cache, NULL, cache->store,
vdentry,
FSCACHE_VOLUME_IS_WEIRD);
- end_removing(vdentry);
cachefiles_put_directory(volume->dentry);
cond_resched();
goto retry;
^ permalink raw reply related
* Re: [PATCH v4 11/14] Add start_renaming_two_dentries()
From: NeilBrown @ 2025-10-30 23:37 UTC (permalink / raw)
To: Al Viro
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251030062214.GW2441659@ZenIV>
On Thu, 30 Oct 2025, Al Viro wrote:
> On Thu, Oct 30, 2025 at 10:31:11AM +1100, NeilBrown wrote:
>
> > +++ b/fs/debugfs/inode.c
>
> Why does debugfs_change_name() need any of that horror? Seriously, WTF?
> This is strictly a name change on a filesystem that never, ever moves
> anything from one directory to another.
"horror" is clearly in the eye of the beholder, and not a helpful
description...
Is there anything in this use of start_renaming_two_dentries() which is
harmful? I agree that not all of the functionality is needed in this
case, but some of it is.
Would you prefer we also add
start_renaming_two_dentries_with_same_parent()
or similar?
>
> IMO struct renamedata is a fucking eyesore, but that aside, this:
>
> > @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
> > if (ret)
> > goto out;
> >
> > - lock_rename(tmp_parent, fsi->sb->s_root);
> > + rd.old_parent = tmp_parent;
> > + rd.new_parent = fsi->sb->s_root;
> >
> > /* booleans */
> > - d_exchange(tmp_bool_dir, fsi->bool_dir);
> > + ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> > + if (!ret) {
> > + d_exchange(tmp_bool_dir, fsi->bool_dir);
> >
> > - swap(fsi->bool_num, bool_num);
> > - swap(fsi->bool_pending_names, bool_names);
> > - swap(fsi->bool_pending_values, bool_values);
> > + swap(fsi->bool_num, bool_num);
> > + swap(fsi->bool_pending_names, bool_names);
> > + swap(fsi->bool_pending_values, bool_values);
> >
> > - fsi->bool_dir = tmp_bool_dir;
> > + fsi->bool_dir = tmp_bool_dir;
> > + end_renaming(&rd);
> > + }
> >
> > /* classes */
> > - d_exchange(tmp_class_dir, fsi->class_dir);
> > - fsi->class_dir = tmp_class_dir;
> > + ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> > + if (ret == 0) {
> > + d_exchange(tmp_class_dir, fsi->class_dir);
> > + fsi->class_dir = tmp_class_dir;
> >
> > - unlock_rename(tmp_parent, fsi->sb->s_root);
> > + end_renaming(&rd);
> > + }
> >
> > out:
> > sel_remove_old_bool_data(bool_num, bool_names, bool_values);
>
> is very interesting - suddenly you get two non-overlapping scopes instead of one.
> Why is that OK?
>
From the perspective of code performing lookup of these names, two
consecutive lookups would not be locked so they could see
inconsistencies anyway.
From the perspective of code changing these names, that is protected by
selinux_state.policy_mutex which is held across the combined operation.
A readdir could possibly see the old inum for one name and the new inum
for the other name. I don't imagine this would be a problem.
I have added a comment to the commit message to highlight this.
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH v4 12/14] ecryptfs: use new start_creating/start_removing APIs
From: NeilBrown @ 2025-10-30 23:41 UTC (permalink / raw)
To: Al Viro
Cc: 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,
apparmor, linux-security-module, selinux
In-Reply-To: <20251030062420.GX2441659@ZenIV>
On Thu, 30 Oct 2025, Al Viro wrote:
> On Thu, Oct 30, 2025 at 10:31:12AM +1100, NeilBrown wrote:
>
> > +static struct dentry *ecryptfs_start_creating_dentry(struct dentry *dentry)
> > {
> > - struct dentry *lower_dir_dentry;
> > + struct dentry *parent = dget_parent(dentry->d_parent);
>
> "Grab the reference to grandparent"?
>
That's somewhat embarrassing :-(
Fixed as below.
Thanks a lot!
NeilBrown
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index b3702105d236..6a5bca89e752 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -26,7 +26,7 @@
static struct dentry *ecryptfs_start_creating_dentry(struct dentry *dentry)
{
- struct dentry *parent = dget_parent(dentry->d_parent);
+ struct dentry *parent = dget_parent(dentry);
struct dentry *ret;
ret = start_creating_dentry(ecryptfs_dentry_to_lower(parent),
@@ -37,7 +37,7 @@ static struct dentry *ecryptfs_start_creating_dentry(struct dentry *dentry)
static struct dentry *ecryptfs_start_removing_dentry(struct dentry *dentry)
{
- struct dentry *parent = dget_parent(dentry->d_parent);
+ struct dentry *parent = dget_parent(dentry);
struct dentry *ret;
ret = start_removing_dentry(ecryptfs_dentry_to_lower(parent),
^ permalink raw reply related
* [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-10-31 7:40 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Mimi Zohar
Cc: Karel Srot, Paul Moore, James Morris, Serge E. Hallyn,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, open list,
open list:MODULE SUPPORT
In-Reply-To: <20250928030358.3873311-1-coxu@redhat.com>
Currently, when in-kernel module decompression (CONFIG_MODULE_DECOMPRESS)
is enabled, IMA has no way to verify the appended module signature as it
can't decompress the module.
Define a new LSM hook security_kernel_module_read_file which will be
called after kernel module decompression is done so IMA can access the
decompressed kernel module to verify the appended signature.
Since IMA can access both xattr and appended kernel module signature
with the new LSM hook, it no longer uses the security_kernel_post_read_file
LSM hook for kernel module loading.
Before enabling in-kernel module decompression, a kernel module in
initramfs can still be loaded with ima_policy=secure_boot. So adjust the
kernel module rule in secure_boot policy to allow either an IMA
signature OR an appended signature i.e. to use
"appraise func=MODULE_CHECK appraise_type=imasig|modsig".
Reported-by: Karel Srot <ksrot@redhat.com>
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
v1: https://lore.kernel.org/linux-integrity/20250928030358.3873311-1-coxu@redhat.com/
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 7 +++++++
kernel/module/main.c | 10 +++++++++-
security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 2 +-
security/security.c | 17 +++++++++++++++++
6 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 8c42b4bde09c..ced42eb8b618 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -232,6 +232,8 @@ LSM_HOOK(int, 0, kernel_read_file, struct file *file,
enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
+LSM_HOOK(int, 0, kernel_module_read_file, struct file *file, char *buf,
+ loff_t size)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
int flags)
LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old,
diff --git a/include/linux/security.h b/include/linux/security.h
index 92ac3f27b973..e47951292c73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -508,6 +508,7 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
bool contents);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
+int security_kernel_module_read_file(struct file *file, char *buf, loff_t size);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
int security_task_fix_setgid(struct cred *new, const struct cred *old,
@@ -1295,6 +1296,12 @@ static inline int security_kernel_post_read_file(struct file *file,
return 0;
}
+static inline int security_kernel_module_read_file(struct file *file,
+ char *buf, loff_t size)
+{
+ return 0;
+}
+
static inline int security_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..40bc86fa7384 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3678,6 +3678,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
struct load_info info = { };
void *buf = NULL;
int len;
+ int err;
len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
if (len < 0) {
@@ -3686,7 +3687,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
}
if (flags & MODULE_INIT_COMPRESSED_FILE) {
- int err = module_decompress(&info, buf, len);
+ err = module_decompress(&info, buf, len);
vfree(buf); /* compressed data is no longer needed */
if (err) {
mod_stat_inc(&failed_decompress);
@@ -3698,6 +3699,13 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
info.len = len;
}
+ err = security_kernel_module_read_file(f, (char *)info.hdr, info.len);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ free_copy(&info, flags);
+ return err;
+ }
+
return load_module(&info, uargs, flags);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cdd225f65a62..53d2e90176ea 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -635,6 +635,27 @@ static int ima_file_check(struct file *file, int mask)
MAY_APPEND), FILE_CHECK);
}
+/**
+ * ima_read_kernel_module - collect/appraise/audit measurement
+ * @file: file pointer to the module.
+ * @buf: buffer containing module data (possibly decompressed).
+ * @size: size of the buffer.
+ *
+ * This IMA hook for kernel_module_read_file LSM hook is called after a kernel
+ * module has been read into memory and (if applicable) decompressed. It
+ * measures and/or appraises the module based on the IMA policy.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int ima_read_kernel_module(struct file *file, char *buf, loff_t size)
+{
+ struct lsm_prop prop;
+
+ security_current_getlsmprop_subj(&prop);
+ return process_measurement(file, current_cred(), &prop, buf, size,
+ MAY_READ, MODULE_CHECK);
+}
+
static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
size_t buf_size)
{
@@ -881,6 +902,10 @@ static int ima_post_read_file(struct file *file, char *buf, loff_t size,
enum ima_hooks func;
struct lsm_prop prop;
+ /* kernel module will be addressed in ima_read_kernel_module */
+ if (read_id == READING_MODULE)
+ return 0;
+
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -1250,6 +1275,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, ima_load_data),
LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
LSM_HOOK_INIT(kernel_read_file, ima_read_file),
+ LSM_HOOK_INIT(kernel_module_read_file, ima_read_kernel_module),
LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
LSM_HOOK_INIT(path_post_mknod, ima_post_path_mknod),
#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 128fab897930..2c9bdc618ac9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -241,7 +241,7 @@ static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
{.action = APPRAISE, .func = MODULE_CHECK,
- .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST},
{.action = APPRAISE, .func = FIRMWARE_CHECK,
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
{.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
diff --git a/security/security.c b/security/security.c
index 4d3c03a4524c..311ba63a8889 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3442,6 +3442,23 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
}
EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
+/**
+ * security_kernel_module_read_file() - Read a kernel module loaded by finit_module
+ * @file: file
+ * @buf: contents of decompressed kernel module
+ * @size: size of decompressed kernel module
+ *
+ * Read a kernel module loaded by the finit_module syscall. Unlike
+ * security_kernel_post_read_file, it has access to the decompressed kernel module.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_kernel_module_read_file(struct file *file, char *buf, loff_t size)
+{
+ return call_int_hook(kernel_module_read_file, file, buf, size);
+}
+EXPORT_SYMBOL_GPL(security_kernel_module_read_file);
+
/**
* security_kernel_load_data() - Load data provided by userspace
* @id: data identifier
base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-31 7:58 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <1fb245a0e72a360df3a768726351e7fa76301471.camel@linux.ibm.com>
On Thu, Oct 30, 2025 at 12:50:48PM -0400, Mimi Zohar wrote:
>On Thu, 2025-10-30 at 21:42 +0800, Coiby Xu wrote:
>> > >
>> > > Another question is whether we should allow loading a kernel module with
>> > > appended signature but misses IMA signature. Both IMA arch specific policy
>> > > and init_module syscall only require appended signature verification. On
>> > > the other hand, we only allow "appraise_type=imasig|modsig" but not
>> > > appraise_type=modsig. How about we allow loading a kernel module with
>> > > valid appended signature regardless of its IMA signature? We won't call
>> > > set_module_sig_enforced but as long as we know is_module_sig_enforced()
>> > > is true, we allow the module in IMA.
>> >
>> > Based on the policy, IMA enforces signature verification. Only if
>> > CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
>> > IMA kernel module appraise rule. However, custom policies could still require
>> > both types of signatures, not necessarily signed by the same entity.
>> >
>> > The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
>> > appended signature.
>>
>> Thanks for the clarification! If I understand you correctly, some users
>> may want to enforce IMA signature verification and we should provide
>> such flexibility. Then do you think it's a good idea to change the kernel
>> module rule in ima_policy=secure_boot to
>> "appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
>> ima_policy=secure_boot can also work for in-kernel decompressing
>> modules?
>
>Yes, that's fine. Unlike the arch specific policy rules and the Kconfig
>appraise rules, which persist after loading a custom policy, the builtin secure
>boot rules do not persist.
Thanks for the confirmation! v2 has been posted.
>
>Mimi
>
>
--
Best regards,
Coiby
^ permalink raw reply
* [PATCH v2 0/2] ipe: add script enforcement mechanism with AT_EXECVE_CHECK
From: Yanzhu Huang @ 2025-10-31 10:16 UTC (permalink / raw)
To: wufan, paul, mic
Cc: jmorris, serge, corbet, yanzhuhuang, linux-security-module,
linux-doc, linux-kernel
In-Reply-To: <20251023233656.661344-1-yanzhuhuang@linux.microsoft.com>
Indirect file execution through interpreters (e.g. python script.py, sh
script.sh) should have integrity policy enforced by IPE based on the
rules. Currently, IPE can only enforce policy on the interpreter binary
itself, but has no visibility into the scripts that the interpreter
executes.
Overview
--------
This patch series introduces script enforcement for IPE, allowing integrity
evaluation of indirectly executed scripts through the AT_EXECVE_CHECK flag.
Patch 1 adds the core implementation with ipe_bprm_creds_for_exec() hook
that integrates with the AT_EXECVE_CHECK mechanism.
Patch 2 updates admin guide documentation to explain the script enforcement
mechanism.
The IPE test suite has been updated to include script enforcement tests:
https://github.com/microsoft/ipe/pull/6
Yanzhu Huang (2):
ipe: Add AT_EXECVE_CHECK support for script enforcement
ipe: Update documentation for script enforcement
Documentation/admin-guide/LSM/ipe.rst | 17 ++++++++++++++---
security/ipe/audit.c | 1 +
security/ipe/hooks.c | 27 +++++++++++++++++++++++++++
security/ipe/hooks.h | 3 +++
security/ipe/ipe.c | 1 +
5 files changed, 46 insertions(+), 3 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/2] ipe: Add AT_EXECVE_CHECK support for script enforcement
From: Yanzhu Huang @ 2025-10-31 10:16 UTC (permalink / raw)
To: wufan, paul, mic
Cc: jmorris, serge, corbet, yanzhuhuang, linux-security-module,
linux-doc, linux-kernel
In-Reply-To: <20251031101700.694964-1-yanzhuhuang@linux.microsoft.com>
This patch adds a new ipe_bprm_creds_for_exec() hook that integrates
with the AT_EXECVE_CHECK mechanism. To enable script enforcement,
interpreters need to incorporate the AT_EXECVE_CHECK flag when
calling execveat() on script files before execuation.
When a userspace interpreter calls execveat() with the AT_EXECVE_CHECK
flag, this hook triggers IPE policy evaluation on the script file. The
hook only triggers IPE when bprm->is_check is true, ensuring it's
being called from an AT_EXECVE_CHECK context. It then builds an
evaluation context for an IPE_OP_EXEC operation and invokes IPE policy.
The kernel returns the policy decision to the interpreter, which can
then decide whether to proceed with script execution.
This extends IPE enforcement to indirectly executed scripts, permitting
trusted scripts to execute while denying untrusted ones.
Signed-off-by: Yanzhu Huang <yanzhuhuang@linux.microsoft.com>
---
security/ipe/audit.c | 1 +
security/ipe/hooks.c | 27 +++++++++++++++++++++++++++
security/ipe/hooks.h | 3 +++
security/ipe/ipe.c | 1 +
4 files changed, 32 insertions(+)
diff --git a/security/ipe/audit.c b/security/ipe/audit.c
index de5fed62592e..3f0deeb54912 100644
--- a/security/ipe/audit.c
+++ b/security/ipe/audit.c
@@ -46,6 +46,7 @@ static const char *const audit_op_names[__IPE_OP_MAX + 1] = {
static const char *const audit_hook_names[__IPE_HOOK_MAX] = {
"BPRM_CHECK",
+ "BPRM_CREDS_FOR_EXEC",
"MMAP",
"MPROTECT",
"KERNEL_READ",
diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
index d0323b81cd8f..32dd99abd4de 100644
--- a/security/ipe/hooks.c
+++ b/security/ipe/hooks.c
@@ -35,6 +35,33 @@ int ipe_bprm_check_security(struct linux_binprm *bprm)
return ipe_evaluate_event(&ctx);
}
+/**
+ * ipe_bprm_creds_for_exec() - ipe security hook function for bprm creds check.
+ * @bprm: Supplies a pointer to a linux_binprm structure to source the file
+ * being evaluated.
+ *
+ * This LSM hook is called when userspace signals the kernel to check a file
+ * for execution through the execveat syscall with the AT_EXECVE_CHECK flag.
+ * The hook triggers IPE policy evaluation on the script file and returns
+ * the policy decision to userspace. The userspace program receives the
+ * return code and can decide whether to proceed with script execution.
+ *
+ * Return:
+ * * %0 - Success
+ * * %-EACCES - Did not pass IPE policy
+ */
+int ipe_bprm_creds_for_exec(struct linux_binprm *bprm)
+{
+ struct ipe_eval_ctx ctx = IPE_EVAL_CTX_INIT;
+
+ if (!bprm->is_check)
+ return 0;
+
+ ipe_build_eval_ctx(&ctx, bprm->file, IPE_OP_EXEC,
+ IPE_HOOK_BPRM_CREDS_FOR_EXEC);
+ return ipe_evaluate_event(&ctx);
+}
+
/**
* ipe_mmap_file() - ipe security hook function for mmap check.
* @f: File being mmap'd. Can be NULL in the case of anonymous memory.
diff --git a/security/ipe/hooks.h b/security/ipe/hooks.h
index 38d4a387d039..07db37332740 100644
--- a/security/ipe/hooks.h
+++ b/security/ipe/hooks.h
@@ -13,6 +13,7 @@
enum ipe_hook_type {
IPE_HOOK_BPRM_CHECK = 0,
+ IPE_HOOK_BPRM_CREDS_FOR_EXEC,
IPE_HOOK_MMAP,
IPE_HOOK_MPROTECT,
IPE_HOOK_KERNEL_READ,
@@ -24,6 +25,8 @@ enum ipe_hook_type {
int ipe_bprm_check_security(struct linux_binprm *bprm);
+int ipe_bprm_creds_for_exec(struct linux_binprm *bprm);
+
int ipe_mmap_file(struct file *f, unsigned long reqprot, unsigned long prot,
unsigned long flags);
diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
index 4317134cb0da..845e3fd7a345 100644
--- a/security/ipe/ipe.c
+++ b/security/ipe/ipe.c
@@ -47,6 +47,7 @@ struct ipe_inode *ipe_inode(const struct inode *inode)
static struct security_hook_list ipe_hooks[] __ro_after_init = {
LSM_HOOK_INIT(bprm_check_security, ipe_bprm_check_security),
+ LSM_HOOK_INIT(bprm_creds_for_exec, ipe_bprm_creds_for_exec),
LSM_HOOK_INIT(mmap_file, ipe_mmap_file),
LSM_HOOK_INIT(file_mprotect, ipe_file_mprotect),
LSM_HOOK_INIT(kernel_read_file, ipe_kernel_read_file),
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/2] ipe: Update documentation for script enforcement
From: Yanzhu Huang @ 2025-10-31 10:17 UTC (permalink / raw)
To: wufan, paul, mic
Cc: jmorris, serge, corbet, yanzhuhuang, linux-security-module,
linux-doc, linux-kernel
In-Reply-To: <20251031101700.694964-1-yanzhuhuang@linux.microsoft.com>
This patch adds explanation of script enforcement mechanism in admin
guide documentation. Describes how IPE supports integrity enforcement
for indirectly executed scripts through the AT_EXECVE_CHECK flag, and
how this differs from kernel enforcement for compiled executables.
Signed-off-by: Yanzhu Huang <yanzhuhuang@linux.microsoft.com>
---
Documentation/admin-guide/LSM/ipe.rst | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
index dc7088451f9d..3f205d7dd533 100644
--- a/Documentation/admin-guide/LSM/ipe.rst
+++ b/Documentation/admin-guide/LSM/ipe.rst
@@ -95,7 +95,20 @@ languages when these scripts are invoked by passing these program files
to the interpreter. This is because the way interpreters execute these
files; the scripts themselves are not evaluated as executable code
through one of IPE's hooks, but they are merely text files that are read
-(as opposed to compiled executables) [#interpreters]_.
+(as opposed to compiled executables). However, with the introduction of the
+``AT_EXECVE_CHECK`` flag (see `AT_EXECVE_CHECK <https://docs.kernel.org/userspace-api/check_exec.html#at-execve-check>`__),
+interpreters can use it to signal the kernel that a script file will be executed,
+and request the kernel to perform LSM security checks on it.
+
+IPE's EXECUTE operation enforcement differs between compiled executables and
+interpreted scripts: For compiled executables, enforcement is triggered
+automatically by the kernel during ``execve()``, ``execveat()``, ``mmap()``
+and ``mprotect()`` syscalls when loading executable content. For interpreted
+scripts, enforcement requires explicit interpreter integration using
+``execveat()`` with ``AT_EXECVE_CHECK`` flag. Unlike exec syscalls that IPE
+intercepts during the execution process, this mechanism needs the interpreter
+to take the initiative, and existing interpreters won't be automatically
+supported unless the signal call is added.
Threat Model
------------
@@ -806,8 +819,6 @@ A:
.. [#digest_cache_lsm] https://lore.kernel.org/lkml/20240415142436.2545003-1-roberto.sassu@huaweicloud.com/
-.. [#interpreters] There is `some interest in solving this issue <https://lore.kernel.org/lkml/20220321161557.495388-1-mic@digikod.net/>`_.
-
.. [#devdoc] Please see :doc:`the design docs </security/ipe>` for more on
this topic.
--
2.43.0
^ permalink raw reply related
* [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Thorsten Blum @ 2025-10-31 11:06 UTC (permalink / raw)
To: Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-security-module, linux-kernel
strcpy() is deprecated and sprintf() does not perform bounds checking
either. While the current code works correctly, strscpy() and snprintf()
are safer alternatives that follow secure coding best practices.
Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/device_cgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index dc4df7475081..a41f558f6fdd 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -273,9 +273,9 @@ static char type_to_char(short type)
static void set_majmin(char *str, unsigned m)
{
if (m == ~0)
- strcpy(str, "*");
+ strscpy(str, "*", MAJMINLEN);
else
- sprintf(str, "%u", m);
+ snprintf(str, MAJMINLEN, "%u", m);
}
static int devcgroup_seq_show(struct seq_file *m, void *v)
--
2.51.1
^ permalink raw reply related
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: David Laight @ 2025-10-31 12:59 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031110647.102728-2-thorsten.blum@linux.dev>
On Fri, 31 Oct 2025 12:06:47 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:
> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. While the current code works correctly, strscpy() and snprintf()
> are safer alternatives that follow secure coding best practices.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/device_cgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..a41f558f6fdd 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -273,9 +273,9 @@ static char type_to_char(short type)
> static void set_majmin(char *str, unsigned m)
> {
> if (m == ~0)
> - strcpy(str, "*");
> + strscpy(str, "*", MAJMINLEN);
> else
> - sprintf(str, "%u", m);
> + snprintf(str, MAJMINLEN, "%u", m);
> }
>
> static int devcgroup_seq_show(struct seq_file *m, void *v)
There is no point using sting length limits that aren't passed into the function.
In any case the code seems to be crap (why is 'security' code always bad?)
(See https://elixir.bootlin.com/linux/v6.18-rc3/source/security/device_cgroup.c#L247)
I doubt ex->major or ex->minor can ever be ~0.
So there are two sets of calls, one set passes ~0 and the other doesn't.
The output buffers are then passed into another printf().
Even if ex->major can be ~0 there are much cleaner ways of writing this code.
David
^ permalink raw reply
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Serge E. Hallyn @ 2025-10-31 13:02 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031110647.102728-2-thorsten.blum@linux.dev>
On Fri, Oct 31, 2025 at 12:06:47PM +0100, Thorsten Blum wrote:
> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. While the current code works correctly, strscpy() and snprintf()
> are safer alternatives that follow secure coding best practices.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/device_cgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..a41f558f6fdd 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -273,9 +273,9 @@ static char type_to_char(short type)
> static void set_majmin(char *str, unsigned m)
> {
> if (m == ~0)
> - strcpy(str, "*");
> + strscpy(str, "*", MAJMINLEN);
I dunno, I'm not saying I would say no to this, but it does look
a little ridiculous. If it used some macro version which computes
the length of str instead of typing in MAJMINLEN, that might be
better. But we're just as likely here to get MAJMINLEN out of
sync with the length of strscpy as anything else happening, and
all to copy over two bytes.
> else
> - sprintf(str, "%u", m);
> + snprintf(str, MAJMINLEN, "%u", m);
Here, to me it always looks suspect to use snprintf without
checking its return value, and in this case, if snprintf cuts
off the value it is in fact a potential security issue, one
which did not exist before this.
So make up your mind: is it worth doing the length check or
not? If not, then this switch is uncalled for. If so, then
you should check the return value, else one day I may be able
to whitelist a device which the admin didn't allow.
> }
>
> static int devcgroup_seq_show(struct seq_file *m, void *v)
> --
> 2.51.1
^ permalink raw reply
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Thorsten Blum @ 2025-10-31 15:23 UTC (permalink / raw)
To: David Laight
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031125916.3b0c8b22@pumpkin>
On 31. Oct 2025, at 13:59, David Laight wrote:
> Even if ex->major can be ~0 there are much cleaner ways of writing this code.
Thanks for pointing this out. Looking at the bigger picture makes it
clear that most of the code can actually be removed. What do you think
of this change?
Thanks,
Thorsten
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index a41f558f6fdd..cb845b1fad6b 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -244,7 +244,6 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
#define DEVCG_DENY 2
#define DEVCG_LIST 3
-#define MAJMINLEN 13
#define ACCLEN 4
static void set_access(char *acc, short access)
@@ -270,19 +269,11 @@ static char type_to_char(short type)
return 'X';
}
-static void set_majmin(char *str, unsigned m)
-{
- if (m == ~0)
- strscpy(str, "*", MAJMINLEN);
- else
- snprintf(str, MAJMINLEN, "%u", m);
-}
-
static int devcgroup_seq_show(struct seq_file *m, void *v)
{
struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
struct dev_exception_item *ex;
- char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
+ char acc[ACCLEN];
rcu_read_lock();
/*
@@ -293,17 +284,12 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
*/
if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
set_access(acc, DEVCG_ACC_MASK);
- set_majmin(maj, ~0);
- set_majmin(min, ~0);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
- maj, min, acc);
+ seq_printf(m, "%c *:* %s\n", type_to_char(DEVCG_DEV_ALL), acc);
} else {
list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
set_access(acc, ex->access);
- set_majmin(maj, ex->major);
- set_majmin(min, ex->minor);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
- maj, min, acc);
+ seq_printf(m, "%c %u:%u %s\n", type_to_char(ex->type),
+ ex->major, ex->minor, acc);
}
}
rcu_read_unlock();
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Christian Brauner @ 2025-10-31 15:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <CAHk-=wgZ9x+yxUB9sjete2s9KBiHnPm2+rcwiWNXhx-rpcKxcw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4471 bytes --]
On Fri, Oct 24, 2025 at 01:31:11PM -0700, Linus Torvalds wrote:
> [ Adding LSM people. Also Christian, because he did the cred refcount
Sorry, late to the party. I was working on other stuf. Let me see...
> cleanup with override_creds() and friends last year, and I'm
> suggesting taking that one step further ]
>
> On Fri, 24 Oct 2025 at 06:58, Jens Axboe <axboe@kernel.dk> wrote:
> >
> > Ondrej Mosnacek (1):
> > nbd: override creds to kernel when calling sock_{send,recv}msg()
>
> I've pulled this, but looking at the patch, I note that more than half
> the patch - 75% to be exact - is just boilerplate for "I need to
> allocate the kernel cred and deal with error handling there".
>
> It literally has three lines of new actual useful code (two statements
> and one local variable declaration), and then nine lines of the "setup
> dance".
>
> Which isn't wrong, but when the infrastructure boilerplate is three
> times more than the actual code, it makes me think we should maybe
> just get rid of the
>
> my_kernel_cred = prepare_kernel_cred(&init_task);
>
> pattern for this use-case, and just let people use "init_cred"
> directly for things like this.
>
> Because that's essentially what that prepare_kernel_cred() thing
> returns, except it allocates a new copy of said thing, so now you have
> error handling and you have to free it after-the-fact.
>
> And I'm not seeing that the extra error handling and freeing dance
> actually buys us anything at all.
>
> Now, some *other* users actually go on to change the creds: they want
> that prepare_kernel_cred() dance because they then actually do
> something else like using their own keyring or whatever (eg the NFS
> idmap code or some other filesystem stuff).
>
> So it's not like prepare_kernel_cred() is wrong, but in this kind of
> case where people just go "I'm a driver with hardware access, I want
> to do something with kernel privileges not user privileges", it
> actually seems counterproductive to have extra code just to complicate
> things.
>
> Now, my gut feel is that if we just let people use 'init_cred'
> directly, we should also make sure that it's always exposed as a
> 'const struct cred' , but wouldn't that be a whole lot simpler and
> more straightforward?
>
> This is *not* the only use case of that.
>
> We now have at least four use-cases of this "raw kernel cred" pattern:
> core-dumping over unix domain socket, nbd, firmware loading and SCSI
> target all do this exact thing as far as I can tell.
>
> So they all just want that bare kernel cred, and this interface then
> forces it to do extra work instead of just doing
>
> old_cred = override_creds(&init_cred);
> ...
> revert_creds(old_cred);
>
> and it ends up being extra code for allocating and freeing that copy
> of a cred that we already *had* and could just have used directly.
>
> I did just check that making 'init_cred' be const
Hm, two immediate observations before I go off and write the series.
(1) The thing is that init_cred would have to be exposed to modules via
EXPORT_SYMBOL() for this to work. It would be easier to just force
the use of init_task->cred instead.
That pointer deref won't matter in the face of the allocations and
refcounts we wipe out with this. Then we should also move init_cred
to init/init_task.c and make it static const. Nobody really needs it
currently.
(2) I think the plain override_creds() would work but we can do better.
I envision we can leverage CLASS() to completely hide any access to
init_cred and force a scope with kernel creds.
/me goess off to write that up.
Ok, so I have it and it survives the coredump socket tests. They are a
prime example for this sort of thing. Any unprivileged task needs to be
able to connect to the coredump socket when it coredumps so we override
credentials only for the path lookup. With my patchset this becomes:
if (flags & SOCK_COREDUMP) {
struct path root;
task_lock(&init_task);
get_fs_root(init_task.fs, &root);
task_unlock(&init_task);
scoped_with_kernel_creds()
err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
LOOKUP_NO_MAGICLINKS, &path);
path_put(&root);
if (err)
goto fail;
} else {
Patches appended.
[-- Attachment #2: 0000-creds-add-scoped_-with_kernel_creds.eml --]
[-- Type: message/rfc822, Size: 2435 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 0/6] creds: add {scoped_}with_kernel_creds()
Date: Fri, 31 Oct 2025 16:37:35 +0100
Message-ID: <20251031-work-creds-init_cred-v1-0-cbf0400d6e0e@kernel.org>
Don't needlessly duplicate the initial credentials just to briefly use
them. Avoid all that work and add guards that hide all this away.
Note, using init_task in the macros to get at the credentials has the
advantage that we don't actually need to export init_cred to modules
such as nbd.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (6):
cred: add {scoped_}with_kernel_creds
creds: make init_cred static
firmware: don't copy kernel creds
nbd: don't copy kernel creds
target: don't copy kernel creds
unix: don't copy creds
drivers/base/firmware_loader/main.c | 59 +++++++++++++++--------------------
drivers/block/nbd.c | 17 ++--------
drivers/target/target_core_configfs.c | 14 ++-------
include/linux/cred.h | 25 +++++++++++++++
include/linux/init_task.h | 1 -
init/init_task.c | 24 ++++++++++++++
kernel/cred.c | 24 --------------
net/unix/af_unix.c | 17 +++-------
security/keys/process_keys.c | 2 +-
9 files changed, 83 insertions(+), 100 deletions(-)
---
base-commit: d2818517e3486d11c9bd55aca3e14059e4c69886
change-id: 20251031-work-creds-init_cred-db9556a70d67
[-- Attachment #3: 0001-cred-add-scoped_-with_kernel_creds.eml --]
[-- Type: message/rfc822, Size: 2559 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 1/6] cred: add {scoped_}with_kernel_creds
Date: Fri, 31 Oct 2025 16:37:36 +0100
Message-ID: <20251031-work-creds-init_cred-v1-1-cbf0400d6e0e@kernel.org>
Add a new CLASS(with_kernel_creds) allowing code to run with kernel
creds without having to duplicate them first. Only use the CLASS() never
the plain helpers!
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/cred.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 89ae50ad2ace..8a8d6b3fbadb 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -20,6 +20,8 @@
struct cred;
struct inode;
+extern struct task_struct init_task;
+
/*
* COW Supplementary groups list
*/
@@ -180,6 +182,29 @@ static inline const struct cred *revert_creds(const struct cred *revert_cred)
return rcu_replace_pointer(current->cred, revert_cred, 1);
}
+static inline const struct cred *__assume_kernel_creds(void)
+{
+ return rcu_replace_pointer(current->cred, init_task.cred, 1);
+}
+
+static inline void __yield_kernel_creds(const struct cred *revert_cred)
+{
+ WARN_ON_ONCE(current->cred != init_task.cred);
+ rcu_replace_pointer(current->cred, revert_cred, 1);
+}
+
+DEFINE_CLASS(with_kernel_creds,
+ const struct cred *,
+ __yield_kernel_creds(_T),
+ __assume_kernel_creds(), void)
+
+#define with_kernel_creds() \
+ CLASS(with_kernel_creds, __UNIQUE_ID(cred))()
+
+#define scoped_with_kernel_creds() \
+ for (CLASS(with_kernel_creds, __UNIQUE_ID(cred))(), \
+ *__p = (void *)1; __p; __p = NULL)
+
/**
* get_cred_many - Get references on a set of credentials
* @cred: The credentials to reference
--
2.47.3
[-- Attachment #4: 0002-creds-make-init_cred-static.eml --]
[-- Type: message/rfc822, Size: 4417 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 2/6] creds: make init_cred static
Date: Fri, 31 Oct 2025 16:37:37 +0100
Message-ID: <20251031-work-creds-init_cred-v1-2-cbf0400d6e0e@kernel.org>
There's zero need to expose struct init_cred.
The very few places that need direct access can just go through
init_task and be done with it.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/init_task.h | 1 -
init/init_task.c | 24 ++++++++++++++++++++++++
kernel/cred.c | 24 ------------------------
security/keys/process_keys.c | 2 +-
4 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index bccb3f1f6262..a6cb241ea00c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -25,7 +25,6 @@
extern struct files_struct init_files;
extern struct fs_struct init_fs;
extern struct nsproxy init_nsproxy;
-extern struct cred init_cred;
#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
#define INIT_PREV_CPUTIME(x) .prev_cputime = { \
diff --git a/init/init_task.c b/init/init_task.c
index a55e2189206f..68059eac9a1e 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -62,6 +62,30 @@ unsigned long init_shadow_call_stack[SCS_SIZE / sizeof(long)] = {
};
#endif
+/*
+ * The initial credentials for the initial task
+ */
+static const struct cred init_cred = {
+ .usage = ATOMIC_INIT(4),
+ .uid = GLOBAL_ROOT_UID,
+ .gid = GLOBAL_ROOT_GID,
+ .suid = GLOBAL_ROOT_UID,
+ .sgid = GLOBAL_ROOT_GID,
+ .euid = GLOBAL_ROOT_UID,
+ .egid = GLOBAL_ROOT_GID,
+ .fsuid = GLOBAL_ROOT_UID,
+ .fsgid = GLOBAL_ROOT_GID,
+ .securebits = SECUREBITS_DEFAULT,
+ .cap_inheritable = CAP_EMPTY_SET,
+ .cap_permitted = CAP_FULL_SET,
+ .cap_effective = CAP_FULL_SET,
+ .cap_bset = CAP_FULL_SET,
+ .user = INIT_USER,
+ .user_ns = &init_user_ns,
+ .group_info = &init_groups,
+ .ucounts = &init_ucounts,
+};
+
/*
* Set up the first task table, touch at your own risk!. Base=0,
* limit=0x1fffff (=2MB)
diff --git a/kernel/cred.c b/kernel/cred.c
index dbf6b687dc5c..9ff0b349b80b 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -38,30 +38,6 @@ static struct kmem_cache *cred_jar;
/* init to 2 - one for init_task, one to ensure it is never freed */
static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
-/*
- * The initial credentials for the initial task
- */
-struct cred init_cred = {
- .usage = ATOMIC_INIT(4),
- .uid = GLOBAL_ROOT_UID,
- .gid = GLOBAL_ROOT_GID,
- .suid = GLOBAL_ROOT_UID,
- .sgid = GLOBAL_ROOT_GID,
- .euid = GLOBAL_ROOT_UID,
- .egid = GLOBAL_ROOT_GID,
- .fsuid = GLOBAL_ROOT_UID,
- .fsgid = GLOBAL_ROOT_GID,
- .securebits = SECUREBITS_DEFAULT,
- .cap_inheritable = CAP_EMPTY_SET,
- .cap_permitted = CAP_FULL_SET,
- .cap_effective = CAP_FULL_SET,
- .cap_bset = CAP_FULL_SET,
- .user = INIT_USER,
- .user_ns = &init_user_ns,
- .group_info = &init_groups,
- .ucounts = &init_ucounts,
-};
-
/*
* The RCU callback to actually dispose of a set of credentials
*/
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index b5d5333ab330..98ba8a7d3118 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -51,7 +51,7 @@ static struct key *get_user_register(struct user_namespace *user_ns)
if (!reg_keyring) {
reg_keyring = keyring_alloc(".user_reg",
user_ns->owner, INVALID_GID,
- &init_cred,
+ init_task.cred,
KEY_POS_WRITE | KEY_POS_SEARCH |
KEY_USR_VIEW | KEY_USR_READ,
0,
--
2.47.3
[-- Attachment #5: 0003-firmware-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 3963 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 3/6] firmware: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:38 +0100
Message-ID: <20251031-work-creds-init_cred-v1-3-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/base/firmware_loader/main.c | 59 ++++++++++++++++---------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 6942c62fa59d..bee3050a20d9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -829,8 +829,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
size_t offset, u32 opt_flags)
{
struct firmware *fw = NULL;
- struct cred *kern_cred = NULL;
- const struct cred *old_cred;
bool nondirect = false;
int ret;
@@ -871,45 +869,38 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
* called by a driver when serving an unrelated request from userland, we use
* the kernel credentials to read the file.
*/
- kern_cred = prepare_kernel_cred(&init_task);
- if (!kern_cred) {
- ret = -ENOMEM;
- goto out;
- }
- old_cred = override_creds(kern_cred);
+ scoped_with_kernel_creds() {
+ ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
- ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
-
- /* Only full reads can support decompression, platform, and sysfs. */
- if (!(opt_flags & FW_OPT_PARTIAL))
- nondirect = true;
+ /* Only full reads can support decompression, platform, and sysfs. */
+ if (!(opt_flags & FW_OPT_PARTIAL))
+ nondirect = true;
#ifdef CONFIG_FW_LOADER_COMPRESS_ZSTD
- if (ret == -ENOENT && nondirect)
- ret = fw_get_filesystem_firmware(device, fw->priv, ".zst",
- fw_decompress_zstd);
+ if (ret == -ENOENT && nondirect)
+ ret = fw_get_filesystem_firmware(device, fw->priv, ".zst",
+ fw_decompress_zstd);
#endif
#ifdef CONFIG_FW_LOADER_COMPRESS_XZ
- if (ret == -ENOENT && nondirect)
- ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
- fw_decompress_xz);
+ if (ret == -ENOENT && nondirect)
+ ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
+ fw_decompress_xz);
#endif
- if (ret == -ENOENT && nondirect)
- ret = firmware_fallback_platform(fw->priv);
+ if (ret == -ENOENT && nondirect)
+ ret = firmware_fallback_platform(fw->priv);
- if (ret) {
- if (!(opt_flags & FW_OPT_NO_WARN))
- dev_warn(device,
- "Direct firmware load for %s failed with error %d\n",
- name, ret);
- if (nondirect)
- ret = firmware_fallback_sysfs(fw, name, device,
- opt_flags, ret);
- } else
- ret = assign_fw(fw, device);
-
- revert_creds(old_cred);
- put_cred(kern_cred);
+ if (ret) {
+ if (!(opt_flags & FW_OPT_NO_WARN))
+ dev_warn(device,
+ "Direct firmware load for %s failed with error %d\n",
+ name, ret);
+ if (nondirect)
+ ret = firmware_fallback_sysfs(fw, name, device,
+ opt_flags, ret);
+ } else {
+ ret = assign_fw(fw, device);
+ }
+ }
out:
if (ret < 0) {
--
2.47.3
[-- Attachment #6: 0004-nbd-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 3029 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 4/6] nbd: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:39 +0100
Message-ID: <20251031-work-creds-init_cred-v1-4-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/block/nbd.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index a853c65ac65d..1f0d89e21ec8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -52,7 +52,6 @@
static DEFINE_IDR(nbd_index_idr);
static DEFINE_MUTEX(nbd_index_mutex);
static struct workqueue_struct *nbd_del_wq;
-static struct cred *nbd_cred;
static int nbd_total_devices = 0;
struct nbd_sock {
@@ -555,7 +554,6 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
int result;
struct msghdr msg = {} ;
unsigned int noreclaim_flag;
- const struct cred *old_cred;
if (unlikely(!sock)) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
@@ -564,10 +562,10 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
return -EINVAL;
}
- old_cred = override_creds(nbd_cred);
-
msg.msg_iter = *iter;
+ with_kernel_creds();
+
noreclaim_flag = memalloc_noreclaim_save();
do {
sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
@@ -590,8 +588,6 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
memalloc_noreclaim_restore(noreclaim_flag);
- revert_creds(old_cred);
-
return result;
}
@@ -2683,15 +2679,7 @@ static int __init nbd_init(void)
return -ENOMEM;
}
- nbd_cred = prepare_kernel_cred(&init_task);
- if (!nbd_cred) {
- destroy_workqueue(nbd_del_wq);
- unregister_blkdev(NBD_MAJOR, "nbd");
- return -ENOMEM;
- }
-
if (genl_register_family(&nbd_genl_family)) {
- put_cred(nbd_cred);
destroy_workqueue(nbd_del_wq);
unregister_blkdev(NBD_MAJOR, "nbd");
return -EINVAL;
@@ -2746,7 +2734,6 @@ static void __exit nbd_cleanup(void)
/* Also wait for nbd_dev_remove_work() completes */
destroy_workqueue(nbd_del_wq);
- put_cred(nbd_cred);
idr_destroy(&nbd_index_idr);
unregister_blkdev(NBD_MAJOR, "nbd");
}
--
2.47.3
[-- Attachment #7: 0005-target-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 2249 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 5/6] target: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:40 +0100
Message-ID: <20251031-work-creds-init_cred-v1-5-cbf0400d6e0e@kernel.org>
Get rid of all the boilerplate and tightly scope when the task runs with
kernel creds.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/target/target_core_configfs.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index b19acd662726..9e51c535ba8c 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -3670,8 +3670,6 @@ static int __init target_core_init_configfs(void)
{
struct configfs_subsystem *subsys = &target_core_fabrics;
struct t10_alua_lu_gp *lu_gp;
- struct cred *kern_cred;
- const struct cred *old_cred;
int ret;
pr_debug("TARGET_CORE[0]: Loading Generic Kernel Storage"
@@ -3748,16 +3746,8 @@ static int __init target_core_init_configfs(void)
if (ret < 0)
goto out;
- /* We use the kernel credentials to access the target directory */
- kern_cred = prepare_kernel_cred(&init_task);
- if (!kern_cred) {
- ret = -ENOMEM;
- goto out;
- }
- old_cred = override_creds(kern_cred);
- target_init_dbroot();
- revert_creds(old_cred);
- put_cred(kern_cred);
+ scoped_with_kernel_creds()
+ target_init_dbroot();
return 0;
--
2.47.3
[-- Attachment #8: 0006-unix-don-t-copy-creds.eml --]
[-- Type: message/rfc822, Size: 2244 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 6/6] unix: don't copy creds
Date: Fri, 31 Oct 2025 16:37:41 +0100
Message-ID: <20251031-work-creds-init_cred-v1-6-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
net/unix/af_unix.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 768098dec231..68c94f49f7b5 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1210,25 +1210,16 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
unix_mkname_bsd(sunaddr, addr_len);
if (flags & SOCK_COREDUMP) {
- const struct cred *cred;
- struct cred *kcred;
struct path root;
- kcred = prepare_kernel_cred(&init_task);
- if (!kcred) {
- err = -ENOMEM;
- goto fail;
- }
-
task_lock(&init_task);
get_fs_root(init_task.fs, &root);
task_unlock(&init_task);
- cred = override_creds(kcred);
- err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
- LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
- LOOKUP_NO_MAGICLINKS, &path);
- put_cred(revert_creds(cred));
+ scoped_with_kernel_creds()
+ err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
+ LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
+ LOOKUP_NO_MAGICLINKS, &path);
path_put(&root);
if (err)
goto fail;
--
2.47.3
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Christian Brauner @ 2025-10-31 15:53 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <20251031-zerkratzen-privileg-77a7fb326e34@brauner>
On Fri, Oct 31, 2025 at 04:43:54PM +0100, Christian Brauner wrote:
> On Fri, Oct 24, 2025 at 01:31:11PM -0700, Linus Torvalds wrote:
> > [ Adding LSM people. Also Christian, because he did the cred refcount
>
> Sorry, late to the party. I was working on other stuf. Let me see...
>
> > cleanup with override_creds() and friends last year, and I'm
> > suggesting taking that one step further ]
> >
> > On Fri, 24 Oct 2025 at 06:58, Jens Axboe <axboe@kernel.dk> wrote:
> > >
> > > Ondrej Mosnacek (1):
> > > nbd: override creds to kernel when calling sock_{send,recv}msg()
> >
> > I've pulled this, but looking at the patch, I note that more than half
> > the patch - 75% to be exact - is just boilerplate for "I need to
> > allocate the kernel cred and deal with error handling there".
> >
> > It literally has three lines of new actual useful code (two statements
> > and one local variable declaration), and then nine lines of the "setup
> > dance".
> >
> > Which isn't wrong, but when the infrastructure boilerplate is three
> > times more than the actual code, it makes me think we should maybe
> > just get rid of the
> >
> > my_kernel_cred = prepare_kernel_cred(&init_task);
> >
> > pattern for this use-case, and just let people use "init_cred"
> > directly for things like this.
> >
> > Because that's essentially what that prepare_kernel_cred() thing
> > returns, except it allocates a new copy of said thing, so now you have
> > error handling and you have to free it after-the-fact.
> >
> > And I'm not seeing that the extra error handling and freeing dance
> > actually buys us anything at all.
> >
> > Now, some *other* users actually go on to change the creds: they want
> > that prepare_kernel_cred() dance because they then actually do
> > something else like using their own keyring or whatever (eg the NFS
> > idmap code or some other filesystem stuff).
> >
> > So it's not like prepare_kernel_cred() is wrong, but in this kind of
> > case where people just go "I'm a driver with hardware access, I want
> > to do something with kernel privileges not user privileges", it
> > actually seems counterproductive to have extra code just to complicate
> > things.
> >
> > Now, my gut feel is that if we just let people use 'init_cred'
> > directly, we should also make sure that it's always exposed as a
> > 'const struct cred' , but wouldn't that be a whole lot simpler and
> > more straightforward?
> >
> > This is *not* the only use case of that.
> >
> > We now have at least four use-cases of this "raw kernel cred" pattern:
> > core-dumping over unix domain socket, nbd, firmware loading and SCSI
> > target all do this exact thing as far as I can tell.
> >
> > So they all just want that bare kernel cred, and this interface then
> > forces it to do extra work instead of just doing
> >
> > old_cred = override_creds(&init_cred);
> > ...
> > revert_creds(old_cred);
> >
> > and it ends up being extra code for allocating and freeing that copy
> > of a cred that we already *had* and could just have used directly.
> >
> > I did just check that making 'init_cred' be const
>
> Hm, two immediate observations before I go off and write the series.
>
> (1) The thing is that init_cred would have to be exposed to modules via
> EXPORT_SYMBOL() for this to work. It would be easier to just force
> the use of init_task->cred instead.
>
> That pointer deref won't matter in the face of the allocations and
> refcounts we wipe out with this. Then we should also move init_cred
> to init/init_task.c and make it static const. Nobody really needs it
> currently.
>
> (2) I think the plain override_creds() would work but we can do better.
> I envision we can leverage CLASS() to completely hide any access to
> init_cred and force a scope with kernel creds.
>
> /me goess off to write that up.
>
> Ok, so I have it and it survives the coredump socket tests. They are a
> prime example for this sort of thing. Any unprivileged task needs to be
> able to connect to the coredump socket when it coredumps so we override
> credentials only for the path lookup. With my patchset this becomes:
>
> if (flags & SOCK_COREDUMP) {
> struct path root;
>
> task_lock(&init_task);
> get_fs_root(init_task.fs, &root);
> task_unlock(&init_task);
>
> scoped_with_kernel_creds()
> err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
> LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> LOOKUP_NO_MAGICLINKS, &path);
> path_put(&root);
> if (err)
> goto fail;
> } else {
>
> Patches appended.
> Date: Fri, 31 Oct 2025 16:37:35 +0100
> From: Christian Brauner <brauner@kernel.org>
> To: Christian Brauner <brauner@kernel.org>
> Subject: [PATCH 0/6] creds: add {scoped_}with_kernel_creds()
> Message-Id: <20251031-work-creds-init_cred-v1-0-cbf0400d6e0e@kernel.org>
Needs one diff I forgot to fold:
diff --git a/init/init_task.c b/init/init_task.c
index 68059eac9a1e..15288e62334f 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -62,6 +62,9 @@ unsigned long init_shadow_call_stack[SCS_SIZE / sizeof(long)] = {
};
#endif
+/* init to 2 - one for init_task, one to ensure it is never freed */
+static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
+
/*
* The initial credentials for the initial task
*/
diff --git a/kernel/cred.c b/kernel/cred.c
index 9ff0b349b80b..ac87ed9d43b1 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -35,9 +35,6 @@ do { \
static struct kmem_cache *cred_jar;
-/* init to 2 - one for init_task, one to ensure it is never freed */
-static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
-
/*
* The RCU callback to actually dispose of a set of credentials
*/
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Linus Torvalds @ 2025-10-31 16:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <20251031-zerkratzen-privileg-77a7fb326e34@brauner>
On Fri, 31 Oct 2025 at 08:44, Christian Brauner <brauner@kernel.org> wrote:
>
> Hm, two immediate observations before I go off and write the series.
>
> (1) The thing is that init_cred would have to be exposed to modules via
> EXPORT_SYMBOL() for this to work. It would be easier to just force
> the use of init_task->cred instead.
Yea, I guess we already export that.
> That pointer deref won't matter in the face of the allocations and
> refcounts we wipe out with this. Then we should also move init_cred
> to init/init_task.c and make it static const. Nobody really needs it
> currently.
Well, I did the "does it compile ok" with it marked as 'const', but as
mentioned, those 'struct cred' instances aren't *really* const, they
are only pseudo-const things in that they are *marked* const so that
nobody modifies them by mistake, but then the ref-counting will cast
the constness away in order to update references.
So I don't think we can *actually* mark it "static const", because
that will put the data structure in the const data section, and then
the refcounting will trigger kernel page faults.
End result: I think we can indeed move it to init/init_task.c. And
yes, we can and should make it static to that file, but not plain
'const'.
If we expose it to others - but I think you're right that maybe it's
not a good idea - we should *expose* it as a 'const' data structure.
But we should probably put it in some explicitly writable section (I
was going to suggest marking it "__read_mostly", but it turns out some
architectures #define that to be empty, so a "const __read_mosyly"
data structure could still end up in a read-only section).
> (2) I think the plain override_creds() would work but we can do better.
> I envision we can leverage CLASS() to completely hide any access to
> init_cred and force a scope with kernel creds.
Ack.
Linus
^ 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