[05/14] umh: Separate the user mode driver and the user mode helper support
diff mbox series

Message ID 87tuyyf0ln.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • Make the user mode driver code a better citizen
Related show

Commit Message

Eric W. Biederman June 26, 2020, 12:55 p.m. UTC
This makes it clear which code is part of the core user mode
helper support and which code is needed to implement user mode
drivers.

This makes the kernel smaller for everyone who does not use a usermode
driver.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/bpfilter.h |   2 +-
 include/linux/umd.h      |  16 +++++
 include/linux/umh.h      |  10 ---
 kernel/Makefile          |   1 +
 kernel/umd.c             | 146 +++++++++++++++++++++++++++++++++++++++
 kernel/umh.c             | 139 -------------------------------------
 6 files changed, 164 insertions(+), 150 deletions(-)
 create mode 100644 include/linux/umd.h
 create mode 100644 kernel/umd.c

Comments

Tetsuo Handa June 26, 2020, 4:22 p.m. UTC | #1
On 2020/06/26 21:55, Eric W. Biederman wrote:
> +static void umd_cleanup(struct subprocess_info *info)
> +{
> +	struct umh_info *umh_info = info->data;
> +
> +	/* cleanup if umh_pipe_setup() was successful but exec failed */

s/umh_pipe_setup/umd_setup/

> +	if (info->retval) {
> +		fput(umh_info->pipe_to_umh);
> +		fput(umh_info->pipe_from_umh);
> +	}
> +}

After this cleanup, I expect adding some protections/isolation which kernel threads
have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's

  It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
  That works and very convenient when it comes to debugging. Compare that to debugging
  a kernel module!

but I think doing it is essential for keeping usermode blob processes as secure/robust
as kernel threads.
Eric W. Biederman June 26, 2020, 4:45 p.m. UTC | #2
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:

> On 2020/06/26 21:55, Eric W. Biederman wrote:
>> +static void umd_cleanup(struct subprocess_info *info)
>> +{
>> +	struct umh_info *umh_info = info->data;
>> +
>> +	/* cleanup if umh_pipe_setup() was successful but exec failed */
>
> s/umh_pipe_setup/umd_setup/

Good catch.  I will fix that when I respin.

>> +	if (info->retval) {
>> +		fput(umh_info->pipe_to_umh);
>> +		fput(umh_info->pipe_from_umh);
>> +	}
>> +}
>
> After this cleanup, I expect adding some protections/isolation which kernel threads
> have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
> SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
> SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's
>
>   It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
>   That works and very convenient when it comes to debugging. Compare that to debugging
>   a kernel module!
>
> but I think doing it is essential for keeping usermode blob processes as secure/robust
> as kernel threads.

Do you have an application for a user mode driver?

I think concerns like that are best addressed in the context of a
specific driver/usecase.  Just to make certain we are solving the right
problems.

My sense is that an advantage of user mode drivers can safely be buggier
than kernel drivers and the freedom to kill them when the drivers go
wrong (knowing the drivers will restart) is important.

Does this series by using the normal path through exec solve your
concerns with LSMs being able to identify these processes (both
individually and as class)?.

Eric
Tetsuo Handa June 27, 2020, 1:26 a.m. UTC | #3
On 2020/06/27 1:45, Eric W. Biederman wrote:
>> After this cleanup, I expect adding some protections/isolation which kernel threads
>> have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
>> SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
>> SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's
>>
>>   It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
>>   That works and very convenient when it comes to debugging. Compare that to debugging
>>   a kernel module!
>>
>> but I think doing it is essential for keeping usermode blob processes as secure/robust
>> as kernel threads.
> 
> Do you have an application for a user mode driver?

No, I'm not a user of this interface.

> 
> I think concerns like that are best addressed in the context of a
> specific driver/usecase.  Just to make certain we are solving the right
> problems.
> 
> My sense is that an advantage of user mode drivers can safely be buggier
> than kernel drivers and the freedom to kill them when the drivers go
> wrong (knowing the drivers will restart) is important.

Right. Segmentation fault in user mode drivers does not cause a kernel oops
is an advantage of user mode drivers. But the freedom to disturb/kill user mode
drivers due to interference like ptrace()/signals from user mode processes,
SIGKILL from OOM-killer/SysRq-i etc. is a big disadvantage of user mode drivers.
I expect that user mode drivers should be killable only when the manager
interface detected that user mode drivers need to be stopped (or restarted).

One of advantages kernel mode drivers have is that their memory is not swapped
out/in. I don't know whether mlockall(MCL_FUTURE) should be automatically applied
to user mode drivers.

