Re: [RFC PATCH] f*xattr: allow O_PATH descriptors

[Amir Goldstein]

On Wed, Jun 8, 2022 at 5:23 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> From: Miklos Szeredi <mszeredi@xxxxxxxxxx>
>
> Support file descriptors obtained via O_PATH for extended attribute
> operations.
>
> Extended attributes are for example used by SELinux for the security
> context of file objects. To avoid time-of-check-time-of-use issues while
> setting those contexts it is advisable to pin the file in question and
> operate on a file descriptor instead of the path name. This can be
> emulated in userspace via /proc/self/fd/NN [1] but requires a procfs,
> which might not be mounted e.g. inside of chroots, see[2].
>
> [1]: https://github.com/SELinuxProject/selinux/commit/7e979b56fd2cee28f647376a7233d2ac2d12ca50
> [2]: https://github.com/SELinuxProject/selinux/commit/de285252a1801397306032e070793889c9466845
>
> Original patch by Miklos Szeredi <mszeredi@xxxxxxxxxx>
> https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-6-mszeredi@xxxxxxxxxx/
>
> > While this carries a minute risk of someone relying on the property of
> > xattr syscalls rejecting O_PATH descriptors, it saves the trouble of
> > introducing another set of syscalls.

The bitter irony is that we now want to add another set of syscalls ;-)

https://lore.kernel.org/linux-fsdevel/CAOQ4uxiqG-w8s+zRqk945UtJcE4u0zjPhSs=MSYJ0jMLLjUTFg@xxxxxxxxxxxxxx/

> >
> > Only file->f_path and file->f_inode are accessed in these functions.
> >
> > Current versions return EBADF, hence easy to detect the presense of
> > this feature and fall back in case it's missing.
>
> CC: linux-api@xxxxxxxxxxxxxxx
> CC: linux-man@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>

I think it is important to inspect this with consistency of the UAPI in mind.
What I see is that fchdir(), fcntl(), fstat(), fstatat() already accept O_PATH
so surely they behave the same w.r.t old kernels and EBADF.
Those could all be better documented in their man pages.

w.r.t permission checks, this is no different than what *xattr() variants
already provide.

Therefore, I see no reason to object to this UAPI change.

You may add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>

Thanks,
Amir.

> ---
>  fs/xattr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e8dd03e4561e..16360ac4eb1b 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -656,7 +656,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
>  SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
>                 const void __user *,value, size_t, size, int, flags)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         int error = -EBADF;
>
>         if (!f.file)
> @@ -768,7 +768,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
>  SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
>                 void __user *, value, size_t, size)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         ssize_t error = -EBADF;
>
>         if (!f.file)
> @@ -844,7 +844,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
>
>  SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         ssize_t error = -EBADF;
>
>         if (!f.file)
> @@ -910,7 +910,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
>
>  SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         int error = -EBADF;
>
>         if (!f.file)
> --
> 2.36.1
>