Re: [PATCH v3 2/2] fs/xattr: add *at family syscalls
On Fri 26-04-24 18:20:14, Christian Göttsche wrote:
> From: Christian Göttsche
>
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc//fd/ detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions and without a file
> descriptor opened with read access requiring SELinux read permission.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Pass the value of the extended attribute, its length, and for
> setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
> struct xattr_args to not exceed six syscall arguments and not
> merging the AT_* and XATTR_* flags.
>
> Signed-off-by: Christian Göttsche
The patch looks good to me. Just a few nits below:
> -static int path_setxattr(const char __user *pathname,
> +static int do_setxattrat(int dfd, const char __user *pathname, unsigned int
> at_flags,
Can we please stay within 80 columns (happens in multiple places in the
patch)? I don't insist but it makes things easier to read in some setups so
I prefer it.
> @@ -852,13 +908,21 @@ listxattr(struct dentry *d, char __user *list, size_t
> size)
> return error;
> }
>
> -static ssize_t path_listxattr(const char __user *pathname, char __user *list,
> - size_t size, unsigned int lookup_flags)
> +static ssize_t do_listxattrat(int dfd, const char __user *pathname, char
> __user *list,
> + size_t size, int flags)
So I like how in previous syscalls you have 'at_flags', 'lookup_flags', and
'xattr_flags'. That makes things much easier to digest. Can you please stay
with that convention here as well and call this argument 'at_flags'? Also I
think the argument ordering like "dfd, pathname, at_flags, list, size" is
more consistent with other syscalls you define.
> @@ -870,16 +934,22 @@ static ssize_t path_listxattr(const char __user
> *pathname, char __user *list,
> return error;
> }
>
> +SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname, char
> __user *, list,
> + size_t, size, int, flags)
> +{
> + return do_listxattrat(dfd, pathname, list, size, flags);
> +}
> +
Same comment as above - "flags" -> "at_flags" and reorder args please.
> @@ -917,13 +987,21 @@ removexattr(struct mnt_idmap *idmap, struct dentry *d,
> return vfs_removexattr(idmap, d, kname);
> }
>
> -static int path_removexattr(const char __user *pathname,
> - const char __user *name, unsigned int lookup_flags)
> +static int do_removexattrat(int dfd, const char __user *pathname,
> + const char __user *name, int flags)
> {
Same comment as above - "flags" -> "at_flags" and reorder args please.
> @@ -939,16 +1017,22 @@ static int path_removexattr(const char __user
> *pathname,
> return error;
> }
>
> +SYSCALL_DEFINE4(removexattrat, int, dfd, const char __user *, pathname,
> + const char __user *, name, int, flags)
> +{
Same comment as above - "flags" -> "at_flags" and reorder args please.
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH v3 2/2] fs/xattr: add *at family syscalls
On Fri, Apr 26, 2024, at 18:20, Christian Göttsche wrote:
> From: Christian Göttsche
>
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc//fd/ detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions and without a file
> descriptor opened with read access requiring SELinux read permission.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Pass the value of the extended attribute, its length, and for
> setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
> struct xattr_args to not exceed six syscall arguments and not
> merging the AT_* and XATTR_* flags.
>
> Signed-off-by: Christian Göttsche
> CC: x...@kernel.org
> CC: linux-al...@vger.kernel.org
> CC: linux-ker...@vger.kernel.org
> CC: linux-arm-ker...@lists.infradead.org
> CC: linux-i...@vger.kernel.org
> CC: linux-m...@lists.linux-m68k.org
> CC: linux-m...@vger.kernel.org
> CC: linux-par...@vger.kernel.org
> CC: linuxppc-dev@lists.ozlabs.org
> CC: linux-s...@vger.kernel.org
> CC: linux...@vger.kernel.org
> CC: sparcli...@vger.kernel.org
> CC: linux-fsde...@vger.kernel.org
> CC: au...@vger.kernel.org
> CC: linux-a...@vger.kernel.org
> CC: linux-...@vger.kernel.org
> CC: linux-security-mod...@vger.kernel.org
> CC: seli...@vger.kernel.org
I checked that the syscalls are all well-formed regarding
argument types, number of arguments and (absence of)
compat handling, and that they are wired up correctly
across architectures
I did not look at the actual implementation in detail.
Reviewed-by: Arnd Bergmann
[PATCH v3 2/2] fs/xattr: add *at family syscalls
From: Christian Göttsche
Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
removexattrat(). Those can be used to operate on extended attributes,
especially security related ones, either relative to a pinned directory
or on a file descriptor without read access, avoiding a
/proc//fd/ detour, requiring a mounted procfs.
One use case will be setfiles(8) setting SELinux file contexts
("security.selinux") without race conditions and without a file
descriptor opened with read access requiring SELinux read permission.
Use the do_{name}at() pattern from fs/open.c.
Pass the value of the extended attribute, its length, and for
setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
struct xattr_args to not exceed six syscall arguments and not
merging the AT_* and XATTR_* flags.
Signed-off-by: Christian Göttsche
CC: x...@kernel.org
CC: linux-al...@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: linux-arm-ker...@lists.infradead.org
CC: linux-i...@vger.kernel.org
CC: linux-m...@lists.linux-m68k.org
CC: linux-m...@vger.kernel.org
CC: linux-par...@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-s...@vger.kernel.org
CC: linux...@vger.kernel.org
CC: sparcli...@vger.kernel.org
CC: linux-fsde...@vger.kernel.org
CC: au...@vger.kernel.org
CC: linux-a...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-security-mod...@vger.kernel.org
CC: seli...@vger.kernel.org
---
v3:
- pass value, size and xattr_flags via new struct xattr_args to
split AT_* and XATTR_* flags
v2: https://lore.kernel.org/lkml/20230511150802.737477-1-cgzo...@googlemail.com/
- squash syscall introduction and wire up commits
- add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
v1 discussion:
https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
Previous approach ("f*xattr: allow O_PATH descriptors"):
https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
---
arch/alpha/kernel/syscalls/syscall.tbl | 4 +
arch/arm/tools/syscall.tbl | 4 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 8 ++
arch/m68k/kernel/syscalls/syscall.tbl | 4 +
arch/microblaze/kernel/syscalls/syscall.tbl | 4 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 4 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 4 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 4 +
arch/parisc/kernel/syscalls/syscall.tbl | 4 +
arch/powerpc/kernel/syscalls/syscall.tbl| 4 +
arch/s390/kernel/syscalls/syscall.tbl | 4 +
arch/sh/kernel/syscalls/syscall.tbl | 4 +
arch/sparc/kernel/syscalls/syscall.tbl | 4 +
arch/x86/entry/syscalls/syscall_32.tbl | 4 +
arch/x86/entry/syscalls/syscall_64.tbl | 4 +
arch/xtensa/kernel/syscalls/syscall.tbl | 4 +
fs/xattr.c | 128
include/asm-generic/audit_change_attr.h | 6 +
include/linux/syscalls.h| 10 ++
include/uapi/asm-generic/unistd.h | 12 +-
include/uapi/linux/xattr.h | 6 +
22 files changed, 208 insertions(+), 24 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl
b/arch/alpha/kernel/syscalls/syscall.tbl
index 8ff110826ce2..fdc11249f1b8 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -501,3 +501,7 @@
569common lsm_get_self_attr sys_lsm_get_self_attr
570common lsm_set_self_attr sys_lsm_set_self_attr
571common lsm_list_modulessys_lsm_list_modules
+572common setxattrat sys_setxattrat
+573common getxattrat sys_getxattrat
+574common listxattrat sys_listxattrat
+575common removexattrat sys_removexattrat
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index b6c9e01e14f5..22fbbcd8e2b5 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -475,3 +475,7 @@
459common lsm_get_self_attr sys_lsm_get_self_attr
460common lsm_set_self_attr sys_lsm_set_self_attr
461common lsm_list_modulessys_lsm_list_modules
+462common setxattrat sys_setxattrat
+463common getxattrat sys_getxattrat
+464common listxattrat sys_listxattrat
+465common removexattrat sys_removexattrat
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 491b2b9bd553..f3a77719eb05 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -39,7 +39,7 @@
#define __ARM_NR_compat_set_tls(__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END(__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 462
+#define