> 
> Does this series by using the normal path through exec solve your
> concerns with LSMs being able to identify these processes (both
> individually and as class)?.

I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
at do_open_execat() from do_execveat_common().

I guess "no" for inode based LSMs, for they want a chance to associate
security labels at blob_to_mnt().
Eric W. Biederman June 27, 2020, 4:21 a.m. UTC | #4
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/06/27 1:45, Eric W. Biederman wrote:
>> Does this series by using the normal path through exec solve your
>> concerns with LSMs being able to identify these processes (both
>> individually and as class)?.
>
> I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
> AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
> at do_open_execat() from do_execveat_common().

Is that a problem with the current do_execveat_common in general?

That does not sound like a problem in the user mode driver case as
there are no symlinks involved.

Eric
Tetsuo Handa June 27, 2020, 4:36 a.m. UTC | #5
On 2020/06/27 13:21, Eric W. Biederman wrote:
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
>> On 2020/06/27 1:45, Eric W. Biederman wrote:
>>> Does this series by using the normal path through exec solve your
>>> concerns with LSMs being able to identify these processes (both
>>> individually and as class)?.
>>
>> I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
>> AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
>> at do_open_execat() from do_execveat_common().
> 
> Is that a problem with the current do_execveat_common in general?

In general. Since LSM does not receive parameters needed for obtaining
AT_SYMLINK_NOFOLLOW "struct path" (and it is racy even if parameters were
passed to LSM), I want to obtain both paths in one place.

> 
> That does not sound like a problem in the user mode driver case as
> there are no symlinks involved.

Right.

Patch
diff mbox series

diff --git a/include/linux/bpfilter.h b/include/linux/bpfilter.h
index d815622cd31e..b42e44e29033 100644
--- a/include/linux/bpfilter.h
+++ b/include/linux/bpfilter.h
@@ -3,7 +3,7 @@ 
 #define _LINUX_BPFILTER_H
 
 #include <uapi/linux/bpfilter.h>
-#include <linux/umh.h>
+#include <linux/umd.h>
 
 struct sock;
 int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char __user *optval,
diff --git a/include/linux/umd.h b/include/linux/umd.h
new file mode 100644
index 000000000000..3f8c5743202b
--- /dev/null
+++ b/include/linux/umd.h
@@ -0,0 +1,16 @@ 
+#ifndef __LINUX_UMD_H__
+#define __LINUX_UMD_H__
+
+#include <linux/umh.h>
+
+struct umh_info {
+	const char *cmdline;
+	struct file *pipe_to_umh;
+	struct file *pipe_from_umh;
+	struct list_head list;
+	void (*cleanup)(struct umh_info *info);
+	pid_t pid;
+};
+int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
+
+#endif /* __LINUX_UMD_H__ */
diff --git a/include/linux/umh.h b/include/linux/umh.h
index de08af00c68a..73173c4a07e5 100644
--- a/include/linux/umh.h
+++ b/include/linux/umh.h
@@ -39,16 +39,6 @@  call_usermodehelper_setup(const char *path, char **argv, char **envp,
 			  int (*init)(struct subprocess_info *info, struct cred *new),
 			  void (*cleanup)(struct subprocess_info *), void *data);
 
-struct umh_info {
-	const char *cmdline;
-	struct file *pipe_to_umh;
-	struct file *pipe_from_umh;
-	struct list_head list;
-	void (*cleanup)(struct umh_info *info);
-	pid_t pid;
-};
-int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
-
 extern int
 call_usermodehelper_exec(struct subprocess_info *info, int wait);
 
diff --git a/kernel/Makefile b/kernel/Makefile
index f3218bc5ec69..a81d7354323c 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -12,6 +12,7 @@  obj-y     = fork.o exec_domain.o panic.o \
 	    notifier.o ksysfs.o cred.o reboot.o \
 	    async.o range.o smpboot.o ucount.o
 
+obj-$(CONFIG_BPFILTER) += umd.o
 obj-$(CONFIG_MODULES) += kmod.o
 obj-$(CONFIG_MULTIUSER) += groups.o
 
