On Thu, Apr 15, 2021 at 12:08:20PM +0200, Christian Brauner wrote:
> On Thu, Apr 15, 2021 at 02:14:17PM +0700, Dmitry Kadashev wrote:
> > On Thu, Apr 8, 2021 at 3:45 PM Dmitry Kadashev <dkadashev@xxxxxxxxx> wrote:
> > >
> > > On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> > > <christian.brauner@xxxxxxxxxx> wrote:
> > > > The only thing that is a bit unpleasant here is that this change
> > > > breaks the consistency between the creation helpers:
> > > >
> > > > do_mkdirat()
> > > > do_symlinkat()
> > > > do_linkat()
> > > > do_mknodat()
> > > >
> > > > All but of them currently take
> > > > const char __user *pathname
> > > > and call
> > > > user_path_create()
> > > > with that do_mkdirat() change that's no longer true. One of the major
> > > > benefits over the recent years in this code is naming and type consistency.
> > > > And since it's just matter of time until io_uring will also gain support
> > > > for do_{symlinkat,linkat,mknodat} I would think switching all of them to
> > > > take a struct filename
> > > > and then have all do_* helpers call getname() might just be nicer in the
> > > > long run.
> > >
> > > So, I've finally got some time to look into this. do_mknodat() and
> > > do_symlinkat() are easy. But do_linkat() is more complicated, I could use some
> > > hints as to what's the reasonable way to implement the change.
> > >
> > > The problem is linkat() requires CAP_DAC_READ_SEARCH capability if AT_EMPTY_PATH
> > > flag is passed. Right now do_linkat checks the capability before calling
> > > getname_flags (essentially). If do_linkat is changed to accept struct filename
> > > then there is no bulletproof way to force CAP_DAC_READ_SEARCH presence (e.g. if
> > > for whatever reason AT_EMPTY_PATH is not in flags passed to do_linkat). Also, it
> > > means that the caller is responsible to process AT_EMPTY_PATH in the first
> > > place, which means logic duplication.
> > >
> > > Any ideas what's the best way to approach this?
> >
> > Ping. If someone can see how we can avoid making do_linkat() callers
> > ensure the process has CAP_DAC_READ_SEARCH capability if AT_EMPTY_PATH
> > was passed then the hints would be really appreciated.
>
> Would something like this help?
Because I always forget this:
_Completely_ untested.
(And sorry for the late reply. Just a lot of mail.)
>
> From 7adeec2fe4a954e4e4b8a158a4d9fe705b82b978 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <christian.brauner@xxxxxxxxxx>
> Date: Thu, 15 Apr 2021 12:03:42 +0200
> Subject: [PATCH] namei: add getname_uflags()
>
> There are a couple of places where we already open-code the (flags &
> AT_EMPTY_PATH) check and io_uring will likely add another one in the future.
> Let's just add a simple helper getname_uflags() that handles this directly and
> use it.
> getname_flags() itself doesn't need access to lookup flags other than
> LOOKUP_EMPTY so this is basically just a boolean already so be honest about it.
>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
> fs/exec.c | 10 ++--------
> fs/fsopen.c | 6 +++---
> fs/namei.c | 6 +++---
> include/linux/fs.h | 4 ++++
> 4 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 18594f11c31f..53c633f69f4a 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -2069,10 +2069,7 @@ SYSCALL_DEFINE5(execveat,
> const char __user *const __user *, envp,
> int, flags)
> {
> - int lookup_flags = (flags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0;
> -
> - return do_execveat(fd,
> - getname_flags(filename, lookup_flags, NULL),
> + return do_execveat(fd, getname_uflags(filename, flags),
> argv, envp, flags);
> }
>
> @@ -2090,10 +2087,7 @@ COMPAT_SYSCALL_DEFINE5(execveat, int, fd,
> const compat_uptr_t __user *, envp,
> int, flags)
> {
> - int lookup_flags = (flags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0;
> -
> - return compat_do_execveat(fd,
> - getname_flags(filename, lookup_flags, NULL),
> + return compat_do_execveat(fd, getname_uflags(filename, flags),
> argv, envp, flags);
> }
> #endif
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index 27a890aa493a..00906abaf466 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -321,7 +321,7 @@ SYSCALL_DEFINE5(fsconfig,
> struct fs_context *fc;
> struct fd f;
> int ret;
> - int lookup_flags = 0;
> + bool lookup_empty = false;
>
> struct fs_parameter param = {
> .type = fs_value_is_undefined,
> @@ -411,11 +411,11 @@ SYSCALL_DEFINE5(fsconfig,
> }
> break;
> case FSCONFIG_SET_PATH_EMPTY:
> - lookup_flags = LOOKUP_EMPTY;
> + lookup_empty = true;
> fallthrough;
> case FSCONFIG_SET_PATH:
> param.type = fs_value_is_filename;
> - param.name = getname_flags(_value, lookup_flags, NULL);
> + param.name = getname_flags(_value, lookup_empty, NULL);
> if (IS_ERR(param.name)) {
> ret = PTR_ERR(param.name);
> goto out_key;
> diff --git a/fs/namei.c b/fs/namei.c
> index 216f16e74351..7694f6bcd711 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -125,7 +125,7 @@
> #define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname))
>
> struct filename *
> -getname_flags(const char __user *filename, int flags, int *empty)
> +getname_flags(const char __user *filename, bool lookup_empty, int *empty)
> {
> struct filename *result;
> char *kname;
> @@ -191,7 +191,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
> if (unlikely(!len)) {
> if (empty)
> *empty = 1;
> - if (!(flags & LOOKUP_EMPTY)) {
> + if (lookup_empty) {
> putname(result);
> return ERR_PTR(-ENOENT);
> }
> @@ -206,7 +206,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
> struct filename *
> getname(const char __user * filename)
> {
> - return getname_flags(filename, 0, NULL);
> + return getname_flags(filename, false, NULL);
> }
>
> struct filename *
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ec8f3ddf4a6a..6dbd629ece04 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2644,6 +2644,10 @@ static inline struct file *file_clone_open(struct file *file)
> extern int filp_close(struct file *, fl_owner_t id);
>
> extern struct filename *getname_flags(const char __user *, int, int *);
> +extern struct filename *getname_uflags(const char __user *filename, int uflags)
> +{
> + return getname_flags(filename, (flags & AT_EMPTY_PATH), NULL);
> +}
> extern struct filename *getname(const char __user *);
> extern struct filename *getname_kernel(const char *);
> extern void putname(struct filename *name);
> --
> 2.27.0
>
