Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner <christian.brauner@ubuntu.com>]

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@gmail.com> wrote:
> >
> > On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> > <christian.brauner@ubuntu.com> 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?

From 7adeec2fe4a954e4e4b8a158a4d9fe705b82b978 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner@ubuntu.com>
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@ubuntu.com>
---
 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