diff --git a/kernel/umd.c b/kernel/umd.c
new file mode 100644
index 000000000000..8efaa84b6aa1
--- /dev/null
+++ b/kernel/umd.c
@@ -0,0 +1,146 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * umd - User mode driver support
+ */
+#include <linux/shmem_fs.h>
+#include <linux/pipe_fs_i.h>
+#include <linux/umd.h>
+
+static LIST_HEAD(umh_list);
+static DEFINE_MUTEX(umh_list_lock);
+
+static int umd_setup(struct subprocess_info *info, struct cred *new)
+{
+	struct umh_info *umh_info = info->data;
+	struct file *from_umh[2];
+	struct file *to_umh[2];
+	int err;
+
+	/* create pipe to send data to umh */
+	err = create_pipe_files(to_umh, 0);
+	if (err)
+		return err;
+	err = replace_fd(0, to_umh[0], 0);
+	fput(to_umh[0]);
+	if (err < 0) {
+		fput(to_umh[1]);
+		return err;
+	}
+
+	/* create pipe to receive data from umh */
+	err = create_pipe_files(from_umh, 0);
+	if (err) {
+		fput(to_umh[1]);
+		replace_fd(0, NULL, 0);
+		return err;
+	}
+	err = replace_fd(1, from_umh[1], 0);
+	fput(from_umh[1]);
+	if (err < 0) {
+		fput(to_umh[1]);
+		replace_fd(0, NULL, 0);
+		fput(from_umh[0]);
+		return err;
+	}
+
+	umh_info->pipe_to_umh = to_umh[1];
+	umh_info->pipe_from_umh = from_umh[0];
+	umh_info->pid = task_pid_nr(current);
+	current->flags |= PF_UMH;
+	return 0;
+}
+
+static void umd_cleanup(struct subprocess_info *info)
+{
+	struct umh_info *umh_info = info->data;
+
+	/* cleanup if umh_pipe_setup() was successful but exec failed */
+	if (info->retval) {
+		fput(umh_info->pipe_to_umh);
+		fput(umh_info->pipe_from_umh);
+	}
+}
+
+/**
+ * fork_usermode_blob - fork a blob of bytes as a usermode process
+ * @data: a blob of bytes that can be do_execv-ed as a file
+ * @len: length of the blob
+ * @info: information about usermode process (shouldn't be NULL)
+ *
+ * If info->cmdline is set it will be used as command line for the
+ * user process, else "usermodehelper" is used.
+ *
+ * Returns either negative error or zero which indicates success
+ * in executing a blob of bytes as a usermode process. In such
+ * case 'struct umh_info *info' is populated with two pipes
+ * and a pid of the process. The caller is responsible for health
+ * check of the user process, killing it via pid, and closing the
+ * pipes when user process is no longer needed.
+ */
+int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
+{
+	const char *cmdline = (info->cmdline) ? info->cmdline : "usermodehelper";
+	struct subprocess_info *sub_info;
+	char **argv = NULL;
+	struct file *file;
+	ssize_t written;
+	loff_t pos = 0;
+	int err;
+
+	file = shmem_kernel_file_setup("", len, 0);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	written = kernel_write(file, data, len, &pos);
+	if (written != len) {
+		err = written;
+		if (err >= 0)
+			err = -ENOMEM;
+		goto out;
+	}
+
+	err = -ENOMEM;
+	argv = argv_split(GFP_KERNEL, cmdline, NULL);
+	if (!argv)
+		goto out;
+
+	sub_info = call_usermodehelper_setup("none", argv, NULL, GFP_KERNEL,
+					     umd_setup, umd_cleanup, info);
+	if (!sub_info)
+		goto out;
+
+	sub_info->file = file;
+	err = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
+	if (!err) {
+		mutex_lock(&umh_list_lock);
+		list_add(&info->list, &umh_list);
+		mutex_unlock(&umh_list_lock);
+	}
+out:
+	if (argv)
+		argv_free(argv);
+	fput(file);
+	return err;
+}
+EXPORT_SYMBOL_GPL(fork_usermode_blob);
+
+void __exit_umh(struct task_struct *tsk)
+{
+	struct umh_info *info;
+	pid_t pid = tsk->pid;
+
+	mutex_lock(&umh_list_lock);
+	list_for_each_entry(info, &umh_list, list) {
+		if (info->pid == pid) {
+			list_del(&info->list);
+			mutex_unlock(&umh_list_lock);
+			goto out;
+		}
+	}
+	mutex_unlock(&umh_list_lock);
+	return;
+out:
+	if (info->cleanup)
+		info->cleanup(info);
+}
+
diff --git a/kernel/umh.c b/kernel/umh.c
index 14d63b5f29a7..3e4e453d45c8 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -26,8 +26,6 @@ 
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <linux/uaccess.h>
-#include <linux/shmem_fs.h>
-#include <linux/pipe_fs_i.h>
 
 #include <trace/events/module.h>
 
@@ -38,8 +36,6 @@  static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
 static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
 static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
