* [PATCH v5 5/7] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Andrey Albershteyn @ 2025-05-12 13:22 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250512-xattrat-syscall-v5-0-ffbc7c477332@kernel.org>
Future patches will add new syscalls which use these functions. As
this interface won't be used for ioctls only the EOPNOSUPP is more
appropriate return code.
This patch coverts return code from ENOIOCTLCMD to EOPNOSUPP for
vfs_fileattr_get and vfs_fileattr_set. To save old behavior
translate EOPNOSUPP back for current users - overlayfs, encryptfs
and fs/ioctl.c.
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
---
fs/ecryptfs/inode.c | 8 +++++++-
fs/file_attr.c | 12 ++++++++++--
fs/overlayfs/inode.c | 2 +-
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 51a5c54eb740..6bf08ff4d7f7 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -1124,7 +1124,13 @@ static int ecryptfs_removexattr(struct dentry *dentry, struct inode *inode,
static int ecryptfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
{
- return vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa);
+ int rc;
+
+ rc = vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa);
+ if (rc == -EOPNOTSUPP)
+ rc = -ENOIOCTLCMD;
+
+ return rc;
}
static int ecryptfs_fileattr_set(struct mnt_idmap *idmap,
diff --git a/fs/file_attr.c b/fs/file_attr.c
index d9eab553dc25..d696f440fa4f 100644
--- a/fs/file_attr.c
+++ b/fs/file_attr.c
@@ -79,7 +79,7 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
int error;
if (!inode->i_op->fileattr_get)
- return -ENOIOCTLCMD;
+ return -EOPNOTSUPP;
error = security_inode_file_getattr(dentry, fa);
if (error)
@@ -239,7 +239,7 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
int err;
if (!inode->i_op->fileattr_set)
- return -ENOIOCTLCMD;
+ return -EOPNOTSUPP;
if (!inode_owner_or_capable(idmap, inode))
return -EPERM;
@@ -281,6 +281,8 @@ int ioctl_getflags(struct file *file, unsigned int __user *argp)
int err;
err = vfs_fileattr_get(file->f_path.dentry, &fa);
+ if (err == -EOPNOTSUPP)
+ err = -ENOIOCTLCMD;
if (!err)
err = put_user(fa.flags, argp);
return err;
@@ -302,6 +304,8 @@ int ioctl_setflags(struct file *file, unsigned int __user *argp)
fileattr_fill_flags(&fa, flags);
err = vfs_fileattr_set(idmap, dentry, &fa);
mnt_drop_write_file(file);
+ if (err == -EOPNOTSUPP)
+ err = -ENOIOCTLCMD;
}
}
return err;
@@ -314,6 +318,8 @@ int ioctl_fsgetxattr(struct file *file, void __user *argp)
int err;
err = vfs_fileattr_get(file->f_path.dentry, &fa);
+ if (err == -EOPNOTSUPP)
+ err = -ENOIOCTLCMD;
if (!err)
err = copy_fsxattr_to_user(&fa, argp);
@@ -334,6 +340,8 @@ int ioctl_fssetxattr(struct file *file, void __user *argp)
if (!err) {
err = vfs_fileattr_set(idmap, dentry, &fa);
mnt_drop_write_file(file);
+ if (err == -EOPNOTSUPP)
+ err = -ENOIOCTLCMD;
}
}
return err;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 6f0e15f86c21..096d44712bb1 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -721,7 +721,7 @@ int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa)
return err;
err = vfs_fileattr_get(realpath->dentry, fa);
- if (err == -ENOIOCTLCMD)
+ if (err == -EOPNOTSUPP)
err = -ENOTTY;
return err;
}
--
2.47.2
--
- Andrey
^ permalink raw reply related
* [PATCH v5 4/7] fs: split fileattr/fsxattr converters into helpers
From: Andrey Albershteyn @ 2025-05-12 13:18 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250512-xattrat-syscall-v5-0-a88b20e37aae@kernel.org>
This will be helpful for file_get/setattr syscalls to convert
between fileattr and fsxattr.
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/file_attr.c | 32 +++++++++++++++++++++-----------
include/linux/fileattr.h | 2 ++
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/fs/file_attr.c b/fs/file_attr.c
index be62d97cc444..d9eab553dc25 100644
--- a/fs/file_attr.c
+++ b/fs/file_attr.c
@@ -89,6 +89,16 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
}
EXPORT_SYMBOL(vfs_fileattr_get);
+void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
+{
+ memset(fsx, 0, sizeof(struct fsxattr));
+ fsx->fsx_xflags = fa->fsx_xflags;
+ fsx->fsx_extsize = fa->fsx_extsize;
+ fsx->fsx_nextents = fa->fsx_nextents;
+ fsx->fsx_projid = fa->fsx_projid;
+ fsx->fsx_cowextsize = fa->fsx_cowextsize;
+}
+
/**
* copy_fsxattr_to_user - copy fsxattr to userspace.
* @fa: fileattr pointer
@@ -100,12 +110,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
{
struct fsxattr xfa;
- memset(&xfa, 0, sizeof(xfa));
- xfa.fsx_xflags = fa->fsx_xflags;
- xfa.fsx_extsize = fa->fsx_extsize;
- xfa.fsx_nextents = fa->fsx_nextents;
- xfa.fsx_projid = fa->fsx_projid;
- xfa.fsx_cowextsize = fa->fsx_cowextsize;
+ fileattr_to_fsxattr(fa, &xfa);
if (copy_to_user(ufa, &xfa, sizeof(xfa)))
return -EFAULT;
@@ -114,6 +119,15 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
}
EXPORT_SYMBOL(copy_fsxattr_to_user);
+void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
+{
+ fileattr_fill_xflags(fa, fsx->fsx_xflags);
+ fa->fsx_extsize = fsx->fsx_extsize;
+ fa->fsx_nextents = fsx->fsx_nextents;
+ fa->fsx_projid = fsx->fsx_projid;
+ fa->fsx_cowextsize = fsx->fsx_cowextsize;
+}
+
static int copy_fsxattr_from_user(struct fileattr *fa,
struct fsxattr __user *ufa)
{
@@ -122,11 +136,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
if (copy_from_user(&xfa, ufa, sizeof(xfa)))
return -EFAULT;
- fileattr_fill_xflags(fa, xfa.fsx_xflags);
- fa->fsx_extsize = xfa.fsx_extsize;
- fa->fsx_nextents = xfa.fsx_nextents;
- fa->fsx_projid = xfa.fsx_projid;
- fa->fsx_cowextsize = xfa.fsx_cowextsize;
+ fsxattr_to_fileattr(&xfa, fa);
return 0;
}
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 6030d0bf7ad3..433efa0f4784 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -33,7 +33,9 @@ struct fileattr {
bool fsx_valid:1;
};
+void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx);
int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
+void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
void fileattr_fill_flags(struct fileattr *fa, u32 flags);
--
2.47.2
^ permalink raw reply related
* [PATCH v5 3/7] selinux: implement inode_file_[g|s]etattr hooks
From: Andrey Albershteyn @ 2025-05-12 13:18 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250512-xattrat-syscall-v5-0-a88b20e37aae@kernel.org>
These hooks are called on inode extended attribute retrieval/change.
Cc: selinux@vger.kernel.org
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
---
security/selinux/hooks.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7a7dcab81db..9c6e264b090f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3366,6 +3366,18 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap,
return -EACCES;
}
+static int selinux_inode_file_setattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+}
+
+static int selinux_inode_file_getattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return dentry_has_perm(current_cred(), dentry, FILE__GETATTR);
+}
+
static int selinux_path_notify(const struct path *path, u64 mask,
unsigned int obj_type)
{
@@ -7272,6 +7284,8 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr),
LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
+ LSM_HOOK_INIT(inode_file_getattr, selinux_inode_file_getattr),
+ LSM_HOOK_INIT(inode_file_setattr, selinux_inode_file_setattr),
LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl),
LSM_HOOK_INIT(inode_get_acl, selinux_inode_get_acl),
LSM_HOOK_INIT(inode_remove_acl, selinux_inode_remove_acl),
--
2.47.2
^ permalink raw reply related
* [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls
From: Andrey Albershteyn @ 2025-05-12 13:18 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn, Andrey Albershteyn
This patchset introduced two new syscalls file_getattr() and
file_setattr(). These syscalls are similar to FS_IOC_FSSETXATTR ioctl()
except they use *at() semantics. Therefore, there's no need to open the
file to get a fd.
These syscalls allow userspace to set filesystem inode attributes on
special files. One of the usage examples is XFS quota projects.
XFS has project quotas which could be attached to a directory. All
new inodes in these directories inherit project ID set on parent
directory.
The project is created from userspace by opening and calling
FS_IOC_FSSETXATTR on each inode. This is not possible for special
files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
with empty project ID. Those inodes then are not shown in the quota
accounting but still exist in the directory. This is not critical but in
the case when special files are created in the directory with already
existing project quota, these new inodes inherit extended attributes.
This creates a mix of special files with and without attributes.
Moreover, special files with attributes don't have a possibility to
become clear or change the attributes. This, in turn, prevents userspace
from re-creating quota project on these existing files.
NAME
file_getattr/file_setattr - get/set filesystem inode attributes
SYNOPSIS
#include <sys/syscall.h> /* Definition of SYS_* constants */
#include <unistd.h>
long syscall(SYS_file_getattr, int dirfd, const char *pathname,
struct fsxattr *fsx, size_t size, unsigned int at_flags);
long syscall(SYS_file_setattr, int dirfd, const char *pathname,
struct fsxattr *fsx, size_t size, unsigned int at_flags);
Note: glibc doesn't provide for file_getattr()/file_setattr(),
use syscall(2) instead.
DESCRIPTION
The syscalls take fd and path. If path is absolute, fd is not
used. If path is empty, fd can be AT_FDCWD or any valid fd which
will be used to get/set attributes on.
This is an alternative to FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR
ioctl with a difference that file don't need to be open as we
can reference it with a path instead of fd. By having this we
can manipulated filesystem inode attributes not only on regular
files but also on special ones. This is not possible with
FS_IOC_FSSETXATTR ioctl as with special files we can not call
ioctl() directly on the filesystem inode using file descriptor.
at_flags can be set to AT_SYMLINK_NOFOLLOW or AT_EMPTY_PATH.
RETURN VALUE
On success, 0 is returned. On error, -1 is returned, and errno
is set to indicate the error.
ERRORS
EINVAL Invalid at_flag specified (only
AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH is
supported).
EINVAL Size was smaller than any known version of
struct fsxattr.
EINVAL Invalid combination of parameters provided in
fsxattr for this type of file.
E2BIG Size of input argument **struct fsxattr** is too
big.
EBADF Invalid file descriptor was provided.
EPERM No permission to change this file.
EOPNOTSUPP Filesystem does not support setting attributes
on this type of inode
HISTORY
Added in Linux 6.15.
EXAMPLE
Create directory and file "mkdir ./dir && touch ./dir/foo" and then
execute the following program:
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <linux/fs.h>
#include <stdio.h>
#include <sys/syscall.h>
#include <unistd.h>
int
main(int argc, char **argv) {
int dfd;
int error;
struct fsxattr fsx;
dfd = open("./dir", O_RDONLY);
if (dfd == -1) {
printf("can not open ./dir");
return dfd;
}
error = syscall(467, dfd, "./foo", &fsx, 0);
if (error) {
printf("can not call 467: %s", strerror(errno));
return error;
}
printf("dir/foo flags: %d\n", fsx.fsx_xflags);
fsx.fsx_xflags |= FS_XFLAG_NODUMP;
error = syscall(468, dfd, "./foo", &fsx, 0);
if (error) {
printf("can not call 468: %s", strerror(errno));
return error;
}
printf("dir/foo flags: %d\n", fsx.fsx_xflags);
return error;
}
SEE ALSO
ioctl(2), ioctl_iflags(2), ioctl_xfs_fsgetxattr(2)
---
Changes in v5:
- Remove setting of LOOKUP_EMPTY flags which does not have any effect
- Return -ENOSUPP from vfs_fileattr_set()
- Add fsxattr masking (by Amir)
- Fix UAF issue dentry
- Fix getname_maybe_null() issue with NULL path
- Implement file_getattr/file_setattr hooks
- Return LSM return code from file_setattr
- Rename from getfsxattrat/setfsxattrat to file_getattr/file_setattr
- Link to v4: https://lore.kernel.org/r/20250321-xattrat-syscall-v4-0-3e82e6fb3264@kernel.org
Changes in v4:
- Use getname_maybe_null() for correct handling of dfd + path semantic
- Remove restriction for special files on which flags are allowed
- Utilize copy_struct_from_user() for better future compatibility
- Add draft man page to cover letter
- Convert -ENOIOCTLCMD to -EOPNOSUPP as more appropriate for syscall
- Add missing __user to header declaration of syscalls
- Link to v3: https://lore.kernel.org/r/20250211-xattrat-syscall-v3-1-a07d15f898b2@kernel.org
Changes in v3:
- Remove unnecessary "dfd is dir" check as it checked in user_path_at()
- Remove unnecessary "same filesystem" check
- Use CLASS() instead of directly calling fdget/fdput
- Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org
v1:
https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
Previous discussion:
https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
---
Amir Goldstein (1):
fs: prepare for extending file_get/setattr()
Andrey Albershteyn (6):
fs: split fileattr related helpers into separate file
lsm: introduce new hooks for setting/getting inode fsxattr
selinux: implement inode_file_[g|s]etattr hooks
fs: split fileattr/fsxattr converters into helpers
fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
fs: introduce file_getattr and file_setattr syscalls
arch/alpha/kernel/syscalls/syscall.tbl | 2 +
arch/arm/tools/syscall.tbl | 2 +
arch/arm64/tools/syscall_32.tbl | 2 +
arch/m68k/kernel/syscalls/syscall.tbl | 2 +
arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
arch/parisc/kernel/syscalls/syscall.tbl | 2 +
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
arch/s390/kernel/syscalls/syscall.tbl | 2 +
arch/sh/kernel/syscalls/syscall.tbl | 2 +
arch/sparc/kernel/syscalls/syscall.tbl | 2 +
arch/x86/entry/syscalls/syscall_32.tbl | 2 +
arch/x86/entry/syscalls/syscall_64.tbl | 2 +
arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
fs/Makefile | 3 +-
fs/ecryptfs/inode.c | 8 +-
fs/file_attr.c | 475 ++++++++++++++++++++++++++++
fs/ioctl.c | 309 ------------------
fs/overlayfs/inode.c | 2 +-
include/linux/fileattr.h | 26 ++
include/linux/lsm_hook_defs.h | 2 +
include/linux/security.h | 16 +
include/linux/syscalls.h | 6 +
include/uapi/asm-generic/unistd.h | 8 +-
include/uapi/linux/fs.h | 3 +
security/security.c | 30 ++
security/selinux/hooks.c | 14 +
29 files changed, 621 insertions(+), 313 deletions(-)
---
base-commit: 0d8d44db295ccad20052d6301ef49ff01fb8ae2d
change-id: 20250114-xattrat-syscall-6a1136d2db59
Best regards,
--
Andrey Albershteyn <aalbersh@kernel.org>
^ permalink raw reply
* [PATCH v5 2/7] lsm: introduce new hooks for setting/getting inode fsxattr
From: Andrey Albershteyn @ 2025-05-12 13:18 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250512-xattrat-syscall-v5-0-a88b20e37aae@kernel.org>
Introduce new hooks for setting and getting filesystem extended
attributes on inode (FS_IOC_FSGETXATTR).
Cc: selinux@vger.kernel.org
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
---
fs/file_attr.c | 19 ++++++++++++++++---
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 16 ++++++++++++++++
security/security.c | 30 ++++++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/fs/file_attr.c b/fs/file_attr.c
index 2910b7047721..be62d97cc444 100644
--- a/fs/file_attr.c
+++ b/fs/file_attr.c
@@ -76,10 +76,15 @@ EXPORT_SYMBOL(fileattr_fill_flags);
int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
{
struct inode *inode = d_inode(dentry);
+ int error;
if (!inode->i_op->fileattr_get)
return -ENOIOCTLCMD;
+ error = security_inode_file_getattr(dentry, fa);
+ if (error)
+ return error;
+
return inode->i_op->fileattr_get(dentry, fa);
}
EXPORT_SYMBOL(vfs_fileattr_get);
@@ -242,12 +247,20 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
} else {
fa->flags |= old_ma.flags & ~FS_COMMON_FL;
}
+
err = fileattr_set_prepare(inode, &old_ma, fa);
- if (!err)
- err = inode->i_op->fileattr_set(idmap, dentry, fa);
+ if (err)
+ goto out;
+ err = security_inode_file_setattr(dentry, fa);
+ if (err)
+ goto out;
+ err = inode->i_op->fileattr_set(idmap, dentry, fa);
+ if (err)
+ goto out;
}
+
+out:
inode_unlock(inode);
-
return err;
}
EXPORT_SYMBOL(vfs_fileattr_set);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index bf3bbac4e02a..9600a4350e79 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -157,6 +157,8 @@ LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
struct dentry *dentry, const char *name)
LSM_HOOK(void, LSM_RET_VOID, inode_post_removexattr, struct dentry *dentry,
const char *name)
+LSM_HOOK(int, 0, inode_file_setattr, struct dentry *dentry, struct fileattr *fa)
+LSM_HOOK(int, 0, inode_file_getattr, struct dentry *dentry, struct fileattr *fa)
LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
LSM_HOOK(void, LSM_RET_VOID, inode_post_set_acl, struct dentry *dentry,
diff --git a/include/linux/security.h b/include/linux/security.h
index cc9b54d95d22..d2da2f654345 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -451,6 +451,10 @@ int security_inode_listxattr(struct dentry *dentry);
int security_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name);
void security_inode_post_removexattr(struct dentry *dentry, const char *name);
+int security_inode_file_setattr(struct dentry *dentry,
+ struct fileattr *fa);
+int security_inode_file_getattr(struct dentry *dentry,
+ struct fileattr *fa);
int security_inode_need_killpriv(struct dentry *dentry);
int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
int security_inode_getsecurity(struct mnt_idmap *idmap,
@@ -1053,6 +1057,18 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
const char *name)
{ }
+static inline int security_inode_file_setattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return 0;
+}
+
+static inline int security_inode_file_getattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return 0;
+}
+
static inline int security_inode_need_killpriv(struct dentry *dentry)
{
return cap_inode_need_killpriv(dentry);
diff --git a/security/security.c b/security/security.c
index fb57e8fddd91..09c891e6027d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2622,6 +2622,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
call_void_hook(inode_post_removexattr, dentry, name);
}
+/**
+ * security_inode_file_setattr() - check if setting fsxattr is allowed
+ * @dentry: file to set filesystem extended attributes on
+ * @fa: extended attributes to set on the inode
+ *
+ * Called when file_setattr() syscall or FS_IOC_FSSETXATTR ioctl() is called on
+ * inode
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_inode_file_setattr(struct dentry *dentry, struct fileattr *fa)
+{
+ return call_int_hook(inode_file_setattr, dentry, fa);
+}
+
+/**
+ * security_inode_file_getattr() - check if retrieving fsxattr is allowed
+ * @dentry: file to retrieve filesystem extended attributes from
+ * @fa: extended attributes to get
+ *
+ * Called when file_getattr() syscall or FS_IOC_FSGETXATTR ioctl() is called on
+ * inode
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_inode_file_getattr(struct dentry *dentry, struct fileattr *fa)
+{
+ return call_int_hook(inode_file_getattr, dentry, fa);
+}
+
/**
* security_inode_need_killpriv() - Check if security_inode_killpriv() required
* @dentry: associated dentry
--
2.47.2
^ permalink raw reply related
* [PATCH v5 1/7] fs: split fileattr related helpers into separate file
From: Andrey Albershteyn @ 2025-05-12 13:18 UTC (permalink / raw)
To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Tyler Hicks, Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250512-xattrat-syscall-v5-0-a88b20e37aae@kernel.org>
From: Andrey Albershteyn <aalbersh@kernel.org>
This patch moves function related to file extended attributes manipulations to
separate file. Just refactoring.
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
---
fs/Makefile | 3 +-
fs/file_attr.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++
fs/ioctl.c | 309 ---------------------------------------------
include/linux/fileattr.h | 4 +
4 files changed, 324 insertions(+), 310 deletions(-)
diff --git a/fs/Makefile b/fs/Makefile
index 77fd7f7b5d02..2f1daaea86da 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -15,7 +15,8 @@ obj-y := open.o read_write.o file_table.o super.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
fs_types.o fs_context.o fs_parser.o fsopen.o init.o \
- kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o
+ kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \
+ file_attr.o
obj-$(CONFIG_BUFFER_HEAD) += buffer.o mpage.o
obj-$(CONFIG_PROC_FS) += proc_namespace.o
diff --git a/fs/file_attr.c b/fs/file_attr.c
new file mode 100644
index 000000000000..2910b7047721
--- /dev/null
+++ b/fs/file_attr.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/fscrypt.h>
+#include <linux/fileattr.h>
+
+/**
+ * fileattr_fill_xflags - initialize fileattr with xflags
+ * @fa: fileattr pointer
+ * @xflags: FS_XFLAG_* flags
+ *
+ * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
+ * other fields are zeroed.
+ */
+void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
+{
+ memset(fa, 0, sizeof(*fa));
+ fa->fsx_valid = true;
+ fa->fsx_xflags = xflags;
+ if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
+ fa->flags |= FS_IMMUTABLE_FL;
+ if (fa->fsx_xflags & FS_XFLAG_APPEND)
+ fa->flags |= FS_APPEND_FL;
+ if (fa->fsx_xflags & FS_XFLAG_SYNC)
+ fa->flags |= FS_SYNC_FL;
+ if (fa->fsx_xflags & FS_XFLAG_NOATIME)
+ fa->flags |= FS_NOATIME_FL;
+ if (fa->fsx_xflags & FS_XFLAG_NODUMP)
+ fa->flags |= FS_NODUMP_FL;
+ if (fa->fsx_xflags & FS_XFLAG_DAX)
+ fa->flags |= FS_DAX_FL;
+ if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
+ fa->flags |= FS_PROJINHERIT_FL;
+}
+EXPORT_SYMBOL(fileattr_fill_xflags);
+
+/**
+ * fileattr_fill_flags - initialize fileattr with flags
+ * @fa: fileattr pointer
+ * @flags: FS_*_FL flags
+ *
+ * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
+ * All other fields are zeroed.
+ */
+void fileattr_fill_flags(struct fileattr *fa, u32 flags)
+{
+ memset(fa, 0, sizeof(*fa));
+ fa->flags_valid = true;
+ fa->flags = flags;
+ if (fa->flags & FS_SYNC_FL)
+ fa->fsx_xflags |= FS_XFLAG_SYNC;
+ if (fa->flags & FS_IMMUTABLE_FL)
+ fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
+ if (fa->flags & FS_APPEND_FL)
+ fa->fsx_xflags |= FS_XFLAG_APPEND;
+ if (fa->flags & FS_NODUMP_FL)
+ fa->fsx_xflags |= FS_XFLAG_NODUMP;
+ if (fa->flags & FS_NOATIME_FL)
+ fa->fsx_xflags |= FS_XFLAG_NOATIME;
+ if (fa->flags & FS_DAX_FL)
+ fa->fsx_xflags |= FS_XFLAG_DAX;
+ if (fa->flags & FS_PROJINHERIT_FL)
+ fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
+}
+EXPORT_SYMBOL(fileattr_fill_flags);
+
+/**
+ * vfs_fileattr_get - retrieve miscellaneous file attributes
+ * @dentry: the object to retrieve from
+ * @fa: fileattr pointer
+ *
+ * Call i_op->fileattr_get() callback, if exists.
+ *
+ * Return: 0 on success, or a negative error on failure.
+ */
+int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
+{
+ struct inode *inode = d_inode(dentry);
+
+ if (!inode->i_op->fileattr_get)
+ return -ENOIOCTLCMD;
+
+ return inode->i_op->fileattr_get(dentry, fa);
+}
+EXPORT_SYMBOL(vfs_fileattr_get);
+
+/**
+ * copy_fsxattr_to_user - copy fsxattr to userspace.
+ * @fa: fileattr pointer
+ * @ufa: fsxattr user pointer
+ *
+ * Return: 0 on success, or -EFAULT on failure.
+ */
+int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
+{
+ struct fsxattr xfa;
+
+ memset(&xfa, 0, sizeof(xfa));
+ xfa.fsx_xflags = fa->fsx_xflags;
+ xfa.fsx_extsize = fa->fsx_extsize;
+ xfa.fsx_nextents = fa->fsx_nextents;
+ xfa.fsx_projid = fa->fsx_projid;
+ xfa.fsx_cowextsize = fa->fsx_cowextsize;
+
+ if (copy_to_user(ufa, &xfa, sizeof(xfa)))
+ return -EFAULT;
+
+ return 0;
+}
+EXPORT_SYMBOL(copy_fsxattr_to_user);
+
+static int copy_fsxattr_from_user(struct fileattr *fa,
+ struct fsxattr __user *ufa)
+{
+ struct fsxattr xfa;
+
+ if (copy_from_user(&xfa, ufa, sizeof(xfa)))
+ return -EFAULT;
+
+ fileattr_fill_xflags(fa, xfa.fsx_xflags);
+ fa->fsx_extsize = xfa.fsx_extsize;
+ fa->fsx_nextents = xfa.fsx_nextents;
+ fa->fsx_projid = xfa.fsx_projid;
+ fa->fsx_cowextsize = xfa.fsx_cowextsize;
+
+ return 0;
+}
+
+/*
+ * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
+ * any invalid configurations.
+ *
+ * Note: must be called with inode lock held.
+ */
+static int fileattr_set_prepare(struct inode *inode,
+ const struct fileattr *old_ma,
+ struct fileattr *fa)
+{
+ int err;
+
+ /*
+ * The IMMUTABLE and APPEND_ONLY flags can only be changed by
+ * the relevant capability.
+ */
+ if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
+ !capable(CAP_LINUX_IMMUTABLE))
+ return -EPERM;
+
+ err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
+ if (err)
+ return err;
+
+ /*
+ * Project Quota ID state is only allowed to change from within the init
+ * namespace. Enforce that restriction only if we are trying to change
+ * the quota ID state. Everything else is allowed in user namespaces.
+ */
+ if (current_user_ns() != &init_user_ns) {
+ if (old_ma->fsx_projid != fa->fsx_projid)
+ return -EINVAL;
+ if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
+ FS_XFLAG_PROJINHERIT)
+ return -EINVAL;
+ } else {
+ /*
+ * Caller is allowed to change the project ID. If it is being
+ * changed, make sure that the new value is valid.
+ */
+ if (old_ma->fsx_projid != fa->fsx_projid &&
+ !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
+ return -EINVAL;
+ }
+
+ /* Check extent size hints. */
+ if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
+ return -EINVAL;
+
+ if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
+ !S_ISDIR(inode->i_mode))
+ return -EINVAL;
+
+ if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
+ !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+ return -EINVAL;
+
+ /*
+ * It is only valid to set the DAX flag on regular files and
+ * directories on filesystems.
+ */
+ if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
+ !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
+ return -EINVAL;
+
+ /* Extent size hints of zero turn off the flags. */
+ if (fa->fsx_extsize == 0)
+ fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
+ if (fa->fsx_cowextsize == 0)
+ fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
+
+ return 0;
+}
+
+/**
+ * vfs_fileattr_set - change miscellaneous file attributes
+ * @idmap: idmap of the mount
+ * @dentry: the object to change
+ * @fa: fileattr pointer
+ *
+ * After verifying permissions, call i_op->fileattr_set() callback, if
+ * exists.
+ *
+ * Verifying attributes involves retrieving current attributes with
+ * i_op->fileattr_get(), this also allows initializing attributes that have
+ * not been set by the caller to current values. Inode lock is held
+ * thoughout to prevent racing with another instance.
+ *
+ * Return: 0 on success, or a negative error on failure.
+ */
+int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
+ struct fileattr *fa)
+{
+ struct inode *inode = d_inode(dentry);
+ struct fileattr old_ma = {};
+ int err;
+
+ if (!inode->i_op->fileattr_set)
+ return -ENOIOCTLCMD;
+
+ if (!inode_owner_or_capable(idmap, inode))
+ return -EPERM;
+
+ inode_lock(inode);
+ err = vfs_fileattr_get(dentry, &old_ma);
+ if (!err) {
+ /* initialize missing bits from old_ma */
+ if (fa->flags_valid) {
+ fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
+ fa->fsx_extsize = old_ma.fsx_extsize;
+ fa->fsx_nextents = old_ma.fsx_nextents;
+ fa->fsx_projid = old_ma.fsx_projid;
+ fa->fsx_cowextsize = old_ma.fsx_cowextsize;
+ } else {
+ fa->flags |= old_ma.flags & ~FS_COMMON_FL;
+ }
+ err = fileattr_set_prepare(inode, &old_ma, fa);
+ if (!err)
+ err = inode->i_op->fileattr_set(idmap, dentry, fa);
+ }
+ inode_unlock(inode);
+
+ return err;
+}
+EXPORT_SYMBOL(vfs_fileattr_set);
+
+int ioctl_getflags(struct file *file, unsigned int __user *argp)
+{
+ struct fileattr fa = { .flags_valid = true }; /* hint only */
+ int err;
+
+ err = vfs_fileattr_get(file->f_path.dentry, &fa);
+ if (!err)
+ err = put_user(fa.flags, argp);
+ return err;
+}
+EXPORT_SYMBOL(ioctl_getflags);
+
+int ioctl_setflags(struct file *file, unsigned int __user *argp)
+{
+ struct mnt_idmap *idmap = file_mnt_idmap(file);
+ struct dentry *dentry = file->f_path.dentry;
+ struct fileattr fa;
+ unsigned int flags;
+ int err;
+
+ err = get_user(flags, argp);
+ if (!err) {
+ err = mnt_want_write_file(file);
+ if (!err) {
+ fileattr_fill_flags(&fa, flags);
+ err = vfs_fileattr_set(idmap, dentry, &fa);
+ mnt_drop_write_file(file);
+ }
+ }
+ return err;
+}
+EXPORT_SYMBOL(ioctl_setflags);
+
+int ioctl_fsgetxattr(struct file *file, void __user *argp)
+{
+ struct fileattr fa = { .fsx_valid = true }; /* hint only */
+ int err;
+
+ err = vfs_fileattr_get(file->f_path.dentry, &fa);
+ if (!err)
+ err = copy_fsxattr_to_user(&fa, argp);
+
+ return err;
+}
+EXPORT_SYMBOL(ioctl_fsgetxattr);
+
+int ioctl_fssetxattr(struct file *file, void __user *argp)
+{
+ struct mnt_idmap *idmap = file_mnt_idmap(file);
+ struct dentry *dentry = file->f_path.dentry;
+ struct fileattr fa;
+ int err;
+
+ err = copy_fsxattr_from_user(&fa, argp);
+ if (!err) {
+ err = mnt_want_write_file(file);
+ if (!err) {
+ err = vfs_fileattr_set(idmap, dentry, &fa);
+ mnt_drop_write_file(file);
+ }
+ }
+ return err;
+}
+EXPORT_SYMBOL(ioctl_fssetxattr);
diff --git a/fs/ioctl.c b/fs/ioctl.c
index c91fd2b46a77..5bf1e4215252 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -453,315 +453,6 @@ static int ioctl_file_dedupe_range(struct file *file,
return ret;
}
-/**
- * fileattr_fill_xflags - initialize fileattr with xflags
- * @fa: fileattr pointer
- * @xflags: FS_XFLAG_* flags
- *
- * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
- * other fields are zeroed.
- */
-void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
-{
- memset(fa, 0, sizeof(*fa));
- fa->fsx_valid = true;
- fa->fsx_xflags = xflags;
- if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
- fa->flags |= FS_IMMUTABLE_FL;
- if (fa->fsx_xflags & FS_XFLAG_APPEND)
- fa->flags |= FS_APPEND_FL;
- if (fa->fsx_xflags & FS_XFLAG_SYNC)
- fa->flags |= FS_SYNC_FL;
- if (fa->fsx_xflags & FS_XFLAG_NOATIME)
- fa->flags |= FS_NOATIME_FL;
- if (fa->fsx_xflags & FS_XFLAG_NODUMP)
- fa->flags |= FS_NODUMP_FL;
- if (fa->fsx_xflags & FS_XFLAG_DAX)
- fa->flags |= FS_DAX_FL;
- if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
- fa->flags |= FS_PROJINHERIT_FL;
-}
-EXPORT_SYMBOL(fileattr_fill_xflags);
-
-/**
- * fileattr_fill_flags - initialize fileattr with flags
- * @fa: fileattr pointer
- * @flags: FS_*_FL flags
- *
- * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
- * All other fields are zeroed.
- */
-void fileattr_fill_flags(struct fileattr *fa, u32 flags)
-{
- memset(fa, 0, sizeof(*fa));
- fa->flags_valid = true;
- fa->flags = flags;
- if (fa->flags & FS_SYNC_FL)
- fa->fsx_xflags |= FS_XFLAG_SYNC;
- if (fa->flags & FS_IMMUTABLE_FL)
- fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
- if (fa->flags & FS_APPEND_FL)
- fa->fsx_xflags |= FS_XFLAG_APPEND;
- if (fa->flags & FS_NODUMP_FL)
- fa->fsx_xflags |= FS_XFLAG_NODUMP;
- if (fa->flags & FS_NOATIME_FL)
- fa->fsx_xflags |= FS_XFLAG_NOATIME;
- if (fa->flags & FS_DAX_FL)
- fa->fsx_xflags |= FS_XFLAG_DAX;
- if (fa->flags & FS_PROJINHERIT_FL)
- fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
-}
-EXPORT_SYMBOL(fileattr_fill_flags);
-
-/**
- * vfs_fileattr_get - retrieve miscellaneous file attributes
- * @dentry: the object to retrieve from
- * @fa: fileattr pointer
- *
- * Call i_op->fileattr_get() callback, if exists.
- *
- * Return: 0 on success, or a negative error on failure.
- */
-int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
-{
- struct inode *inode = d_inode(dentry);
-
- if (!inode->i_op->fileattr_get)
- return -ENOIOCTLCMD;
-
- return inode->i_op->fileattr_get(dentry, fa);
-}
-EXPORT_SYMBOL(vfs_fileattr_get);
-
-/**
- * copy_fsxattr_to_user - copy fsxattr to userspace.
- * @fa: fileattr pointer
- * @ufa: fsxattr user pointer
- *
- * Return: 0 on success, or -EFAULT on failure.
- */
-int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
-{
- struct fsxattr xfa;
-
- memset(&xfa, 0, sizeof(xfa));
- xfa.fsx_xflags = fa->fsx_xflags;
- xfa.fsx_extsize = fa->fsx_extsize;
- xfa.fsx_nextents = fa->fsx_nextents;
- xfa.fsx_projid = fa->fsx_projid;
- xfa.fsx_cowextsize = fa->fsx_cowextsize;
-
- if (copy_to_user(ufa, &xfa, sizeof(xfa)))
- return -EFAULT;
-
- return 0;
-}
-EXPORT_SYMBOL(copy_fsxattr_to_user);
-
-static int copy_fsxattr_from_user(struct fileattr *fa,
- struct fsxattr __user *ufa)
-{
- struct fsxattr xfa;
-
- if (copy_from_user(&xfa, ufa, sizeof(xfa)))
- return -EFAULT;
-
- fileattr_fill_xflags(fa, xfa.fsx_xflags);
- fa->fsx_extsize = xfa.fsx_extsize;
- fa->fsx_nextents = xfa.fsx_nextents;
- fa->fsx_projid = xfa.fsx_projid;
- fa->fsx_cowextsize = xfa.fsx_cowextsize;
-
- return 0;
-}
-
-/*
- * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
- * any invalid configurations.
- *
- * Note: must be called with inode lock held.
- */
-static int fileattr_set_prepare(struct inode *inode,
- const struct fileattr *old_ma,
- struct fileattr *fa)
-{
- int err;
-
- /*
- * The IMMUTABLE and APPEND_ONLY flags can only be changed by
- * the relevant capability.
- */
- if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
- !capable(CAP_LINUX_IMMUTABLE))
- return -EPERM;
-
- err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
- if (err)
- return err;
-
- /*
- * Project Quota ID state is only allowed to change from within the init
- * namespace. Enforce that restriction only if we are trying to change
- * the quota ID state. Everything else is allowed in user namespaces.
- */
- if (current_user_ns() != &init_user_ns) {
- if (old_ma->fsx_projid != fa->fsx_projid)
- return -EINVAL;
- if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
- FS_XFLAG_PROJINHERIT)
- return -EINVAL;
- } else {
- /*
- * Caller is allowed to change the project ID. If it is being
- * changed, make sure that the new value is valid.
- */
- if (old_ma->fsx_projid != fa->fsx_projid &&
- !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
- return -EINVAL;
- }
-
- /* Check extent size hints. */
- if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
- return -EINVAL;
-
- if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
- !S_ISDIR(inode->i_mode))
- return -EINVAL;
-
- if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
- !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
- return -EINVAL;
-
- /*
- * It is only valid to set the DAX flag on regular files and
- * directories on filesystems.
- */
- if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
- !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
- return -EINVAL;
-
- /* Extent size hints of zero turn off the flags. */
- if (fa->fsx_extsize == 0)
- fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
- if (fa->fsx_cowextsize == 0)
- fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
-
- return 0;
-}
-
-/**
- * vfs_fileattr_set - change miscellaneous file attributes
- * @idmap: idmap of the mount
- * @dentry: the object to change
- * @fa: fileattr pointer
- *
- * After verifying permissions, call i_op->fileattr_set() callback, if
- * exists.
- *
- * Verifying attributes involves retrieving current attributes with
- * i_op->fileattr_get(), this also allows initializing attributes that have
- * not been set by the caller to current values. Inode lock is held
- * thoughout to prevent racing with another instance.
- *
- * Return: 0 on success, or a negative error on failure.
- */
-int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
- struct fileattr *fa)
-{
- struct inode *inode = d_inode(dentry);
- struct fileattr old_ma = {};
- int err;
-
- if (!inode->i_op->fileattr_set)
- return -ENOIOCTLCMD;
-
- if (!inode_owner_or_capable(idmap, inode))
- return -EPERM;
-
- inode_lock(inode);
- err = vfs_fileattr_get(dentry, &old_ma);
- if (!err) {
- /* initialize missing bits from old_ma */
- if (fa->flags_valid) {
- fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
- fa->fsx_extsize = old_ma.fsx_extsize;
- fa->fsx_nextents = old_ma.fsx_nextents;
- fa->fsx_projid = old_ma.fsx_projid;
- fa->fsx_cowextsize = old_ma.fsx_cowextsize;
- } else {
- fa->flags |= old_ma.flags & ~FS_COMMON_FL;
- }
- err = fileattr_set_prepare(inode, &old_ma, fa);
- if (!err)
- err = inode->i_op->fileattr_set(idmap, dentry, fa);
- }
- inode_unlock(inode);
-
- return err;
-}
-EXPORT_SYMBOL(vfs_fileattr_set);
-
-static int ioctl_getflags(struct file *file, unsigned int __user *argp)
-{
- struct fileattr fa = { .flags_valid = true }; /* hint only */
- int err;
-
- err = vfs_fileattr_get(file->f_path.dentry, &fa);
- if (!err)
- err = put_user(fa.flags, argp);
- return err;
-}
-
-static int ioctl_setflags(struct file *file, unsigned int __user *argp)
-{
- struct mnt_idmap *idmap = file_mnt_idmap(file);
- struct dentry *dentry = file->f_path.dentry;
- struct fileattr fa;
- unsigned int flags;
- int err;
-
- err = get_user(flags, argp);
- if (!err) {
- err = mnt_want_write_file(file);
- if (!err) {
- fileattr_fill_flags(&fa, flags);
- err = vfs_fileattr_set(idmap, dentry, &fa);
- mnt_drop_write_file(file);
- }
- }
- return err;
-}
-
-static int ioctl_fsgetxattr(struct file *file, void __user *argp)
-{
- struct fileattr fa = { .fsx_valid = true }; /* hint only */
- int err;
-
- err = vfs_fileattr_get(file->f_path.dentry, &fa);
- if (!err)
- err = copy_fsxattr_to_user(&fa, argp);
-
- return err;
-}
-
-static int ioctl_fssetxattr(struct file *file, void __user *argp)
-{
- struct mnt_idmap *idmap = file_mnt_idmap(file);
- struct dentry *dentry = file->f_path.dentry;
- struct fileattr fa;
- int err;
-
- err = copy_fsxattr_from_user(&fa, argp);
- if (!err) {
- err = mnt_want_write_file(file);
- if (!err) {
- err = vfs_fileattr_set(idmap, dentry, &fa);
- mnt_drop_write_file(file);
- }
- }
- return err;
-}
-
static int ioctl_getfsuuid(struct file *file, void __user *argp)
{
struct super_block *sb = file_inode(file)->i_sb;
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0..6030d0bf7ad3 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -55,5 +55,9 @@ static inline bool fileattr_has_fsx(const struct fileattr *fa)
int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
struct fileattr *fa);
+int ioctl_getflags(struct file *file, unsigned int __user *argp);
+int ioctl_setflags(struct file *file, unsigned int __user *argp);
+int ioctl_fsgetxattr(struct file *file, void __user *argp);
+int ioctl_fssetxattr(struct file *file, void __user *argp);
#endif /* _LINUX_FILEATTR_H */
--
2.47.2
^ permalink raw reply related
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: Mickaël Salaün @ 2025-05-12 10:20 UTC (permalink / raw)
To: John Johansen
Cc: Song Liu, Maxime Bélair, linux-security-module, paul,
jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
penguin-kernel, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <19313f6b-42d7-4845-9a4b-93c7546aadb9@canonical.com>
On Sun, May 11, 2025 at 03:47:21AM -0700, John Johansen wrote:
> On 5/9/25 03:26, Mickaël Salaün wrote:
> > On Thu, May 08, 2025 at 01:18:20AM -0700, John Johansen wrote:
> > > On 5/7/25 23:06, Song Liu wrote:
> > > > On Wed, May 7, 2025 at 8:37 AM Maxime Bélair
> > > > <maxime.belair@canonical.com> wrote:
> > > > [...]
> > > > permission check to each pseudo file. The downside of the syscall, however,
> > > > is that all the permission checks are hard-coded in the kernel (except for
> > >
> > > The permission checks don't have to be hard coded. Each LSM can define how it handles
> > > or manages the syscall. The default is that it isn't supported, but if an lsm decides
> > > to support it, there is now reason that its policy can't determine the use of the
> > > syscall.
> >
> > From an interface design point of view, it would be better to clearly
> > specify the scope of a command (e.g. which components could be impacted
> > by a command), and make sure the documentation reflect that as well.
> > Even better, have a syscalls per required privileges and impact (e.g.
> > privileged or unprivileged). Going this road, I'm not sure if a
> > privileged syscall would make sense given the existing filesystem
> > interface.
> >
>
> uhhhmmm, not just privileged. As you well know we are looking to use
> this for unprivileged policy. The LSM can limit to privileged if it
> wants but it doesn't have to limit it to privileged policy.
Yes, I meant to say having a syscall for unprivileged actions, and maybe
another one for privileged ones, but this might be a hard sell. :)
To say it another way, for your use case, do you need this syscall(s)
for privileged operations? Do you plan to drop (or stop extending) the
filesystem interface or do you think it would be good for (AppArmor)
privileged operations too? I know syscalls might be attractive and
could be used for everything, but it's good to have a well-defined plan
and semantic to avoid using such syscall as another multiplexer with
unrelated operations and required privileges.
If this syscall should also be a way to do privileged operations, should
we also agree on a common set of permissions (e.g. global CAP_MAC_ADMIN
or user namespace one)?
[...]
> > > > Overall, I am still not convinced a syscall for all LSMs is needed. To
> > > > justify such
> > >
> > > its not needed by all LSMs, just a subset of them, and some nebulous
> > > subset of potentially future LSMs that is entirely undefinable.
> > >
> > > If we had had appropriate LSM syscalls landlock wouldn't have needed
> > > to have landlock specific syscalls. Having another LSM go that route
> > > feels wrong especially now that we have some LSM syscalls.
> >
> > I don't agree. Dedicated syscalls are a good thing. See my other
> > reply.
> >
>
> I think we can just disagree on this point.
>
> > > If a
> > > syscall is needed by an LSM its better to try hashing something out
> > > that might have utility for multiple LSMs or at the very least,
> > > potentially have utility in the future.
> > >
> > >
> > > > a syscall, I think we need to show that it is useful in multiple LSMs.
> > > > Also, if we
> > > > really want to have single set of APIs for all LSMs, we may also need
> > > > get_policy,
> > >
> > > We are never going to get a single set of APIs for all LSMs. I will
> > > settle for an api that has utility for a subset
> > >
> > > > remove_policy, etc. This set as-is appears to be an incomplete design. The
> > >
> > > To have a complete design, there needs to be feedback and discussion
> > > from multiple LSMs. This is a starting point.
> > >
> > > > implementation, with call_int_hook, is also problematic. It can easily
> > > > cause some> controversial behaviors.
> > > >
> > > agreed it shouldn't be doing a straight call_int_hook, it should only
> > > call it against the lsm identified by the lsmid
> >
> > Yes, but then, I don't see the point of a "generic" LSM syscall.
>
> its not a generic LSM syscall. Its a syscall or maybe a set of syscalls
> for a specific scoped problem of loading/managing policy.
>
> Can we come to something acceptable? I don't know but we are going to
> look at it before trying for an apparmor specific syscall.
I understand and it's good to have this discussion.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-11 11:26 UTC (permalink / raw)
To: Casey Schaufler, Mickaël Salaün
Cc: Paul Moore, Maxime Bélair, Tetsuo Handa,
linux-security-module, jmorris, serge, kees, stephen.smalley.work,
takedakn, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <71c3c2d6-5569-4580-89a4-513a03a429ab@schaufler-ca.com>
On 5/9/25 07:21, Casey Schaufler wrote:
> On 5/9/2025 3:26 AM, Mickaël Salaün wrote:
>> On Thu, May 08, 2025 at 09:54:19AM -0700, Casey Schaufler wrote:
>>> On 5/8/2025 1:29 AM, John Johansen wrote:
>>>> On 5/7/25 13:25, Paul Moore wrote:
>>>>> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
>>>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>>> On 2025/05/06 23:32, Maxime Bélair wrote:
>>>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>>>>> index dcaad8818679..b39e6635a7d5 100644
>>>>>>> --- a/security/lsm_syscalls.c
>>>>>>> +++ b/security/lsm_syscalls.c
>>>>>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user
>>>>>>> *, ids, u32 __user *, size,
>>>>>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void
>>>>>>> __user *, buf, u32
>>>>>>> __user *, size, u32, flags)
>>>>>>> {
>>>>>>> - return 0;
>>>>>>> + size_t usize;
>>>>>>> +
>>>>>>> + if (get_user(usize, size))
>>>>>>> + return -EFAULT;
>>>>>>> +
>>>>>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize,
>>>>>>> flags);
>>>>>>> }
>>>>>> syzbot will report user-controlled unbounded huge size memory
>>>>>> allocation attempt. ;-)
>>>>>>
>>>>>> This interface might be fine for AppArmor, but TOMOYO won't use this
>>>>>> interface because
>>>>>> TOMOYO's policy is line-oriented ASCII text data where the
>>>>>> destination is switched via
>>>>>> pseudo‑filesystem's filename ...
>>>>> While Tetsuo's comment is limited to TOMOYO, I believe the argument
>>>>> applies to a number of other LSMs as well. The reality is that there
>>>>> is no one policy ideal shared across LSMs and that complicates things
>>>>> like the lsm_manage_policy() proposal. I'm intentionally saying
>>>>> "complicates" and not "prevents" because I don't want to flat out
>>>>> reject something like this, but I think there needs to be a larger
>>>>> discussion among the different LSM groups about what such an API
>>>>> should look like. We may not need to get every LSM to support this
>>>>> new API, but we need to get something that would work for a
>>>>> significant majority and would be general/extensible enough that we
>>>>> would expect it to work with the majority of future LSMs (as much as
>>>>> we can predict the future anyway).
>>>>>
>>>> yep, I look at this is just a starting point for discussion. There
>>>> isn't going to be any discussion without some code, so here is a v1
>>>> that supports a single LSM let the bike shedding begin.
>>> Aside from the issues with allocating a buffer for a big policy
>>> I don't see a problem with this proposal. The system call looks
>>> a lot like the other LSM interfaces, so any developer who likes
>>> those ought to like this one. The infrastructure can easily check
>>> the lsm_id and only call the appropriate LSM hook, so no one
>>> is going to be interfering with other modules.
>> We may not want to only be able to load buffers containing policies, but
>> also to leverage file descriptors like Landlock does. Getting a
>> property from a kernel object or updating it is mainly about dealing
>> with a buffer. And the current LSM syscalls do just that. Other kind
>> of operations may require more than that though.
>>
>> I don't like multiplexer syscalls because they don't expose a clear
>> semantic and can be complex to manage and filter. This new syscall is
>> kind of a multiplexer that redirect commands to an arbitrary set of
>> kernel parts, which can then define their own semantic. I'd like to see
>> a clear set of well-defined operations and their required permission.
>> Even better, one syscall per operation should simplify their interface.
>
> The development and maintenance of system calls is expensive in both
> time and effort. LSM specific system calls frighten me. When I was
> young adding system calls was just not done. A system call would
> never be allowed for a specific sub-system or optional feature. True,
> there are issues with the LSM specific filesystem approach. But I
> like it, as it allows the LSM more freedom in its interfaces and
> won't clutter the API if the LSM goes away or quits using it.
>
I get the reticence on adding syscalls. Indeed its part of why I
want to explore LSM syscalls before going with an apparmor specific
syscall.
The current LSM specific fs approach has limitations that just can't
be reasonably worked around for some use cases, so that leaves going
with an alternate mechanism. For this use case, ioctls are problematic
like the fs. prctl could work for a subset and abused for the whole,
but a syscall feels cleaner.
I am open to other options.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-11 11:20 UTC (permalink / raw)
To: Mickaël Salaün, Casey Schaufler
Cc: Paul Moore, Maxime Bélair, Tetsuo Handa,
linux-security-module, jmorris, serge, kees, stephen.smalley.work,
takedakn, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <20250509.Chuecae0phoo@digikod.net>
On 5/9/25 03:26, Mickaël Salaün wrote:
> On Thu, May 08, 2025 at 09:54:19AM -0700, Casey Schaufler wrote:
>> On 5/8/2025 1:29 AM, John Johansen wrote:
>>> On 5/7/25 13:25, Paul Moore wrote:
>>>> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
>>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>> On 2025/05/06 23:32, Maxime Bélair wrote:
>>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>>>> index dcaad8818679..b39e6635a7d5 100644
>>>>>> --- a/security/lsm_syscalls.c
>>>>>> +++ b/security/lsm_syscalls.c
>>>>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user
>>>>>> *, ids, u32 __user *, size,
>>>>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void
>>>>>> __user *, buf, u32
>>>>>> __user *, size, u32, flags)
>>>>>> {
>>>>>> - return 0;
>>>>>> + size_t usize;
>>>>>> +
>>>>>> + if (get_user(usize, size))
>>>>>> + return -EFAULT;
>>>>>> +
>>>>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize,
>>>>>> flags);
>>>>>> }
>>>>>
>>>>> syzbot will report user-controlled unbounded huge size memory
>>>>> allocation attempt. ;-)
>>>>>
>>>>> This interface might be fine for AppArmor, but TOMOYO won't use this
>>>>> interface because
>>>>> TOMOYO's policy is line-oriented ASCII text data where the
>>>>> destination is switched via
>>>>> pseudo‑filesystem's filename ...
>>>>
>>>> While Tetsuo's comment is limited to TOMOYO, I believe the argument
>>>> applies to a number of other LSMs as well. The reality is that there
>>>> is no one policy ideal shared across LSMs and that complicates things
>>>> like the lsm_manage_policy() proposal. I'm intentionally saying
>>>> "complicates" and not "prevents" because I don't want to flat out
>>>> reject something like this, but I think there needs to be a larger
>>>> discussion among the different LSM groups about what such an API
>>>> should look like. We may not need to get every LSM to support this
>>>> new API, but we need to get something that would work for a
>>>> significant majority and would be general/extensible enough that we
>>>> would expect it to work with the majority of future LSMs (as much as
>>>> we can predict the future anyway).
>>>>
>>>
>>> yep, I look at this is just a starting point for discussion. There
>>> isn't going to be any discussion without some code, so here is a v1
>>> that supports a single LSM let the bike shedding begin.
>>
>> Aside from the issues with allocating a buffer for a big policy
>> I don't see a problem with this proposal. The system call looks
>> a lot like the other LSM interfaces, so any developer who likes
>> those ought to like this one. The infrastructure can easily check
>> the lsm_id and only call the appropriate LSM hook, so no one
>> is going to be interfering with other modules.
>
> We may not want to only be able to load buffers containing policies, but
> also to leverage file descriptors like Landlock does. Getting a
I am not opposed to a syscall that leverages file desriptors like landlock
but that would be a different syscall with different semantics, and
something for an lsm that wants that semantic to introduce.
> property from a kernel object or updating it is mainly about dealing
> with a buffer. And the current LSM syscalls do just that. Other kind
> of operations may require more than that though.
>
sure but they don't do it for the semantic of loading/managing policy.
> I don't like multiplexer syscalls because they don't expose a clear
> semantic and can be complex to manage and filter. This new syscall is
> kind of a multiplexer that redirect commands to an arbitrary set of
> kernel parts, which can then define their own semantic. I'd like to see
> a clear set of well-defined operations and their required permission.
> Even better, one syscall per operation should simplify their interface.
I am not opposed to that approach. This can be multiple syscalls. Its
a v1 to try and see if we can come to any agreement on a set of semantics
^ permalink raw reply
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: John Johansen @ 2025-05-11 11:09 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tetsuo Handa, Maxime Bélair, Song Liu, linux-security-module,
paul, jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <20250509.bokeiCho2oov@digikod.net>
On 5/9/25 03:25, Mickaël Salaün wrote:
> On Thu, May 08, 2025 at 12:52:55AM -0700, John Johansen wrote:
>> On 5/7/25 15:04, Tetsuo Handa wrote:
>>> On 2025/05/08 0:37, Maxime Bélair wrote:
>>>> Again, each module decides which operations to expose through this syscall. In many cases
>>>> the operation will still require CAP_SYS_ADMIN or a similar capability, so environments
>>>> that choose this interface remain secure while gaining its advantages.
>>>
>>> If the interpretation of "flags" argument varies across LSMs, it sounds like ioctl()'s
>>
>> yes that does feel like ioctls(), on the other hand defining them at the LSM level won't
>> offer LSMs flexibility making it so the syscall covers fewer use cases. I am not opposed
>> to either, it just hashing out what people want, and what is acceptable.
>>
>>> "cmd" argument. Also, there is prctl() which can already carry string-ish parameters
>>> without involving open(). Why can't we use prctl() instead of lsm_manage_policy() ?
>>>
>>
>> prctl() can be used, I used it for the unprivileged policy demo. It has its own set of
>> problems. While LSM policy could be associated with the process doing the load/replacement
>> or what ever operation, it isn't necessarily tied to it. A lot of LSM policy is not
>> process specific making prctl() a poor fit.
>>
>> prctl() requires allocating a global prctl()
>>
>> prctl() are already being filtered/controlled by LSMs making them a poort fit for
>> use by an LSM in a stacking situation as it requires updating the policy of other
>> LSMs on the system. Yes seccomp can filter the syscall but that still is an easier
>> barrier to overcome than having to have instruction for how to allow your LSMs
>> prctl() in multiple LSMs.
>>
>>
>> Mickaël already argued the need for landlock to have syscalls. See
>
> Landlock indeed requires syscalls mainly because of its unprivileged
> nature.
>
yes that is the dominant reason
>> https://lore.kernel.org/lkml/20200511192156.1618284-7-mic@digikod.net/
>> and the numerous iterations before that.
>
> This link might be misleading though, it points to an initial version of
> the syscall proposal (v17) and it was then decided to create one syscall
> per operation (v34), which is why we ended with 3 syscalls. See the
> changelog:
> https://lore.kernel.org/r/20210422154123.13086-9-mic@digikod.net
>
yes and no. I am well aware landlock's syscall got split into three syscalls.
All I was trying to do is reference to the start of the discussion on why
landlock needed a syscall(s). I thought the details of why you have three
etc, really didn't add to the discussion. But yeah not also pointing to
v34 could be considered misleading.
>>
>> Ideally those could have been LSM syscalls, with landlock leveraging them.
>
> I don't agree. The Landlock syscalls have a well-defined semantic, with
First I don't begrudge Landlock its syscalls, I think at the time it was
the only way forward.
> documented security requirements, and they deal with specific kernel
> objects identified with file descriptors, including a dedicated one:
> [landlock-ruleset].
I am aware. Those semantics could have been kept and documented, within
a set of LSM syscalls. Yes landlock's syscalls shouldn't have been done
behind a single LSM syscall, I am not advocating for that but maybe
behind several LSM syscalls.
> For the features provided by these Landlock
> syscalls, it would not have been a good idea to reuse existing syscalls,
> nor to rely on the syscall proposed in this series because the interface
> is too specific to some of the current privileged LSMs (i.e. ingest a
> policy blob). Making this interface more generic would lead to even
> less defined semantic though.
Right, so again not a generic LSM syscall. But "generic" LSM syscalls
for certain purposes. Let me walk my statement back a little, what I
find unfortunate was that the landlock LSM syscalls didn't get discussed
as a set of generic LSM syscall's with landlock being the first to
implement them.
The question is hashing out where the generic semantics are vs. the
individual LSMs. Having an LSM syscall to deal with specific kernel
objects idenetified with file descriptors, and allowing each LSMs
to deal with that if it needs is possible.
Its a matter of figuring something out. It could be it turns out it is
not worth it. And some individual LSM syscalls like landlocks are the
way to go, its that it wasn't explored. I don't fault you, and think
it really wasn't even an option at the time.
>
>> AppArmor
>> is getting to where it has similar needs to landlock. Yes we can use ioctls, prctls,
>> netlink, the fs, etc. it doesn't mean that those are the best interfaces to do so,
>
> I think it would make sense to propose AppArmor-specific syscalls.
>
that may be the case, but I think we should explore providing a more
LSM generic interface first.
>> and ideally any interface we use will be of benefit to some other LSMs in the future.
>
> The LSM syscalls may make sense to deal with LSM blobs managed by the
> LSM framework (e.g. get/set properties) when the operations are
> common/generic.
>
> Security policies are specific to each LSM and they should implement
> their own well-defined interface (e.g. filesystem, netlink, syscall).
>
policies at some level are just blobs too. It is worth at least
exploring whether there can be a common interface.
> The LSM framework doesn't provide nor manage any security policy, it
> mainly provides a set of consistent and well-defined kernel hooks with
> security blobs to enforce a security policy. I don't think it makes
> sense to add LSM syscalls to manage things not managed by the LSM
> framework.
we aren't talking about the LSM framework managing security policy,
just whether it makes sense for it to provide a common interface that
an LSM can choose to use to provide it a blob of policy that it
can then manage.
Its just a mechanism. This isn't all that different than using the
filesystem, netlink, or other mechanisms to shuttle the blob
between userspace to the kernel, and then the LSM manages its
policy and data.
The big difference is that using the syscall opens unprivileged
policy up to the LSM more broadly. If we are going to go the syscall
route for apparmor, we might as well see if we can't make that
mechanism more broadly available, and make it easier for other
LSMs in the future.
Again, it might turn out its a fools errand, and we have to do
an apparmor specific syscall, but it is worth exploring first.
^ permalink raw reply
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: John Johansen @ 2025-05-11 10:47 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Song Liu, Maxime Bélair, linux-security-module, paul,
jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
penguin-kernel, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <20250509.ePu7gaim1Foo@digikod.net>
On 5/9/25 03:26, Mickaël Salaün wrote:
> On Thu, May 08, 2025 at 01:18:20AM -0700, John Johansen wrote:
>> On 5/7/25 23:06, Song Liu wrote:
>>> On Wed, May 7, 2025 at 8:37 AM Maxime Bélair
>>> <maxime.belair@canonical.com> wrote:
>>> [...]
>>>>>
>>>>> These two do not feel like real benefits:
>>>>> - One syscall cannot fit all use cases well...
>>>>
>>>> This syscall is not intended to cover every case, nor to replace existing kernel
>>>> interfaces.
>>>>
>>>> Each LSM can decide which operations it wants to support (if any). For example, when
>>>> loading policies, an LSM may choose to allow only policies that further restrict
>>>> privileges.
>>>>
>>>>> - Not working in containers is often not an issue, but a feature.
>>>>
>>>> Indeed, using this syscall requires appropriate capabilities and will not permit
>>>> unprivileged containers to manage policies arbitrarily.
>>>>
>>>> With this syscall, capability checks remain the responsibility of each LSM.
>>>>
>>>> For instance, in the AppArmor patch, a profile can be loaded only if
>>>> aa_policy_admin_capable() succeeds (which requires CAP_MAC_ADMIN). Moreover, by design,
>>>> policies can be loaded only in the current namespace.
>>>>
>>>> I see this syscall as a middle point between exposing the entire sysfs, creating a large
>>>> attack surface, and blocking everything.
>>>>
>>>> Landlock’s existing syscalls already improve security by allowing processes to further
>>>> restrict their ambient rights while adding only a modest attack surface.
>>>>
>>>> This syscall is a further step in that direction: it lets LSMs add restrictive policies
>>>> without requiring exposing every other interface.
>>>
>>> I don't think a syscall makes the API more secure. If necessary, we can add
>>
>> It exposes a different attack surface. Requiring mounting of the fs to where it is visible
>> in the container, provides attack surface, and requires additional external configuration.
>
> We should also keep in mind that syscalls could be accessible from
> everywhere, by everyone, which may increase the attack surface compared
> to a privileged filesystem interface. Adding a second interface may
> also introduce issues. Anyway, I'm definitely not against syscalls, but
> I don't see why the filesystem interface would be "less secure" in this
> context.
>
yes syscalls being accessible from everywhere is another form of attack
surface, that needs to be mediated.
the fs can be mediated, its expose is a multiple lsms with multiple
different interfaces on the files within it. What really is more
problematic is makng the fs available in the container. Yes a
container manager can do it but then you are dependent on the
container manager making your interface available.
Other wise you are looking at making mount available to your app
within the container.
>>
>> Then there is the whole issue of getting the various LSMs to allow another LSM in the
>> stack to be able manage its own policy.
>
> Right, and it's a similar issue with seccomp policies wrt syscalls.
>
yes, though seccomp I have found to be the easier one to deal with
>>
>>> permission check to each pseudo file. The downside of the syscall, however,
>>> is that all the permission checks are hard-coded in the kernel (except for
>>
>> The permission checks don't have to be hard coded. Each LSM can define how it handles
>> or manages the syscall. The default is that it isn't supported, but if an lsm decides
>> to support it, there is now reason that its policy can't determine the use of the
>> syscall.
>
> From an interface design point of view, it would be better to clearly
> specify the scope of a command (e.g. which components could be impacted
> by a command), and make sure the documentation reflect that as well.
> Even better, have a syscalls per required privileges and impact (e.g.
> privileged or unprivileged). Going this road, I'm not sure if a
> privileged syscall would make sense given the existing filesystem
> interface.
>
uhhhmmm, not just privileged. As you well know we are looking to use
this for unprivileged policy. The LSM can limit to privileged if it
wants but it doesn't have to limit it to privileged policy.
>>
>>> BPF LSM); while the sys admin can configure permissions of the pseudo
>>> files in user space.
>>>
>> Other LSMs also have policy that can control access to pseudo filesystems and
>> other resources. Again, the control doesn't have to be hard coded. And seccomp can
>> be used to block the syscall.
>>
>>
>>
>>>> Again, each module decides which operations to expose through this syscall. In many cases
>>>> the operation will still require CAP_SYS_ADMIN or a similar capability, so environments
>>>> that choose this interface remain secure while gaining its advantages.
>>>>
>>>>>> - Avoids overhead of other kernel interfaces for better efficiency
>>>>>
>>>>> .. and it is is probably less efficient, because everything need to
>>>>> fit in the same API.
>>>>
>>>> As shown below, the syscall can significantly improve the performance of policy management.
>>>> A more detailed benchmark is available in [1].
>>>>
>>>> The following table presents the time required to load an AppArmor profile.
>>>>
>>>> For every cell, the first value is the total time taken by aa-load, and the value in
>>>> parentheses is the time spent to load the policy in the kernel only (total - dry‑run).
>>>>
>>>> Results are in microseconds and are averaged over 10 000 runs to reduce variance.
>>>>
>>>>
>>>> | t (µs) | syscall | pseudofs | Speedup |
>>>> |-----------|-------------|-------------|---------------|
>>>> | 1password | 4257 (1127) | 3333 (192) | x1.28 (x5.86) |
>>>> | Xorg | 6099 (2961) | 5167 (2020) | x1.18 (x1.47) |
>>>>
>>>
>>> I am not sure the performance of loading security policies is on any
>>> critical path.
>>
>> generally speaking I agree, but I am also not going to turn down a
>> performance improvement either. Its a nice to have, but not a strong
>> argument for need.
>>
>>> The implementation calls the hook for each LSM, which is why I think the
>>> syscall is not efficient.
>>>
>> it should only call the LSM identified by the lsmid in the call.
>>
>>> Overall, I am still not convinced a syscall for all LSMs is needed. To
>>> justify such
>>
>> its not needed by all LSMs, just a subset of them, and some nebulous
>> subset of potentially future LSMs that is entirely undefinable.
>>
>> If we had had appropriate LSM syscalls landlock wouldn't have needed
>> to have landlock specific syscalls. Having another LSM go that route
>> feels wrong especially now that we have some LSM syscalls.
>
> I don't agree. Dedicated syscalls are a good thing. See my other
> reply.
>
I think we can just disagree on this point.
>> If a
>> syscall is needed by an LSM its better to try hashing something out
>> that might have utility for multiple LSMs or at the very least,
>> potentially have utility in the future.
>>
>>
>>> a syscall, I think we need to show that it is useful in multiple LSMs.
>>> Also, if we
>>> really want to have single set of APIs for all LSMs, we may also need
>>> get_policy,
>>
>> We are never going to get a single set of APIs for all LSMs. I will
>> settle for an api that has utility for a subset
>>
>>> remove_policy, etc. This set as-is appears to be an incomplete design. The
>>
>> To have a complete design, there needs to be feedback and discussion
>> from multiple LSMs. This is a starting point.
>>
>>> implementation, with call_int_hook, is also problematic. It can easily
>>> cause some> controversial behaviors.
>>>
>> agreed it shouldn't be doing a straight call_int_hook, it should only
>> call it against the lsm identified by the lsmid
>
> Yes, but then, I don't see the point of a "generic" LSM syscall.
its not a generic LSM syscall. Its a syscall or maybe a set of syscalls
for a specific scoped problem of loading/managing policy.
Can we come to something acceptable? I don't know but we are going to
look at it before trying for an apparmor specific syscall.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: Casey Schaufler @ 2025-05-09 14:21 UTC (permalink / raw)
To: Mickaël Salaün
Cc: John Johansen, Paul Moore, Maxime Bélair, Tetsuo Handa,
linux-security-module, jmorris, serge, kees, stephen.smalley.work,
takedakn, linux-api, apparmor, linux-kernel, Arnd Bergmann,
Casey Schaufler
In-Reply-To: <20250509.Chuecae0phoo@digikod.net>
On 5/9/2025 3:26 AM, Mickaël Salaün wrote:
> On Thu, May 08, 2025 at 09:54:19AM -0700, Casey Schaufler wrote:
>> On 5/8/2025 1:29 AM, John Johansen wrote:
>>> On 5/7/25 13:25, Paul Moore wrote:
>>>> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
>>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>> On 2025/05/06 23:32, Maxime Bélair wrote:
>>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>>>> index dcaad8818679..b39e6635a7d5 100644
>>>>>> --- a/security/lsm_syscalls.c
>>>>>> +++ b/security/lsm_syscalls.c
>>>>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user
>>>>>> *, ids, u32 __user *, size,
>>>>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void
>>>>>> __user *, buf, u32
>>>>>> __user *, size, u32, flags)
>>>>>> {
>>>>>> - return 0;
>>>>>> + size_t usize;
>>>>>> +
>>>>>> + if (get_user(usize, size))
>>>>>> + return -EFAULT;
>>>>>> +
>>>>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize,
>>>>>> flags);
>>>>>> }
>>>>> syzbot will report user-controlled unbounded huge size memory
>>>>> allocation attempt. ;-)
>>>>>
>>>>> This interface might be fine for AppArmor, but TOMOYO won't use this
>>>>> interface because
>>>>> TOMOYO's policy is line-oriented ASCII text data where the
>>>>> destination is switched via
>>>>> pseudo‑filesystem's filename ...
>>>> While Tetsuo's comment is limited to TOMOYO, I believe the argument
>>>> applies to a number of other LSMs as well. The reality is that there
>>>> is no one policy ideal shared across LSMs and that complicates things
>>>> like the lsm_manage_policy() proposal. I'm intentionally saying
>>>> "complicates" and not "prevents" because I don't want to flat out
>>>> reject something like this, but I think there needs to be a larger
>>>> discussion among the different LSM groups about what such an API
>>>> should look like. We may not need to get every LSM to support this
>>>> new API, but we need to get something that would work for a
>>>> significant majority and would be general/extensible enough that we
>>>> would expect it to work with the majority of future LSMs (as much as
>>>> we can predict the future anyway).
>>>>
>>> yep, I look at this is just a starting point for discussion. There
>>> isn't going to be any discussion without some code, so here is a v1
>>> that supports a single LSM let the bike shedding begin.
>> Aside from the issues with allocating a buffer for a big policy
>> I don't see a problem with this proposal. The system call looks
>> a lot like the other LSM interfaces, so any developer who likes
>> those ought to like this one. The infrastructure can easily check
>> the lsm_id and only call the appropriate LSM hook, so no one
>> is going to be interfering with other modules.
> We may not want to only be able to load buffers containing policies, but
> also to leverage file descriptors like Landlock does. Getting a
> property from a kernel object or updating it is mainly about dealing
> with a buffer. And the current LSM syscalls do just that. Other kind
> of operations may require more than that though.
>
> I don't like multiplexer syscalls because they don't expose a clear
> semantic and can be complex to manage and filter. This new syscall is
> kind of a multiplexer that redirect commands to an arbitrary set of
> kernel parts, which can then define their own semantic. I'd like to see
> a clear set of well-defined operations and their required permission.
> Even better, one syscall per operation should simplify their interface.
The development and maintenance of system calls is expensive in both
time and effort. LSM specific system calls frighten me. When I was
young adding system calls was just not done. A system call would
never be allowed for a specific sub-system or optional feature. True,
there are issues with the LSM specific filesystem approach. But I
like it, as it allows the LSM more freedom in its interfaces and
won't clutter the API if the LSM goes away or quits using it.
^ permalink raw reply
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: Mickaël Salaün @ 2025-05-09 10:26 UTC (permalink / raw)
To: John Johansen
Cc: Song Liu, Maxime Bélair, linux-security-module, paul,
jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
penguin-kernel, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <9aaeda3a-8ef5-4820-b2e4-9180b73fb368@canonical.com>
On Thu, May 08, 2025 at 01:18:20AM -0700, John Johansen wrote:
> On 5/7/25 23:06, Song Liu wrote:
> > On Wed, May 7, 2025 at 8:37 AM Maxime Bélair
> > <maxime.belair@canonical.com> wrote:
> > [...]
> > > >
> > > > These two do not feel like real benefits:
> > > > - One syscall cannot fit all use cases well...
> > >
> > > This syscall is not intended to cover every case, nor to replace existing kernel
> > > interfaces.
> > >
> > > Each LSM can decide which operations it wants to support (if any). For example, when
> > > loading policies, an LSM may choose to allow only policies that further restrict
> > > privileges.
> > >
> > > > - Not working in containers is often not an issue, but a feature.
> > >
> > > Indeed, using this syscall requires appropriate capabilities and will not permit
> > > unprivileged containers to manage policies arbitrarily.
> > >
> > > With this syscall, capability checks remain the responsibility of each LSM.
> > >
> > > For instance, in the AppArmor patch, a profile can be loaded only if
> > > aa_policy_admin_capable() succeeds (which requires CAP_MAC_ADMIN). Moreover, by design,
> > > policies can be loaded only in the current namespace.
> > >
> > > I see this syscall as a middle point between exposing the entire sysfs, creating a large
> > > attack surface, and blocking everything.
> > >
> > > Landlock’s existing syscalls already improve security by allowing processes to further
> > > restrict their ambient rights while adding only a modest attack surface.
> > >
> > > This syscall is a further step in that direction: it lets LSMs add restrictive policies
> > > without requiring exposing every other interface.
> >
> > I don't think a syscall makes the API more secure. If necessary, we can add
>
> It exposes a different attack surface. Requiring mounting of the fs to where it is visible
> in the container, provides attack surface, and requires additional external configuration.
We should also keep in mind that syscalls could be accessible from
everywhere, by everyone, which may increase the attack surface compared
to a privileged filesystem interface. Adding a second interface may
also introduce issues. Anyway, I'm definitely not against syscalls, but
I don't see why the filesystem interface would be "less secure" in this
context.
>
> Then there is the whole issue of getting the various LSMs to allow another LSM in the
> stack to be able manage its own policy.
Right, and it's a similar issue with seccomp policies wrt syscalls.
>
> > permission check to each pseudo file. The downside of the syscall, however,
> > is that all the permission checks are hard-coded in the kernel (except for
>
> The permission checks don't have to be hard coded. Each LSM can define how it handles
> or manages the syscall. The default is that it isn't supported, but if an lsm decides
> to support it, there is now reason that its policy can't determine the use of the
> syscall.
From an interface design point of view, it would be better to clearly
specify the scope of a command (e.g. which components could be impacted
by a command), and make sure the documentation reflect that as well.
Even better, have a syscalls per required privileges and impact (e.g.
privileged or unprivileged). Going this road, I'm not sure if a
privileged syscall would make sense given the existing filesystem
interface.
>
> > BPF LSM); while the sys admin can configure permissions of the pseudo
> > files in user space.
> >
> Other LSMs also have policy that can control access to pseudo filesystems and
> other resources. Again, the control doesn't have to be hard coded. And seccomp can
> be used to block the syscall.
>
>
>
> > > Again, each module decides which operations to expose through this syscall. In many cases
> > > the operation will still require CAP_SYS_ADMIN or a similar capability, so environments
> > > that choose this interface remain secure while gaining its advantages.
> > >
> > > > > - Avoids overhead of other kernel interfaces for better efficiency
> > > >
> > > > .. and it is is probably less efficient, because everything need to
> > > > fit in the same API.
> > >
> > > As shown below, the syscall can significantly improve the performance of policy management.
> > > A more detailed benchmark is available in [1].
> > >
> > > The following table presents the time required to load an AppArmor profile.
> > >
> > > For every cell, the first value is the total time taken by aa-load, and the value in
> > > parentheses is the time spent to load the policy in the kernel only (total - dry‑run).
> > >
> > > Results are in microseconds and are averaged over 10 000 runs to reduce variance.
> > >
> > >
> > > | t (µs) | syscall | pseudofs | Speedup |
> > > |-----------|-------------|-------------|---------------|
> > > | 1password | 4257 (1127) | 3333 (192) | x1.28 (x5.86) |
> > > | Xorg | 6099 (2961) | 5167 (2020) | x1.18 (x1.47) |
> > >
> >
> > I am not sure the performance of loading security policies is on any
> > critical path.
>
> generally speaking I agree, but I am also not going to turn down a
> performance improvement either. Its a nice to have, but not a strong
> argument for need.
>
> > The implementation calls the hook for each LSM, which is why I think the
> > syscall is not efficient.
> >
> it should only call the LSM identified by the lsmid in the call.
>
> > Overall, I am still not convinced a syscall for all LSMs is needed. To
> > justify such
>
> its not needed by all LSMs, just a subset of them, and some nebulous
> subset of potentially future LSMs that is entirely undefinable.
>
> If we had had appropriate LSM syscalls landlock wouldn't have needed
> to have landlock specific syscalls. Having another LSM go that route
> feels wrong especially now that we have some LSM syscalls.
I don't agree. Dedicated syscalls are a good thing. See my other
reply.
> If a
> syscall is needed by an LSM its better to try hashing something out
> that might have utility for multiple LSMs or at the very least,
> potentially have utility in the future.
>
>
> > a syscall, I think we need to show that it is useful in multiple LSMs.
> > Also, if we
> > really want to have single set of APIs for all LSMs, we may also need
> > get_policy,
>
> We are never going to get a single set of APIs for all LSMs. I will
> settle for an api that has utility for a subset
>
> > remove_policy, etc. This set as-is appears to be an incomplete design. The
>
> To have a complete design, there needs to be feedback and discussion
> from multiple LSMs. This is a starting point.
>
> > implementation, with call_int_hook, is also problematic. It can easily
> > cause some> controversial behaviors.
> >
> agreed it shouldn't be doing a straight call_int_hook, it should only
> call it against the lsm identified by the lsmid
Yes, but then, I don't see the point of a "generic" LSM syscall.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: Mickaël Salaün @ 2025-05-09 10:26 UTC (permalink / raw)
To: Casey Schaufler
Cc: John Johansen, Paul Moore, Maxime Bélair, Tetsuo Handa,
linux-security-module, jmorris, serge, kees, stephen.smalley.work,
takedakn, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <efe5b15a-6141-424a-8391-9092e79e4acf@schaufler-ca.com>
On Thu, May 08, 2025 at 09:54:19AM -0700, Casey Schaufler wrote:
> On 5/8/2025 1:29 AM, John Johansen wrote:
> > On 5/7/25 13:25, Paul Moore wrote:
> >> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
> >> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>> On 2025/05/06 23:32, Maxime Bélair wrote:
> >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> >>>> index dcaad8818679..b39e6635a7d5 100644
> >>>> --- a/security/lsm_syscalls.c
> >>>> +++ b/security/lsm_syscalls.c
> >>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user
> >>>> *, ids, u32 __user *, size,
> >>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void
> >>>> __user *, buf, u32
> >>>> __user *, size, u32, flags)
> >>>> {
> >>>> - return 0;
> >>>> + size_t usize;
> >>>> +
> >>>> + if (get_user(usize, size))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize,
> >>>> flags);
> >>>> }
> >>>
> >>> syzbot will report user-controlled unbounded huge size memory
> >>> allocation attempt. ;-)
> >>>
> >>> This interface might be fine for AppArmor, but TOMOYO won't use this
> >>> interface because
> >>> TOMOYO's policy is line-oriented ASCII text data where the
> >>> destination is switched via
> >>> pseudo‑filesystem's filename ...
> >>
> >> While Tetsuo's comment is limited to TOMOYO, I believe the argument
> >> applies to a number of other LSMs as well. The reality is that there
> >> is no one policy ideal shared across LSMs and that complicates things
> >> like the lsm_manage_policy() proposal. I'm intentionally saying
> >> "complicates" and not "prevents" because I don't want to flat out
> >> reject something like this, but I think there needs to be a larger
> >> discussion among the different LSM groups about what such an API
> >> should look like. We may not need to get every LSM to support this
> >> new API, but we need to get something that would work for a
> >> significant majority and would be general/extensible enough that we
> >> would expect it to work with the majority of future LSMs (as much as
> >> we can predict the future anyway).
> >>
> >
> > yep, I look at this is just a starting point for discussion. There
> > isn't going to be any discussion without some code, so here is a v1
> > that supports a single LSM let the bike shedding begin.
>
> Aside from the issues with allocating a buffer for a big policy
> I don't see a problem with this proposal. The system call looks
> a lot like the other LSM interfaces, so any developer who likes
> those ought to like this one. The infrastructure can easily check
> the lsm_id and only call the appropriate LSM hook, so no one
> is going to be interfering with other modules.
We may not want to only be able to load buffers containing policies, but
also to leverage file descriptors like Landlock does. Getting a
property from a kernel object or updating it is mainly about dealing
with a buffer. And the current LSM syscalls do just that. Other kind
of operations may require more than that though.
I don't like multiplexer syscalls because they don't expose a clear
semantic and can be complex to manage and filter. This new syscall is
kind of a multiplexer that redirect commands to an arbitrary set of
kernel parts, which can then define their own semantic. I'd like to see
a clear set of well-defined operations and their required permission.
Even better, one syscall per operation should simplify their interface.
^ permalink raw reply
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: Mickaël Salaün @ 2025-05-09 10:25 UTC (permalink / raw)
To: John Johansen
Cc: Tetsuo Handa, Maxime Bélair, Song Liu, linux-security-module,
paul, jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <7da224cd-fd9c-4c80-9a23-cb977420f50b@canonical.com>
On Thu, May 08, 2025 at 12:52:55AM -0700, John Johansen wrote:
> On 5/7/25 15:04, Tetsuo Handa wrote:
> > On 2025/05/08 0:37, Maxime Bélair wrote:
> > > Again, each module decides which operations to expose through this syscall. In many cases
> > > the operation will still require CAP_SYS_ADMIN or a similar capability, so environments
> > > that choose this interface remain secure while gaining its advantages.
> >
> > If the interpretation of "flags" argument varies across LSMs, it sounds like ioctl()'s
>
> yes that does feel like ioctls(), on the other hand defining them at the LSM level won't
> offer LSMs flexibility making it so the syscall covers fewer use cases. I am not opposed
> to either, it just hashing out what people want, and what is acceptable.
>
> > "cmd" argument. Also, there is prctl() which can already carry string-ish parameters
> > without involving open(). Why can't we use prctl() instead of lsm_manage_policy() ?
> >
>
> prctl() can be used, I used it for the unprivileged policy demo. It has its own set of
> problems. While LSM policy could be associated with the process doing the load/replacement
> or what ever operation, it isn't necessarily tied to it. A lot of LSM policy is not
> process specific making prctl() a poor fit.
>
> prctl() requires allocating a global prctl()
>
> prctl() are already being filtered/controlled by LSMs making them a poort fit for
> use by an LSM in a stacking situation as it requires updating the policy of other
> LSMs on the system. Yes seccomp can filter the syscall but that still is an easier
> barrier to overcome than having to have instruction for how to allow your LSMs
> prctl() in multiple LSMs.
>
>
> Mickaël already argued the need for landlock to have syscalls. See
Landlock indeed requires syscalls mainly because of its unprivileged
nature.
> https://lore.kernel.org/lkml/20200511192156.1618284-7-mic@digikod.net/
> and the numerous iterations before that.
This link might be misleading though, it points to an initial version of
the syscall proposal (v17) and it was then decided to create one syscall
per operation (v34), which is why we ended with 3 syscalls. See the
changelog:
https://lore.kernel.org/r/20210422154123.13086-9-mic@digikod.net
>
> Ideally those could have been LSM syscalls, with landlock leveraging them.
I don't agree. The Landlock syscalls have a well-defined semantic, with
documented security requirements, and they deal with specific kernel
objects identified with file descriptors, including a dedicated one:
[landlock-ruleset]. For the features provided by these Landlock
syscalls, it would not have been a good idea to reuse existing syscalls,
nor to rely on the syscall proposed in this series because the interface
is too specific to some of the current privileged LSMs (i.e. ingest a
policy blob). Making this interface more generic would lead to even
less defined semantic though.
> AppArmor
> is getting to where it has similar needs to landlock. Yes we can use ioctls, prctls,
> netlink, the fs, etc. it doesn't mean that those are the best interfaces to do so,
I think it would make sense to propose AppArmor-specific syscalls.
> and ideally any interface we use will be of benefit to some other LSMs in the future.
The LSM syscalls may make sense to deal with LSM blobs managed by the
LSM framework (e.g. get/set properties) when the operations are
common/generic.
Security policies are specific to each LSM and they should implement
their own well-defined interface (e.g. filesystem, netlink, syscall).
The LSM framework doesn't provide nor manage any security policy, it
mainly provides a set of consistent and well-defined kernel hooks with
security blobs to enforce a security policy. I don't think it makes
sense to add LSM syscalls to manage things not managed by the LSM
framework.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-09 3:25 UTC (permalink / raw)
To: Tetsuo Handa, Maxime Bélair, linux-security-module
Cc: paul, jmorris, serge, mic, kees, stephen.smalley.work, casey,
takedakn, linux-api, apparmor, linux-kernel
In-Reply-To: <75c7424b-fec9-469b-8f73-50ab86948a24@I-love.SAKURA.ne.jp>
On 5/8/25 08:07, Tetsuo Handa wrote:
> On 2025/05/08 23:44, John Johansen wrote:
>> On 5/8/25 05:55, Tetsuo Handa wrote:
>>> On 2025/05/08 17:25, John Johansen wrote:
>>>> That is fine. But curious I am curious what the interface would look like to fit TOMOYO's
>>>> needs.
>>>
>>> Stream (like "FILE *") with restart from the beginning (like rewind(fp)) support.
>>> That is, the caller can read/write at least one byte at a time, and written data
>>> is processed upon encountering '\n'.
>>>
>>
>> that can be emulated within the current sycall, where the lsm maintains a buffer.
>
> That cannot be emulated, for there is no event that is automatically triggered when
> the process terminates (i.e. implicit close() upon exit()) in order to release the
> buffer the LSM maintains.
>
security_task_free()
>> Are you asking to also read data back out as well, that could be added, but doing
>> a syscall per byte here or through the fs is going to have fairly high overhead.
>
> At least one byte means arbitrary bytes; that is, the caller does not need to read
> or write the whole policy at one syscall.
>
got it
>>
>> Without understanding the requirement it would seem to me, that it would be
>> better to emulate that file buffer manipulation in userspace similar say C++
>> stringstreams, and then write the syscall when done.
>
> The size of the whole policy in byte varies a lot.
>
sure, buffers can be variable length. AppArmor policy also varies a lot in size.
More than anything I am trying to understand TOMOYO's requirements. They do
align better with using an fs interface. Can they be met sure, but it would
be more work for TOMOYO.
One of the big motivations for the syscall from the apparmor side is getting
away from the need to have the vfs present or having to pass an fd into the
environment.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: Casey Schaufler @ 2025-05-08 16:54 UTC (permalink / raw)
To: John Johansen, Paul Moore, Maxime Bélair
Cc: Tetsuo Handa, linux-security-module, jmorris, serge, mic, kees,
stephen.smalley.work, takedakn, linux-api, apparmor, linux-kernel,
Casey Schaufler
In-Reply-To: <120954c2-87b7-4bda-958b-2b4f0180a736@canonical.com>
On 5/8/2025 1:29 AM, John Johansen wrote:
> On 5/7/25 13:25, Paul Moore wrote:
>> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>> On 2025/05/06 23:32, Maxime Bélair wrote:
>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>> index dcaad8818679..b39e6635a7d5 100644
>>>> --- a/security/lsm_syscalls.c
>>>> +++ b/security/lsm_syscalls.c
>>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user
>>>> *, ids, u32 __user *, size,
>>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void
>>>> __user *, buf, u32
>>>> __user *, size, u32, flags)
>>>> {
>>>> - return 0;
>>>> + size_t usize;
>>>> +
>>>> + if (get_user(usize, size))
>>>> + return -EFAULT;
>>>> +
>>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize,
>>>> flags);
>>>> }
>>>
>>> syzbot will report user-controlled unbounded huge size memory
>>> allocation attempt. ;-)
>>>
>>> This interface might be fine for AppArmor, but TOMOYO won't use this
>>> interface because
>>> TOMOYO's policy is line-oriented ASCII text data where the
>>> destination is switched via
>>> pseudo‑filesystem's filename ...
>>
>> While Tetsuo's comment is limited to TOMOYO, I believe the argument
>> applies to a number of other LSMs as well. The reality is that there
>> is no one policy ideal shared across LSMs and that complicates things
>> like the lsm_manage_policy() proposal. I'm intentionally saying
>> "complicates" and not "prevents" because I don't want to flat out
>> reject something like this, but I think there needs to be a larger
>> discussion among the different LSM groups about what such an API
>> should look like. We may not need to get every LSM to support this
>> new API, but we need to get something that would work for a
>> significant majority and would be general/extensible enough that we
>> would expect it to work with the majority of future LSMs (as much as
>> we can predict the future anyway).
>>
>
> yep, I look at this is just a starting point for discussion. There
> isn't going to be any discussion without some code, so here is a v1
> that supports a single LSM let the bike shedding begin.
Aside from the issues with allocating a buffer for a big policy
I don't see a problem with this proposal. The system call looks
a lot like the other LSM interfaces, so any developer who likes
those ought to like this one. The infrastructure can easily check
the lsm_id and only call the appropriate LSM hook, so no one
is going to be interfering with other modules.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: Tetsuo Handa @ 2025-05-08 15:07 UTC (permalink / raw)
To: John Johansen, Maxime Bélair, linux-security-module
Cc: paul, jmorris, serge, mic, kees, stephen.smalley.work, casey,
takedakn, linux-api, apparmor, linux-kernel
In-Reply-To: <07a496b2-ed1f-4a18-88d1-7be36dba3a8a@canonical.com>
On 2025/05/08 23:44, John Johansen wrote:
> On 5/8/25 05:55, Tetsuo Handa wrote:
>> On 2025/05/08 17:25, John Johansen wrote:
>>> That is fine. But curious I am curious what the interface would look like to fit TOMOYO's
>>> needs.
>>
>> Stream (like "FILE *") with restart from the beginning (like rewind(fp)) support.
>> That is, the caller can read/write at least one byte at a time, and written data
>> is processed upon encountering '\n'.
>>
>
> that can be emulated within the current sycall, where the lsm maintains a buffer.
That cannot be emulated, for there is no event that is automatically triggered when
the process terminates (i.e. implicit close() upon exit()) in order to release the
buffer the LSM maintains.
> Are you asking to also read data back out as well, that could be added, but doing
> a syscall per byte here or through the fs is going to have fairly high overhead.
At least one byte means arbitrary bytes; that is, the caller does not need to read
or write the whole policy at one syscall.
>
> Without understanding the requirement it would seem to me, that it would be
> better to emulate that file buffer manipulation in userspace similar say C++
> stringstreams, and then write the syscall when done.
The size of the whole policy in byte varies a lot.
^ permalink raw reply
* Re: The "make headers" requirement, revisited: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Shuah Khan @ 2025-05-08 15:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: John Hubbard, Peter Zijlstra, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, Christian Brauner, Lorenzo Stoakes, Shuah Khan
In-Reply-To: <aBy5503w_GuNTu9B@google.com>
On 5/8/25 08:04, Sean Christopherson wrote:
> On Wed, May 07, 2025, Shuah Khan wrote:
>> The issues Peter is seeing regarding KHDR_INCLUDES in the following
>> tests can be easily fixed by simply changing the test Makefile. These
>> aren't framework related.
>>
>> kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
>
> ...
>
>> You can make the change to remove the reference to KHDR_INCLUDES.
>> If don't have the time/bandwidth to do it, I will take care of it.
>
> Please don't remove the KHDR_INCLUDES usage in KVM's selftests, KVM routinely
> adds tests for new uAPI. Having to manually install headers is annoying, but
> IMO it's the least awful solution we have.
Thank you for confirming that KHDR_INCLUDES customization is necessary
for some tests such as kvm.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-08 14:44 UTC (permalink / raw)
To: Tetsuo Handa, Maxime Bélair, linux-security-module
Cc: paul, jmorris, serge, mic, kees, stephen.smalley.work, casey,
takedakn, linux-api, apparmor, linux-kernel
In-Reply-To: <75c0385c-b649-46b0-907f-903e2217f460@I-love.SAKURA.ne.jp>
On 5/8/25 05:55, Tetsuo Handa wrote:
> On 2025/05/08 17:25, John Johansen wrote:
>> That is fine. But curious I am curious what the interface would look like to fit TOMOYO's
>> needs.
>
> Stream (like "FILE *") with restart from the beginning (like rewind(fp)) support.
> That is, the caller can read/write at least one byte at a time, and written data
> is processed upon encountering '\n'.
>
that can be emulated within the current sycall, where the lsm maintains a buffer.
Are you asking to also read data back out as well, that could be added, but doing
a syscall per byte here or through the fs is going to have fairly high overhead.
Without understanding the requirement it would seem to me, that it would be
better to emulate that file buffer manipulation in userspace similar say C++
stringstreams, and then write the syscall when done.
^ permalink raw reply
* Re: The "make headers" requirement, revisited: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Sean Christopherson @ 2025-05-08 14:04 UTC (permalink / raw)
To: Shuah Khan
Cc: John Hubbard, Peter Zijlstra, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, Christian Brauner, Lorenzo Stoakes
In-Reply-To: <e87bbc68-0403-4d67-ae2d-64065e36a011@linuxfoundation.org>
On Wed, May 07, 2025, Shuah Khan wrote:
> The issues Peter is seeing regarding KHDR_INCLUDES in the following
> tests can be easily fixed by simply changing the test Makefile. These
> aren't framework related.
>
> kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
...
> You can make the change to remove the reference to KHDR_INCLUDES.
> If don't have the time/bandwidth to do it, I will take care of it.
Please don't remove the KHDR_INCLUDES usage in KVM's selftests, KVM routinely
adds tests for new uAPI. Having to manually install headers is annoying, but
IMO it's the least awful solution we have.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: Tetsuo Handa @ 2025-05-08 12:55 UTC (permalink / raw)
To: John Johansen, Maxime Bélair, linux-security-module
Cc: paul, jmorris, serge, mic, kees, stephen.smalley.work, casey,
takedakn, linux-api, apparmor, linux-kernel
In-Reply-To: <6d785712-6d8e-491c-86d4-1cbe5895778f@canonical.com>
On 2025/05/08 17:25, John Johansen wrote:
> That is fine. But curious I am curious what the interface would look like to fit TOMOYO's
> needs.
Stream (like "FILE *") with restart from the beginning (like rewind(fp)) support.
That is, the caller can read/write at least one byte at a time, and written data
is processed upon encountering '\n'.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-08 8:29 UTC (permalink / raw)
To: Paul Moore, Maxime Bélair
Cc: Tetsuo Handa, linux-security-module, jmorris, serge, mic, kees,
stephen.smalley.work, casey, takedakn, linux-api, apparmor,
linux-kernel
In-Reply-To: <CAHC9VhRKwB4quqBtYQyxRqCX2C6fCgTbyAP3Ov+NdQ06t1aFdA@mail.gmail.com>
On 5/7/25 13:25, Paul Moore wrote:
> On Wed, May 7, 2025 at 6:41 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> On 2025/05/06 23:32, Maxime Bélair wrote:
>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>> index dcaad8818679..b39e6635a7d5 100644
>>> --- a/security/lsm_syscalls.c
>>> +++ b/security/lsm_syscalls.c
>>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
>>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void __user *, buf, u32
>>> __user *, size, u32, flags)
>>> {
>>> - return 0;
>>> + size_t usize;
>>> +
>>> + if (get_user(usize, size))
>>> + return -EFAULT;
>>> +
>>> + return security_lsm_manage_policy(lsm_id, op, buf, usize, flags);
>>> }
>>
>> syzbot will report user-controlled unbounded huge size memory allocation attempt. ;-)
>>
>> This interface might be fine for AppArmor, but TOMOYO won't use this interface because
>> TOMOYO's policy is line-oriented ASCII text data where the destination is switched via
>> pseudo‑filesystem's filename ...
>
> While Tetsuo's comment is limited to TOMOYO, I believe the argument
> applies to a number of other LSMs as well. The reality is that there
> is no one policy ideal shared across LSMs and that complicates things
> like the lsm_manage_policy() proposal. I'm intentionally saying
> "complicates" and not "prevents" because I don't want to flat out
> reject something like this, but I think there needs to be a larger
> discussion among the different LSM groups about what such an API
> should look like. We may not need to get every LSM to support this
> new API, but we need to get something that would work for a
> significant majority and would be general/extensible enough that we
> would expect it to work with the majority of future LSMs (as much as
> we can predict the future anyway).
>
yep, I look at this is just a starting point for discussion. There
isn't going to be any discussion without some code, so here is a v1
that supports a single LSM let the bike shedding begin.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-08 8:25 UTC (permalink / raw)
To: Tetsuo Handa, Maxime Bélair, linux-security-module
Cc: paul, jmorris, serge, mic, kees, stephen.smalley.work, casey,
takedakn, linux-api, apparmor, linux-kernel
In-Reply-To: <9c68743f-5efa-4a77-a29b-d3e8f2b2a462@I-love.SAKURA.ne.jp>
On 5/7/25 03:40, Tetsuo Handa wrote:
> On 2025/05/06 23:32, Maxime Bélair wrote:
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> index dcaad8818679..b39e6635a7d5 100644
>> --- a/security/lsm_syscalls.c
>> +++ b/security/lsm_syscalls.c
>> @@ -122,5 +122,10 @@ SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
>> SYSCALL_DEFINE5(lsm_manage_policy, u32, lsm_id, u32, op, void __user *, buf, u32
>> __user *, size, u32, flags)
>> {
>> - return 0;
>> + size_t usize;
>> +
>> + if (get_user(usize, size))
>> + return -EFAULT;
>> +
>> + return security_lsm_manage_policy(lsm_id, op, buf, usize, flags);
>> }
>
> syzbot will report user-controlled unbounded huge size memory allocation attempt. ;-)
>
> This interface might be fine for AppArmor, but TOMOYO won't use this interface because
> TOMOYO's policy is line-oriented ASCII text data where the destination is switched via
> pseudo‑filesystem's filename; use of filename helps restricting which type of policy
> can be manipulated by which process.
>
That is fine. But curious I am curious what the interface would look like to fit TOMOYO's
needs. I look at the current implementation as an opening discussion of what the syscall
should look like. I have no delusions that we are going to get something that will fit
all LSMs but without requirements, we won't be able to even attempt to hash something
better out.
^ permalink raw reply
* Re: [PATCH 2/3] lsm: introduce security_lsm_manage_policy hook
From: John Johansen @ 2025-05-08 8:20 UTC (permalink / raw)
To: Maxime Bélair, Song Liu
Cc: linux-security-module, paul, jmorris, serge, mic, kees,
stephen.smalley.work, casey, takedakn, penguin-kernel, linux-api,
apparmor, linux-kernel
In-Reply-To: <bc252425-2703-48c4-a1fa-9268124c2386@canonical.com>
On 5/7/25 08:37, Maxime Bélair wrote:
>
>
> On 5/7/25 08:19, Song Liu wrote:
>> On Tue, May 6, 2025 at 7:40 AM Maxime Bélair
>> <maxime.belair@canonical.com> wrote:
>>>
>>> Define a new LSM hook security_lsm_manage_policy and wire it into the
>>> lsm_manage_policy() syscall so that LSMs can register a unified interface
>>> for policy management. This initial, minimal implementation only supports
>>> the LSM_POLICY_LOAD operation to limit changes.
>>>
>>> Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
>> [...]
>>> diff --git a/security/security.c b/security/security.c
>>> index fb57e8fddd91..256104e338b1 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -5883,6 +5883,27 @@ int security_bdev_setintegrity(struct block_device *bdev,
>>> }
>>> EXPORT_SYMBOL(security_bdev_setintegrity);
>>>
>>> +/**
>>> + * security_lsm_manage_policy() - Manage the policies of LSMs
>>> + * @lsm_id: id of the lsm to target
>>> + * @op: Operation to perform (one of the LSM_POLICY_XXX values)
>>> + * @buf: userspace pointer to policy data
>>> + * @size: size of @buf
>>> + * @flags: lsm policy management flags
>>> + *
>>> + * Manage the policies of a LSM. This notably allows to update them even when
>>> + * the lsmfs is unavailable is restricted. Currently, only LSM_POLICY_LOAD is
>>> + * supported.
>>> + *
>>> + * Return: Returns 0 on success, error on failure.
>>> + */
>>> +int security_lsm_manage_policy(u32 lsm_id, u32 op, void __user *buf,
>>> + size_t size, u32 flags)
>>> +{
>>> + return call_int_hook(lsm_manage_policy, lsm_id, op, buf, size, flags);
>>
>> If the LSM doesn't implement this hook, sys_lsm_manage_policy will return 0
>> for any inputs, right? This is gonna be so confusing for users.
>
> Indeed, that was an oversight. It will return -EOPNOTSUPP in the next patch revision.
>
I think it needs to do more than that. I don't think this should call each LSM, the
infrastructure should filter it and only send it to the LSM identified by the lsm_id
^ 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