[PATCH] tomoyo: Fix incorrect return value from tomoyo_find_next_domain()

[PATCH] tomoyo: Fix incorrect return value from tomoyo_find_next_domain()

[Takeshi Misawa]

When filename exceeds PATH_MAX,
tomoyo_find_next_domain() retval is not ENAMETOOLONG, but ENOENT.

Fix this by retuen kern_path() error.

Signed-off-by: Takeshi Misawa <jeliantsurux@gmail.com>
---
Dear Tetsuo Handa

I found unexpected return value from TOMOYO and try to create a patch.
If this is not acceptable for security reason, please discard this patch.

Regards.
---
 security/tomoyo/domain.c   | 7 +++++--
 security/tomoyo/realpath.c | 9 +++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 8526a0a74023..3d8034701344 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -723,8 +723,10 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
 	/* Get symlink's pathname of program. */
 	retval = -ENOENT;
 	exename.name = tomoyo_realpath_nofollow(original_name);
-	if (!exename.name)
+	if (IS_ERR(exename.name)) {
+		retval = PTR_ERR(exename.name);
 		goto out;
+	}
 	tomoyo_fill_path_info(&exename);
 retry:
 	/* Check 'aggregator' directive. */
@@ -870,7 +872,8 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
 		s->domain_info = domain;
 		atomic_inc(&domain->users);
 	}
-	kfree(exename.name);
+	if (!IS_ERR(exename.name))
+		kfree(exename.name);
 	if (!retval) {
 		ee->r.domain = domain;
 		retval = tomoyo_environ(ee);
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index e7832448d721..d73e66be05ef 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -332,10 +332,15 @@ char *tomoyo_realpath_from_path(const struct path *path)
 char *tomoyo_realpath_nofollow(const char *pathname)
 {
 	struct path path;
+	char *buf = NULL;
+	int err;
 
-	if (pathname && kern_path(pathname, 0, &path) == 0) {
-		char *buf = tomoyo_realpath_from_path(&path);
+	if (pathname) {
+		err = kern_path(pathname, 0, &path);
+		if (unlikely(err))
+			return ERR_PTR(err);
 
+		buf = tomoyo_realpath_from_path(&path);
 		path_put(&path);
 		return buf;
 	}
-- 
2.17.1

[PATCH] tomoyo: Use error code from kern_path() rather than -ENOENT.

[Tetsuo Handa]

Takeshi Misawa has pointed out that tomoyo_find_next_domain() is returning
-ENOENT when tomoyo_realpath_nofollow() failed [1]. That error code was
chosen based on an assumption that when tomoyo_realpath_nofollow() fails,
the cause of failure is kern_path() failure due to a race window that
the pathname used for do_open_execat() from __do_execve_file() was removed
before tomoyo_find_next_domain() is called.

Since tomoyo_realpath_nofollow() is called by tomoyo_find_next_domain()
only, and __do_execve_file() makes sure that bprm->filename != NULL, let's
inline tomoyo_realpath_nofollow() into tomoyo_find_next_domain().

It seems that tomoyo_realpath_nofollow() is currently broken by
commit 449325b52b7a6208 ("umh: introduce fork_usermode_blob() helper")
when do_execve_file() is used. To fix it, we will need to know whether
do_open_execat() was called before tomoyo_find_next_domain() is called.
To fix it in a more accurate and race-free way, we will need to calculate
both LOOKUP_FOLLOW pathname and !LOOKUP_FOLLOW pathname at the same time.

[1] https://lkml.kernel.org/r/20190801030323.GA1958@DESKTOP

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: Takeshi Misawa <jeliantsurux@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexei Starovoitov <ast@kernel.org>
---
 security/tomoyo/common.h   |  1 -
 security/tomoyo/domain.c   | 18 ++++++++++++++----
 security/tomoyo/realpath.c | 20 --------------------
 3 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index 050473df5809..58b51a21cf9c 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -957,7 +957,6 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
 		      va_list args);
 char *tomoyo_read_token(struct tomoyo_acl_param *param);
 char *tomoyo_realpath_from_path(const struct path *path);
-char *tomoyo_realpath_nofollow(const char *pathname);
 const char *tomoyo_get_exe(void);
 const char *tomoyo_yesno(const unsigned int value);
 const struct tomoyo_path_info *tomoyo_compare_name_union
diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 8526a0a74023..5cd06bfd46c7 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -721,10 +721,20 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
 	ee->r.obj = &ee->obj;
 	ee->obj.path1 = bprm->file->f_path;
 	/* Get symlink's pathname of program. */
-	retval = -ENOENT;
-	exename.name = tomoyo_realpath_nofollow(original_name);
-	if (!exename.name)
-		goto out;
+	{
+		struct path path;
+		int ret = kern_path(original_name, 0, &path);
+
+		if (ret) {
+			retval = ret;
+			exename.name = NULL;
+			goto out;
+		}
+		exename.name = tomoyo_realpath_from_path(&path);
+		path_put(&path);
+		if (!exename.name) /* retval was initialized with -ENONEM */
+			goto out;
+	}
 	tomoyo_fill_path_info(&exename);
 retry:
 	/* Check 'aggregator' directive. */
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index e7832448d721..70d456348e1c 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -321,23 +321,3 @@ char *tomoyo_realpath_from_path(const struct path *path)
 		tomoyo_warn_oom(__func__);
 	return name;
 }
-
-/**
- * tomoyo_realpath_nofollow - Get realpath of a pathname.
- *
- * @pathname: The pathname to solve.
- *
- * Returns the realpath of @pathname on success, NULL otherwise.
- */
-char *tomoyo_realpath_nofollow(const char *pathname)
-{
-	struct path path;
-
-	if (pathname && kern_path(pathname, 0, &path) == 0) {
-		char *buf = tomoyo_realpath_from_path(&path);
-
-		path_put(&path);
-		return buf;
-	}
-	return NULL;
-}
-- 
2.16.5