-static LIST_HEAD(umh_list);
-static DEFINE_MUTEX(umh_list_lock);
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
@@ -402,121 +398,6 @@  struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
 }
 EXPORT_SYMBOL(call_usermodehelper_setup);
 
-static int umd_setup(struct subprocess_info *info, struct cred *new)
-{
-	struct umh_info *umh_info = info->data;
-	struct file *from_umh[2];
-	struct file *to_umh[2];
-	int err;
-
-	/* create pipe to send data to umh */
-	err = create_pipe_files(to_umh, 0);
-	if (err)
-		return err;
-	err = replace_fd(0, to_umh[0], 0);
-	fput(to_umh[0]);
-	if (err < 0) {
-		fput(to_umh[1]);
-		return err;
-	}
-
-	/* create pipe to receive data from umh */
-	err = create_pipe_files(from_umh, 0);
-	if (err) {
-		fput(to_umh[1]);
-		replace_fd(0, NULL, 0);
-		return err;
-	}
-	err = replace_fd(1, from_umh[1], 0);
-	fput(from_umh[1]);
-	if (err < 0) {
-		fput(to_umh[1]);
-		replace_fd(0, NULL, 0);
-		fput(from_umh[0]);
-		return err;
-	}
-
-	umh_info->pipe_to_umh = to_umh[1];
-	umh_info->pipe_from_umh = from_umh[0];
-	umh_info->pid = task_pid_nr(current);
-	current->flags |= PF_UMH;
-	return 0;
-}
-
-static void umd_cleanup(struct subprocess_info *info)
-{
-	struct umh_info *umh_info = info->data;
-
-	/* cleanup if umh_pipe_setup() was successful but exec failed */
-	if (info->retval) {
-		fput(umh_info->pipe_to_umh);
-		fput(umh_info->pipe_from_umh);
-	}
-}
-
-/**
- * fork_usermode_blob - fork a blob of bytes as a usermode process
- * @data: a blob of bytes that can be do_execv-ed as a file
- * @len: length of the blob
- * @info: information about usermode process (shouldn't be NULL)
- *
- * If info->cmdline is set it will be used as command line for the
- * user process, else "usermodehelper" is used.
- *
- * Returns either negative error or zero which indicates success
- * in executing a blob of bytes as a usermode process. In such
- * case 'struct umh_info *info' is populated with two pipes
- * and a pid of the process. The caller is responsible for health
- * check of the user process, killing it via pid, and closing the
- * pipes when user process is no longer needed.
- */
-int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
-{
-	const char *cmdline = (info->cmdline) ? info->cmdline : "usermodehelper";
-	struct subprocess_info *sub_info;
-	char **argv = NULL;
-	struct file *file;
-	ssize_t written;
-	loff_t pos = 0;
-	int err;
-
-	file = shmem_kernel_file_setup("", len, 0);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
-	written = kernel_write(file, data, len, &pos);
-	if (written != len) {
-		err = written;
-		if (err >= 0)
-			err = -ENOMEM;
-		goto out;
-	}
-
-	err = -ENOMEM;
-	argv = argv_split(GFP_KERNEL, cmdline, NULL);
-	if (!argv)
-		goto out;
-
-	sub_info = call_usermodehelper_setup("none", argv, NULL, GFP_KERNEL,
-					     umd_setup, umd_cleanup, info);
-	if (!sub_info)
-		goto out;
-
-	sub_info->file = file;
-	err = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
-	if (!err) {
-		mutex_lock(&umh_list_lock);
-		list_add(&info->list, &umh_list);
-		mutex_unlock(&umh_list_lock);
-	}
-out:
-	if (argv)
-		argv_free(argv);
-	fput(file);
-	return err;
-}
-EXPORT_SYMBOL_GPL(fork_usermode_blob);
-
 /**
  * call_usermodehelper_exec - start a usermode application
  * @sub_info: information about the subprocessa
@@ -678,26 +559,6 @@  static int proc_cap_handler(struct ctl_table *table, int write,
 	return 0;
 }
 
-void __exit_umh(struct task_struct *tsk)
-{
-	struct umh_info *info;
-	pid_t pid = tsk->pid;
-
-	mutex_lock(&umh_list_lock);
-	list_for_each_entry(info, &umh_list, list) {
-		if (info->pid == pid) {
-			list_del(&info->list);
-			mutex_unlock(&umh_list_lock);
-			goto out;
-		}
-	}
-	mutex_unlock(&umh_list_lock);
-	return;
-out:
-	if (info->cleanup)
-		info->cleanup(info);
-}
-
 struct ctl_table usermodehelper_table[] = {
 	{
 		.procname	= "bset",