diff mbox series

[RFC,1/2] fs/xattr: add *at family syscalls

Message ID 20220830152858.14866-2-cgzones@googlemail.com (mailing list archive)
State New, archived
Delegated to: Paul Moore
Headers show
Series [RFC,1/2] fs/xattr: add *at family syscalls | expand

Commit Message

Christian Göttsche Aug. 30, 2022, 3:28 p.m. UTC 
Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
removexattrat() to enable extended attribute operations via file
descriptors.  This can be used from userspace to avoid race conditions,
especially on security related extended attributes, like SELinux labels
("security.selinux") via setfiles(8).

Use the do_{name}at() pattern from fs/open.c.
Use a single flag parameter for extended attribute flags (currently
XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
syscall arguments in setxattrat().

Previous discussion ("f*xattr: allow O_PATH descriptors"): https://lore.kernel.org/all/20220607153139.35588-1-cgzones@googlemail.com/

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 fs/xattr.c | 108 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 23 deletions(-)

Comments

Christian Brauner Aug. 30, 2022, 5:09 p.m. UTC | #1 
On Tue, Aug 30, 2022 at 05:28:39PM +0200, Christian Göttsche wrote:
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat() to enable extended attribute operations via file
> descriptors.  This can be used from userspace to avoid race conditions,
> especially on security related extended attributes, like SELinux labels
> ("security.selinux") via setfiles(8).
> 
> Use the do_{name}at() pattern from fs/open.c.
> Use a single flag parameter for extended attribute flags (currently
> XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> syscall arguments in setxattrat().
> 
> Previous discussion ("f*xattr: allow O_PATH descriptors"): https://lore.kernel.org/all/20220607153139.35588-1-cgzones@googlemail.com/
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---

Having a way to operate via file descriptors on xattrs does make a lot
of sense to me in general. We've got code that changes ownership
recursively including using llistxattr(), getxattr(), and setxattr() any
xattrs that store ownership information and being able to passing in an
fd directly would be quite neat...

>  fs/xattr.c | 108 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 85 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index a1f4998bc6be..a4738e28be8c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -27,6 +27,8 @@
>  
>  #include "internal.h"
>  
> +#define XATTR__FLAGS (XATTR_CREATE | XATTR_REPLACE)
> +
>  static const char *
>  strcmp_prefix(const char *a, const char *a_prefix)
>  {
> @@ -559,7 +561,7 @@ int setxattr_copy(const char __user *name, struct xattr_ctx *ctx)
>  {
>  	int error;
>  
> -	if (ctx->flags & ~(XATTR_CREATE|XATTR_REPLACE))
> +	if (ctx->flags & ~XATTR__FLAGS)
>  		return -EINVAL;
>  
>  	error = strncpy_from_user(ctx->kname->name, name,
> @@ -626,21 +628,31 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
>  	return error;
>  }
>  
> -static int path_setxattr(const char __user *pathname,
> +static int do_setxattrat(int dfd, const char __user *pathname,
>  			 const char __user *name, const void __user *value,
> -			 size_t size, int flags, unsigned int lookup_flags)
> +			 size_t size, int flags)
>  {
>  	struct path path;
>  	int error;
> +	int lookup_flags;
> +
> +	/* AT_ and XATTR_ flags must not overlap. */
> +	BUILD_BUG_ON(((AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) & XATTR__FLAGS) != 0);
> +
> +	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | XATTR__FLAGS)) != 0)
> +		return -EINVAL;

It's a bit tasteless that we end up mixing AT_* and XATTR_* flags for sure.

>  
> +	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
> +	if (flags & AT_EMPTY_PATH)
> +		lookup_flags |= LOOKUP_EMPTY;
>  retry:
> -	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
> +	error = user_path_at(dfd, pathname, lookup_flags, &path);
>  	if (error)
>  		return error;
>  	error = mnt_want_write(path.mnt);
>  	if (!error) {
>  		error = setxattr(mnt_user_ns(path.mnt), path.dentry, name,
> -				 value, size, flags);
> +				 value, size, flags & XATTR__FLAGS);
>  		mnt_drop_write(path.mnt);
>  	}
>  	path_put(&path);
> @@ -651,18 +663,25 @@ static int path_setxattr(const char __user *pathname,
>  	return error;
>  }
>  
> +SYSCALL_DEFINE6(setxattrat, int, dfd, const char __user *, pathname,
> +		const char __user *, name, const void __user *, value,
> +		size_t, size, int, flags)
> +{
> +	return do_setxattrat(dfd, pathname, name, value, size, flags);
> +}

I'm not sure we need setxattrat()? Yes, it doesn't have a "pathname"
argument but imho it's not that big of a deal to first perform the
lookup via openat*() and then call fsetxattr() and have fsetxattr()
recognize AT_* flags. It's not that the xattr system calls are
lightweight in the first place.

But maybe the consistency is nicer. I have no strong feelings here.
Al Viro Aug. 31, 2022, 10:17 p.m. UTC | #2 
[linux-arch Cc'd for ABI-related stuff]

On Tue, Aug 30, 2022 at 05:28:39PM +0200, Christian Göttsche wrote:
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat() to enable extended attribute operations via file
> descriptors.  This can be used from userspace to avoid race conditions,
> especially on security related extended attributes, like SELinux labels
> ("security.selinux") via setfiles(8).
> 
> Use the do_{name}at() pattern from fs/open.c.
> Use a single flag parameter for extended attribute flags (currently
> XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> syscall arguments in setxattrat().

	I've no problems with the patchset aside of the flags part;
however, note that XATTR_CREATE and XATTR_REPLACE are actually exposed
to the network - the values are passed to nfsd by clients.
See nfsd4_decode_setxattr() and
        BUILD_BUG_ON(XATTR_CREATE != SETXATTR4_CREATE);
	BUILD_BUG_ON(XATTR_REPLACE != SETXATTR4_REPLACE);
in encode_setxattr() on the client side.

	Makes me really nervous about constraints like that.  Sure,
AT_... flags you are using are in the second octet and these are in
the lowest one, but...
Amir Goldstein Sept. 1, 2022, 8:20 a.m. UTC | #3 
On Thu, Sep 1, 2022 at 2:10 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> [linux-arch Cc'd for ABI-related stuff]
>
> On Tue, Aug 30, 2022 at 05:28:39PM +0200, Christian Göttsche wrote:
> > Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> > removexattrat() to enable extended attribute operations via file
> > descriptors.  This can be used from userspace to avoid race conditions,
> > especially on security related extended attributes, like SELinux labels
> > ("security.selinux") via setfiles(8).
> >
> > Use the do_{name}at() pattern from fs/open.c.
> > Use a single flag parameter for extended attribute flags (currently
> > XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> > syscall arguments in setxattrat().
>
>         I've no problems with the patchset aside of the flags part;
> however, note that XATTR_CREATE and XATTR_REPLACE are actually exposed
> to the network - the values are passed to nfsd by clients.
> See nfsd4_decode_setxattr() and
>         BUILD_BUG_ON(XATTR_CREATE != SETXATTR4_CREATE);
>         BUILD_BUG_ON(XATTR_REPLACE != SETXATTR4_REPLACE);
> in encode_setxattr() on the client side.
>
>         Makes me really nervous about constraints like that.  Sure,
> AT_... flags you are using are in the second octet and these are in
> the lowest one, but...

In this context, I would like to point at

AT_EACCESS
AT_REMOVEDIR

Which are using the same namespace as the AT_ flags but define
a flag in a "private section" of that namespace for faccessat() and
for unlinkat().
unlinkat() does not technically support any of the generic AT_ flags,
but the sycall name does suggest that it is the same namespace.

At the risk of getting shouted at, I propose that we retroactively
formalize this practice and also define
AT_XATTR_* and AT_RENAME_* constants
with the accompanied BUILD_BUG_ON()
and document above the AT_ definitions that the lowest 10 bits
are reserved as private namespace for the specific syscall.

There are also the AT_STATX_*SYNC* flags that could fall
into the category of syscall private namespace, but those flags could
actually be made more generic as there are other syscalls that may
benefit from supporting them.

linkat() is one example that comes to mind.
Similar suggestions have been posted in the past:
https://lore.kernel.org/linux-fsdevel/20190527172655.9287-1-amir73il@gmail.com/
https://lore.kernel.org/linux-fsdevel/CAOQ4uxit0KYiShpEXt8b8SvN8bWWp3Ky929b+UWNDozTCUeTxg@mail.gmail.com/

Thanks,
Amir.
Casey Schaufler Sept. 1, 2022, 4:45 p.m. UTC | #4 
On 8/31/2022 3:17 PM, Al Viro wrote:
> [linux-arch Cc'd for ABI-related stuff]

The LSM list <linux-security-module@vger.kernel.org> should be on
this thread as SELinux isn't the only security module that uses xattrs
extensively.

>
> On Tue, Aug 30, 2022 at 05:28:39PM +0200, Christian Göttsche wrote:
>> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
>> removexattrat() to enable extended attribute operations via file
>> descriptors.  This can be used from userspace to avoid race conditions,
>> especially on security related extended attributes, like SELinux labels
>> ("security.selinux") via setfiles(8).
>>
>> Use the do_{name}at() pattern from fs/open.c.
>> Use a single flag parameter for extended attribute flags (currently
>> XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
>> syscall arguments in setxattrat().
> 	I've no problems with the patchset aside of the flags part;
> however, note that XATTR_CREATE and XATTR_REPLACE are actually exposed
> to the network - the values are passed to nfsd by clients.
> See nfsd4_decode_setxattr() and
>         BUILD_BUG_ON(XATTR_CREATE != SETXATTR4_CREATE);
> 	BUILD_BUG_ON(XATTR_REPLACE != SETXATTR4_REPLACE);
> in encode_setxattr() on the client side.
>
> 	Makes me really nervous about constraints like that.  Sure,
> AT_... flags you are using are in the second octet and these are in
> the lowest one, but...
diff mbox series

Patch

diff --git a/fs/xattr.c b/fs/xattr.c
index a1f4998bc6be..a4738e28be8c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -27,6 +27,8 @@ 
 
 #include "internal.h"
 
+#define XATTR__FLAGS (XATTR_CREATE | XATTR_REPLACE)
+
 static const char *
 strcmp_prefix(const char *a, const char *a_prefix)
 {
@@ -559,7 +561,7 @@  int setxattr_copy(const char __user *name, struct xattr_ctx *ctx)
 {
 	int error;
 
-	if (ctx->flags & ~(XATTR_CREATE|XATTR_REPLACE))
+	if (ctx->flags & ~XATTR__FLAGS)
 		return -EINVAL;
 
 	error = strncpy_from_user(ctx->kname->name, name,
@@ -626,21 +628,31 @@  setxattr(struct user_namespace *mnt_userns, struct dentry *d,
 	return error;
 }
 
-static int path_setxattr(const char __user *pathname,
+static int do_setxattrat(int dfd, const char __user *pathname,
 			 const char __user *name, const void __user *value,
-			 size_t size, int flags, unsigned int lookup_flags)
+			 size_t size, int flags)
 {
 	struct path path;
 	int error;
+	int lookup_flags;
+
+	/* AT_ and XATTR_ flags must not overlap. */
+	BUILD_BUG_ON(((AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) & XATTR__FLAGS) != 0);
+
+	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | XATTR__FLAGS)) != 0)
+		return -EINVAL;
 
+	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+	if (flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
 retry:
-	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+	error = user_path_at(dfd, pathname, lookup_flags, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
 	if (!error) {
 		error = setxattr(mnt_user_ns(path.mnt), path.dentry, name,
-				 value, size, flags);
+				 value, size, flags & XATTR__FLAGS);
 		mnt_drop_write(path.mnt);
 	}
 	path_put(&path);
@@ -651,18 +663,25 @@  static int path_setxattr(const char __user *pathname,
 	return error;
 }
 
+SYSCALL_DEFINE6(setxattrat, int, dfd, const char __user *, pathname,
+		const char __user *, name, const void __user *, value,
+		size_t, size, int, flags)
+{
+	return do_setxattrat(dfd, pathname, name, value, size, flags);
+}
+
 SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
 		const char __user *, name, const void __user *, value,
 		size_t, size, int, flags)
 {
-	return path_setxattr(pathname, name, value, size, flags, LOOKUP_FOLLOW);
+	return do_setxattrat(AT_FDCWD, pathname, name, value, size, flags);
 }
 
 SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
 		const char __user *, name, const void __user *, value,
 		size_t, size, int, flags)
 {
-	return path_setxattr(pathname, name, value, size, flags, 0);
+	return do_setxattrat(AT_FDCWD, pathname, name, value, size, flags | AT_SYMLINK_NOFOLLOW);
 }
 
 SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
@@ -745,14 +764,22 @@  getxattr(struct user_namespace *mnt_userns, struct dentry *d,
 	return error;
 }
 
-static ssize_t path_getxattr(const char __user *pathname,
+static ssize_t do_getxattrat(int dfd, const char __user *pathname,
 			     const char __user *name, void __user *value,
-			     size_t size, unsigned int lookup_flags)
+			     size_t size, int flags)
 {
 	struct path path;
 	ssize_t error;
+	int lookup_flags;
+
+	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+		return -EINVAL;
+
+	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+	if (flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
 retry:
-	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+	error = user_path_at(dfd, pathname, lookup_flags, &path);
 	if (error)
 		return error;
 	error = getxattr(mnt_user_ns(path.mnt), path.dentry, name, value, size);
@@ -764,16 +791,23 @@  static ssize_t path_getxattr(const char __user *pathname,
 	return error;
 }
 
+SYSCALL_DEFINE6(getxattrat, int, dfd, const char __user *, pathname,
+		const char __user *, name, void __user *, value, size_t, size,
+		int, flags)
+{
+	return do_getxattrat(dfd, pathname, name, value, size, flags);
+}
+
 SYSCALL_DEFINE4(getxattr, const char __user *, pathname,
 		const char __user *, name, void __user *, value, size_t, size)
 {
-	return path_getxattr(pathname, name, value, size, LOOKUP_FOLLOW);
+	return do_getxattrat(AT_FDCWD, pathname, name, value, size, 0);
 }
 
 SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
 		const char __user *, name, void __user *, value, size_t, size)
 {
-	return path_getxattr(pathname, name, value, size, 0);
+	return do_getxattrat(AT_FDCWD, pathname, name, value, size, AT_SYMLINK_NOFOLLOW);
 }
 
 SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
@@ -823,13 +857,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)
 {
 	struct path path;
 	ssize_t error;
+	int lookup_flags;
+
+	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+		return -EINVAL;
+
+	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+	if (flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
 retry:
-	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+	error = user_path_at(dfd, pathname, lookup_flags, &path);
 	if (error)
 		return error;
 	error = listxattr(path.dentry, list, size);
@@ -841,16 +883,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);
+}
+
 SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
 		size_t, size)
 {
-	return path_listxattr(pathname, list, size, LOOKUP_FOLLOW);
+	return do_listxattrat(AT_FDCWD, pathname, list, size, 0);
 }
 
 SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
 		size_t, size)
 {
-	return path_listxattr(pathname, list, size, 0);
+	return do_listxattrat(AT_FDCWD, pathname, list, size, AT_SYMLINK_NOFOLLOW);
 }
 
 SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
@@ -869,7 +917,7 @@  SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
 /*
  * Extended attribute REMOVE operations
  */
-static long
+static int
 removexattr(struct user_namespace *mnt_userns, struct dentry *d,
 	    const char __user *name)
 {
@@ -885,13 +933,21 @@  removexattr(struct user_namespace *mnt_userns, struct dentry *d,
 	return vfs_removexattr(mnt_userns, 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)
 {
 	struct path path;
 	int error;
+	int lookup_flags;
+
+	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+		return -EINVAL;
+
+	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+	if (flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
 retry:
-	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+	error = user_path_at(dfd, pathname, lookup_flags, &path);
 	if (error)
 		return error;
 	error = mnt_want_write(path.mnt);
@@ -907,16 +963,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)
+{
+	return do_removexattrat(dfd, pathname, name, flags);
+}
+
 SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
 		const char __user *, name)
 {
-	return path_removexattr(pathname, name, LOOKUP_FOLLOW);
+	return do_removexattrat(AT_FDCWD, pathname, name, 0);
 }
 
 SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
 		const char __user *, name)
 {
-	return path_removexattr(pathname, name, 0);
+	return do_removexattrat(AT_FDCWD, pathname, name, AT_SYMLINK_NOFOLLOW);
 }
 
 SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)