* [PATCH v3 05/16] umh: Separate the user mode driver and the user mode helper support
From: Eric W. Biederman @ 2020-07-02 16:41 UTC (permalink / raw)
To: linux-kernel
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds,
Christian Brauner, Eric W. Biederman, Greg Kroah-Hartman
In-Reply-To: <87y2o1swee.fsf_-_@x220.int.ebiederm.org>
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.
v1: https://lkml.kernel.org/r/87tuyyf0ln.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87imf963s6.fsf_-_@x220.int.ebiederm.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
include/linux/bpfilter.h | 2 +-
include/linux/sched.h | 8 --
include/linux/umh.h | 10 ---
include/linux/usermode_driver.h | 30 +++++++
kernel/Makefile | 1 +
kernel/exit.c | 1 +
kernel/umh.c | 139 ------------------------------
kernel/usermode_driver.c | 146 ++++++++++++++++++++++++++++++++
8 files changed, 179 insertions(+), 158 deletions(-)
create mode 100644 include/linux/usermode_driver.h
create mode 100644 kernel/usermode_driver.c
diff --git a/include/linux/bpfilter.h b/include/linux/bpfilter.h
index d815622cd31e..d6d6206052a6 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/usermode_driver.h>
struct sock;
int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char __user *optval,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index b62e6aaf28f0..59d1e92bb88e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2020,14 +2020,6 @@ static inline void rseq_execve(struct task_struct *t)
#endif
-void __exit_umh(struct task_struct *tsk);
-
-static inline void exit_umh(struct task_struct *tsk)
-{
- if (unlikely(tsk->flags & PF_UMH))
- __exit_umh(tsk);
-}
-
#ifdef CONFIG_DEBUG_RSEQ
void rseq_syscall(struct pt_regs *regs);
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/include/linux/usermode_driver.h b/include/linux/usermode_driver.h
new file mode 100644
index 000000000000..c5f6dc950227
--- /dev/null
+++ b/include/linux/usermode_driver.h
@@ -0,0 +1,30 @@
+#ifndef __LINUX_USERMODE_DRIVER_H__
+#define __LINUX_USERMODE_DRIVER_H__
+
+#include <linux/umh.h>
+
+#ifdef CONFIG_BPFILTER
+void __exit_umh(struct task_struct *tsk);
+
+static inline void exit_umh(struct task_struct *tsk)
+{
+ if (unlikely(tsk->flags & PF_UMH))
+ __exit_umh(tsk);
+}
+#else
+static inline void exit_umh(struct task_struct *tsk)
+{
+}
+#endif
+
+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_USERMODE_DRIVER_H__ */
diff --git a/kernel/Makefile b/kernel/Makefile
index f3218bc5ec69..43928759893a 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) += usermode_driver.o
obj-$(CONFIG_MODULES) += kmod.o
obj-$(CONFIG_MULTIUSER) += groups.o
diff --git a/kernel/exit.c b/kernel/exit.c
index 727150f28103..a081deea52ca 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -63,6 +63,7 @@
#include <linux/random.h>
#include <linux/rcuwait.h>
#include <linux/compat.h>
+#include <linux/usermode_driver.h>
#include <linux/uaccess.h>
#include <asm/unistd.h>
diff --git a/kernel/umh.c b/kernel/umh.c
index b8fa9b99b366..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_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",
diff --git a/kernel/usermode_driver.c b/kernel/usermode_driver.c
new file mode 100644
index 000000000000..5b05863af855
--- /dev/null
+++ b/kernel/usermode_driver.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/usermode_driver.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_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);
+}
+
--
2.25.0
^ permalink raw reply related
* [PATCH v3 03/16] umh: Rename the user mode driver helpers for clarity
From: Eric W. Biederman @ 2020-07-02 16:41 UTC (permalink / raw)
To: linux-kernel
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds,
Christian Brauner, Eric W. Biederman, Greg Kroah-Hartman
In-Reply-To: <87y2o1swee.fsf_-_@x220.int.ebiederm.org>
Now that the functionality of umh_setup_pipe and
umh_clean_and_save_pid has changed their names are too specific and
don't make much sense. Instead name them umd_setup and umd_cleanup
for the functional role in setting up user mode drivers.
v1: https://lkml.kernel.org/r/875zbegf82.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87tuyt63x3.fsf_-_@x220.int.ebiederm.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
kernel/umh.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/umh.c b/kernel/umh.c
index e6b9d6636850..26c3d493f168 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -429,7 +429,7 @@ struct subprocess_info *call_usermodehelper_setup_file(struct file *file,
return sub_info;
}
-static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
+static int umd_setup(struct subprocess_info *info, struct cred *new)
{
struct umh_info *umh_info = info->data;
struct file *from_umh[2];
@@ -470,11 +470,11 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
return 0;
}
-static void umh_clean_and_save_pid(struct subprocess_info *info)
+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 */
+ /* cleanup if umh_setup() was successful but exec failed */
if (info->retval) {
fput(umh_info->pipe_to_umh);
fput(umh_info->pipe_from_umh);
@@ -520,8 +520,8 @@ int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
}
err = -ENOMEM;
- sub_info = call_usermodehelper_setup_file(file, umh_pipe_setup,
- umh_clean_and_save_pid, info);
+ sub_info = call_usermodehelper_setup_file(file, umd_setup, umd_cleanup,
+ info);
if (!sub_info)
goto out;
--
2.25.0
^ permalink raw reply related
* [PATCH v3 02/16] umh: Move setting PF_UMH into umh_pipe_setup
From: Eric W. Biederman @ 2020-07-02 16:41 UTC (permalink / raw)
To: linux-kernel
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds,
Christian Brauner, Eric W. Biederman, Greg Kroah-Hartman
In-Reply-To: <87y2o1swee.fsf_-_@x220.int.ebiederm.org>
I am separating the code specific to user mode drivers from the code
for ordinary user space helpers. Move setting of PF_UMH from
call_usermodehelper_exec_async which is core user mode helper code
into umh_pipe_setup which is user mode driver code.
The code is equally as easy to write in one location as the other and
the movement minimizes the impact of the user mode driver code on the
core of the user mode helper code.
Setting PF_UMH unconditionally is harmless as an action will only
happen if it is paired with an entry on umh_list.
v1: https://lkml.kernel.org/r/87bll6gf8t.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87zh8l63xs.fsf_-_@x220.int.ebiederm.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
kernel/umh.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/umh.c b/kernel/umh.c
index c2a582b3a2bf..e6b9d6636850 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -102,12 +102,10 @@ static int call_usermodehelper_exec_async(void *data)
commit_creds(new);
- if (sub_info->file) {
+ if (sub_info->file)
retval = do_execve_file(sub_info->file,
sub_info->argv, sub_info->envp);
- if (!retval)
- current->flags |= PF_UMH;
- } else
+ else
retval = do_execve(getname_kernel(sub_info->path),
(const char __user *const __user *)sub_info->argv,
(const char __user *const __user *)sub_info->envp);
@@ -468,6 +466,7 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
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;
}
--
2.25.0
^ permalink raw reply related
* [PATCH v3 01/16] umh: Capture the pid in umh_pipe_setup
From: Eric W. Biederman @ 2020-07-02 16:41 UTC (permalink / raw)
To: linux-kernel
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds,
Christian Brauner, Eric W. Biederman, Greg Kroah-Hartman
In-Reply-To: <87y2o1swee.fsf_-_@x220.int.ebiederm.org>
The pid in struct subprocess_info is only used by umh_clean_and_save_pid to
write the pid into umh_info.
Instead always capture the pid on struct umh_info in umh_pipe_setup, removing
code that is specific to user mode drivers from the common user path of
user mode helpers.
v1: https://lkml.kernel.org/r/87h7uygf9i.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/875zb97iix.fsf_-_@x220.int.ebiederm.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
include/linux/umh.h | 1 -
kernel/umh.c | 5 ++---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/umh.h b/include/linux/umh.h
index 0c08de356d0d..aae16a0ebd0f 100644
--- a/include/linux/umh.h
+++ b/include/linux/umh.h
@@ -25,7 +25,6 @@ struct subprocess_info {
struct file *file;
int wait;
int retval;
- pid_t pid;
int (*init)(struct subprocess_info *info, struct cred *new);
void (*cleanup)(struct subprocess_info *info);
void *data;
diff --git a/kernel/umh.c b/kernel/umh.c
index 79f139a7ca03..c2a582b3a2bf 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -102,7 +102,6 @@ static int call_usermodehelper_exec_async(void *data)
commit_creds(new);
- sub_info->pid = task_pid_nr(current);
if (sub_info->file) {
retval = do_execve_file(sub_info->file,
sub_info->argv, sub_info->envp);
@@ -468,6 +467,7 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
umh_info->pipe_to_umh = to_umh[1];
umh_info->pipe_from_umh = from_umh[0];
+ umh_info->pid = task_pid_nr(current);
return 0;
}
@@ -476,13 +476,12 @@ static void umh_clean_and_save_pid(struct subprocess_info *info)
struct umh_info *umh_info = info->data;
/* cleanup if umh_pipe_setup() was successful but exec failed */
- if (info->pid && info->retval) {
+ if (info->retval) {
fput(umh_info->pipe_to_umh);
fput(umh_info->pipe_from_umh);
}
argv_free(info->argv);
- umh_info->pid = info->pid;
}
/**
--
2.25.0
^ permalink raw reply related
* [PATCH v3 00/16] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-07-02 16:40 UTC (permalink / raw)
To: linux-kernel
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds,
Christian Brauner
In-Reply-To: <87bll17ili.fsf_-_@x220.int.ebiederm.org>
This is the third round of my changeset to split the user mode driver
code from the user mode helper code, and to make the code use common
facilities to get things done instead of recreating them just
for the user mode driver code.
I have split the changes into small enough pieces so they should be
easily readable and testable.
The changes lean into the preexisting interfaces in the kernel and
remove special cases for user mode driver code in favor of solutions
that don't need special cases. This results in smaller code with fewer
bugs.
At a practical level this removes the maintenance burden of the user
mode drivers from the user mode helper code and from exec as the special
cases are removed.
Similarly the LSM interaction bugs are fixed by not having unnecessary
special cases for user mode drivers.
I have tested thes changes by booting with the code compiled in and
by killing "bpfilter_umh" and "running iptables -vnL" to restart
the userspace driver, also by running "while true; do iptables -L;rmmod
bpfilter; done" to verify the module load and unload work properly.
I have compiled tested each change with and without CONFIG_BPFILTER
enabled.
From v2 to v3 I have made two siginficant changes.
- I factored thread_group_exit out of pidfd_poll to allow the test
to be used by the bpfilter code.
- I renamed umd.c and umd.h to usermode_driver.c and usermode_driver.h
respectively.
I made a few very small changes from v1 to v2:
- Updated the function name in a comment when the function is renamed
- Moved some more code so that the the !CONFIG_BPFILTER case continues
to compile when I moved the code into umd.c
- A fix for the module loading case to really flush the file descriptor.
- Removed split_argv entirely from fork_usermode_driver.
There was nothing to split so it was just confusing.
Please let me know if you see any bugs. Once the code review is
finished I plan to place the code in a non-rebasing branch
so I can pull it into my tree and so it can also be pulled into
the bpf-next tree.
v1: https://lkml.kernel.org/r/87pn9mgfc2.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87bll17ili.fsf_-_@x220.int.ebiederm.org
Eric W. Biederman (16):
umh: Capture the pid in umh_pipe_setup
umh: Move setting PF_UMH into umh_pipe_setup
umh: Rename the user mode driver helpers for clarity
umh: Remove call_usermodehelper_setup_file.
umh: Separate the user mode driver and the user mode helper support
umd: For clarity rename umh_info umd_info
umd: Rename umd_info.cmdline umd_info.driver_name
umd: Transform fork_usermode_blob into fork_usermode_driver
umh: Stop calling do_execve_file
exec: Remove do_execve_file
bpfilter: Move bpfilter_umh back into init data
umd: Track user space drivers with struct pid
exit: Factor thread_group_exited out of pidfd_poll
bpfilter: Take advantage of the facilities of struct pid
umd: Remove exit_umh
umd: Stop using split_argv
fs/exec.c | 38 ++------
include/linux/binfmts.h | 1 -
include/linux/bpfilter.h | 7 +-
include/linux/sched.h | 9 --
include/linux/sched/signal.h | 2 +
include/linux/umh.h | 15 ----
include/linux/usermode_driver.h | 18 ++++
kernel/Makefile | 1 +
kernel/exit.c | 25 +++++-
kernel/fork.c | 6 +-
kernel/umh.c | 171 +-----------------------------------
kernel/usermode_driver.c | 182 +++++++++++++++++++++++++++++++++++++++
net/bpfilter/bpfilter_kern.c | 38 ++++----
net/bpfilter/bpfilter_umh_blob.S | 2 +-
net/ipv4/bpfilter/sockopt.c | 20 +++--
15 files changed, 275 insertions(+), 260 deletions(-)
Eric W. Biederman (15):
umh: Capture the pid in umh_pipe_setup
umh: Move setting PF_UMH into umh_pipe_setup
umh: Rename the user mode driver helpers for clarity
umh: Remove call_usermodehelper_setup_file.
umh: Separate the user mode driver and the user mode helper support
umd: For clarity rename umh_info umd_info
umd: Rename umd_info.cmdline umd_info.driver_name
umd: Transform fork_usermode_blob into fork_usermode_driver
umh: Stop calling do_execve_file
exec: Remove do_execve_file
bpfilter: Move bpfilter_umh back into init data
umd: Track user space drivers with struct pid
bpfilter: Take advantage of the facilities of struct pid
umd: Remove exit_umh
umd: Stop using split_argv
fs/exec.c | 38 ++------
include/linux/binfmts.h | 1 -
include/linux/bpfilter.h | 7 +-
include/linux/sched.h | 9 --
include/linux/umd.h | 18 ++++
include/linux/umh.h | 15 ----
kernel/Makefile | 1 +
kernel/exit.c | 1 -
kernel/umd.c | 182 +++++++++++++++++++++++++++++++++++++++
kernel/umh.c | 171 +-----------------------------------
net/bpfilter/bpfilter_kern.c | 38 ++++----
net/bpfilter/bpfilter_umh_blob.S | 2 +-
net/ipv4/bpfilter/sockopt.c | 20 +++--
13 files changed, 248 insertions(+), 255 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-07-02 16:02 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Alexei Starovoitov, linux-kernel, David Miller,
Greg Kroah-Hartman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds
In-Reply-To: <757f37f8-5641-91d2-be80-a96ebc74cacb@i-love.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/07/02 22:08, Eric W. Biederman wrote:
>> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
>>
>>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>>> Hmm. The wake up happens just of tgid->wait_pidfd happens just before
>>>> release_task is called so there is a race. As it is possible to wake
>>>> up and then go back to sleep before pid_has_task becomes false.
>>>
>>> What is the reason we want to wait until pid_has_task() becomes false?
>>>
>>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
>>
>> So that it is safe to call bpfilter_umh_cleanup. The previous code
>> performed the wait by having a callback in do_exit.
>
> But bpfilter_umh_cleanup() does only
>
> fput(info->pipe_to_umh);
> fput(info->pipe_from_umh);
> put_pid(info->tgid);
> info->tgid = NULL;
>
> which is (I think) already safe regardless of the usermode process because
> bpfilter_umh_cleanup() merely closes one side of two pipes used between
> two processes and forgets about the usermode process.
It is not safe.
Baring bugs there is only one use of shtudown_umh that matters. The one
in fini_umh. The use of the file by the mm must be finished before
umd_unload_blob. AKA unmount. Which completely frees the filesystem.
>> It might be possible to call bpf_umh_cleanup early but I have not done
>> that analysis.
>>
>> To perform the test correctly what I have right now is:
>
> Waiting for the termination of a SIGKILLed usermode process is not
> such simple.
The waiting is that simple.
You are correct it might not be a quick process.
A good general principle is to start with something simple and correct
for what it does, and then to make it more complicated when real world
cases show up, and it can be understood what the real challenges are.
I am not going to merge known broken code but I am also not going to
overcomplicate it.
Dealing with very rare and pathological cases that are not handled or
considered today is out of scope for my patchset.
Eric
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Tetsuo Handa @ 2020-07-02 13:40 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexei Starovoitov, linux-kernel, David Miller,
Greg Kroah-Hartman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds
In-Reply-To: <87pn9euks9.fsf@x220.int.ebiederm.org>
On 2020/07/02 22:08, Eric W. Biederman wrote:
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
>
>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>> Hmm. The wake up happens just of tgid->wait_pidfd happens just before
>>> release_task is called so there is a race. As it is possible to wake
>>> up and then go back to sleep before pid_has_task becomes false.
>>
>> What is the reason we want to wait until pid_has_task() becomes false?
>>
>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
>
> So that it is safe to call bpfilter_umh_cleanup. The previous code
> performed the wait by having a callback in do_exit.
But bpfilter_umh_cleanup() does only
fput(info->pipe_to_umh);
fput(info->pipe_from_umh);
put_pid(info->tgid);
info->tgid = NULL;
which is (I think) already safe regardless of the usermode process because
bpfilter_umh_cleanup() merely closes one side of two pipes used between
two processes and forgets about the usermode process.
>
> It might be possible to call bpf_umh_cleanup early but I have not done
> that analysis.
>
> To perform the test correctly what I have right now is:
Waiting for the termination of a SIGKILLed usermode process is not
such simple. If a usermode process was killed by the OOM killer, it
might take minutes for the killed process to reach do_exit() due to
invisible memory allocation dependency chain. Since the OOM killer
kicks the OOM reaper, and the OOM reaper forgets about the killed
process after one second if mmap_sem could not be held (in order to
avoid OOM deadlock), the OOM situation will be eventually solved; but
there is no guarantee that the killed process can reach do_exit()
in a short period.
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-07-02 13:08 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Alexei Starovoitov, linux-kernel, David Miller,
Greg Kroah-Hartman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain, Linus Torvalds
In-Reply-To: <1f4d8b7e-bcff-f950-7dac-76e3c4a65661@i-love.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/06/30 21:29, Eric W. Biederman wrote:
>> Hmm. The wake up happens just of tgid->wait_pidfd happens just before
>> release_task is called so there is a race. As it is possible to wake
>> up and then go back to sleep before pid_has_task becomes false.
>
> What is the reason we want to wait until pid_has_task() becomes false?
>
> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
So that it is safe to call bpfilter_umh_cleanup. The previous code
performed the wait by having a callback in do_exit.
It might be possible to call bpf_umh_cleanup early but I have not done
that analysis.
To perform the test correctly what I have right now is:
bool thread_group_exited(struct pid *pid)
{
struct task_struct *tsk;
bool exited;
rcu_read_lock();
tsk = pid_task(pid, PIDTYPE_PID);
exited = !tsk || (READ_ONCE(tsk->exit_state) && thread_group_empty(tsk));
rcu_read_unlock();
return exited;
}
Which is factored out of pidfd_poll. Which means that this won't be
something that the bpfilter code has to maintain. That seems to be a
fundamentally good facility to have regardless of bpfilter.
I will post the whole thing in a bit once I have a chance to dot my i's
and cross my t's.
> By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
> that use of flush_delayed_fput() has to be careful. Al, is it safe to call
> flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
> called from both kernel thread and from process context (e.g. init_module()
> syscall by /sbin/insmod )) ?
And __fput_sync needs to be even more careful.
umd_load_blob is called in these changes without any locks held.
We fundamentally AKA in any correct version of this code need to flush
the file descriptor before we call exec or exec can not open it a
read-only denying all writes from any other opens.
The use case of flush_delayed_fput is exactly the same as that used
when loading the initramfs.
Eric
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Tetsuo Handa @ 2020-07-02 4:26 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christian Borntraeger, Christoph Hellwig, Eric W. Biederman, ast,
axboe, bfields, bridge, chainsaw, christian.brauner, chuck.lever,
davem, dhowells, gregkh, jarkko.sakkinen, jmorris, josh, keescook,
keyrings, kuba, lars.ellenberg, linux-fsdevel, linux-kernel,
linux-nfs, linux-security-module, nikolay, philipp.reisner,
ravenexp, roopa, serge, slyfox, viro, yangtiezhu, netdev,
markward, linux-s390
In-Reply-To: <20200701153859.GT4332@42.do-not-panic.com>
On 2020/07/02 0:38, Luis Chamberlain wrote:
> @@ -156,6 +156,18 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
> */
> if (KWIFEXITED(ret))
> sub_info->retval = KWEXITSTATUS(ret);
> + /*
> + * Do we really want to be passing the signal, or do we pass
> + * a single error code for all cases?
> + */
> + else if (KWIFSIGNALED(ret))
> + sub_info->retval = KWTERMSIG(ret);
No, this is bad. Caller of usermode helper is unable to distinguish exit(9)
and e.g. SIGKILL'ed by the OOM-killer. Please pass raw exit status value.
I feel that caller of usermode helper should not use exit status value.
For example, call_sbin_request_key() is checking
test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags) || key_validate(key) < 0
condition (if usermode helper was invoked) in order to "ignore any errors from
userspace if the key was instantiated".
> + /* Same here */
> + else if (KWIFSTOPPED((ret)))
> + sub_info->retval = KWSTOPSIG(ret);
> + /* And are we really sure we want this? */
> + else if (KWIFCONTINUED((ret)))
> + sub_info->retval = 0;
> }
>
> /* Restore default kernel sig handler */
>
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-07-02 3:59 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, linux-kernel, linux-security-module,
Jethro Beekman, Haitao Huang, Chunyang Hui, Jordan Hand,
Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, bp, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200617220844.57423-12-jarkko.sakkinen@linux.intel.com>
On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_validate_secs(const struct sgx_secs *secs,
> + unsigned long ssaframesize)
> +{
> + if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> + return -EINVAL;
> +
> + if (secs->base & (secs->size - 1))
> + return -EINVAL;
> +
> + if (secs->miscselect & sgx_misc_reserved_mask ||
> + secs->attributes & sgx_attributes_reserved_mask ||
> + secs->xfrm & sgx_xfrm_reserved_mask)
> + return -EINVAL;
> +
> + if (secs->attributes & SGX_ATTR_MODE64BIT) {
> + if (secs->size > sgx_encl_size_max_64)
> + return -EINVAL;
> + } else if (secs->size > sgx_encl_size_max_32)
> + return -EINVAL;
These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.
Internal versions use more obvious pseudocode, e.g.:
if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
(DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
{
#GP(0);
}
> +
> + if (!(secs->xfrm & XFEATURE_MASK_FP) ||
> + !(secs->xfrm & XFEATURE_MASK_SSE) ||
> + (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
> + ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
> + return -EINVAL;
> +
> + if (!secs->ssa_frame_size || ssaframesize > secs->ssa_frame_size)
> + return -EINVAL;
> +
> + if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
> + memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
> + memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
> + memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
> + return -EINVAL;
> +
> + return 0;
> +}
^ permalink raw reply
* [v2 PATCH] crypto: af_alg - Fix regression on empty requests
From: Herbert Xu @ 2020-07-02 3:32 UTC (permalink / raw)
To: Naresh Kamboju
Cc: Eric Biggers, Luis Chamberlain, LTP List, open list,
linux-security-module, keyrings, lkft-triage,
Linux Crypto Mailing List, Jan Stancek, chrubis, Serge E. Hallyn,
James Morris, Jarkko Sakkinen, David Howells, David S. Miller,
Sachin Sant, Linux Next Mailing List, linuxppc-dev, linux- stable
In-Reply-To: <CA+G9fYutuU55iL_6Qrk3oG3iq-37PaxvtA4KnEQHuLH9YpH-QA@mail.gmail.com>
On Tue, Jun 30, 2020 at 02:18:11PM +0530, Naresh Kamboju wrote:
>
> Since we are on this subject,
> LTP af_alg02 test case fails on stable 4.9 and stable 4.4
> This is not a regression because the test case has been failing from
> the beginning.
>
> Is this test case expected to fail on stable 4.9 and 4.4 ?
> or any chance to fix this on these older branches ?
>
> Test output:
> af_alg02.c:52: BROK: Timed out while reading from request socket.
>
> ref:
> https://qa-reports.linaro.org/lkft/linux-stable-rc-4.9-oe/build/v4.9.228-191-g082e807235d7/testrun/2884917/suite/ltp-crypto-tests/test/af_alg02/history/
> https://qa-reports.linaro.org/lkft/linux-stable-rc-4.9-oe/build/v4.9.228-191-g082e807235d7/testrun/2884606/suite/ltp-crypto-tests/test/af_alg02/log
Actually this test really is broken. Even though empty requests
are legal, they should never be done with no write(2) at all.
Because this fundamentally breaks the use of a blocking read(2)
to wait for more data.
Granted this has been broken since 2017 but I'm not going to
reintroduce this just because of a broken test case.
So please either remove af_alg02 or fix it by adding a control
message through sendmsg(2).
Thanks,
---8<---
Some user-space programs rely on crypto requests that have no
control metadata. This broke when a check was added to require
the presence of control metadata with the ctx->init flag.
This patch fixes the regression by setting ctx->init as long as
one sendmsg(2) has been made, with or without a control message.
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 9fcb91ea10c41..5882ed46f1adb 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -851,6 +851,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
err = -EINVAL;
goto unlock;
}
+ ctx->init = true;
if (init) {
ctx->enc = enc;
@@ -858,7 +859,6 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
memcpy(ctx->iv, con.iv->iv, ivsize);
ctx->aead_assoclen = con.aead_assoclen;
- ctx->init = true;
}
while (size) {
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [fs] 140402bab8: stress-ng.splice.ops_per_sec -100.0% regression
From: Linus Torvalds @ 2020-07-01 20:32 UTC (permalink / raw)
To: Christoph Hellwig
Cc: kernel test robot, Al Viro, Ian Kent, David Howells,
Linux Kernel Mailing List, linux-fsdevel, LSM List, NetFilter,
lkp
In-Reply-To: <20200701121344.GA14149@lst.de>
On Wed, Jul 1, 2020 at 5:13 AM Christoph Hellwig <hch@lst.de> wrote:
>
> FYI, this is because stress-nh tests splice using /dev/null. Which
> happens to actually have the iter ops, but doesn't have explicit
> splice_read operation.
Heh. I guess a splice op for /dev/null should be fairly trivial to implement..
Linus
^ permalink raw reply
* Re: [GIT PULL] Security subsystem fixes for v5.8
From: pr-tracker-bot @ 2020-07-01 19:10 UTC (permalink / raw)
To: James Morris; +Cc: Linus Torvalds, linux-security-module, linux-kernel
In-Reply-To: <alpine.LRH.2.21.2006300659080.10756@namei.org>
The pull request you sent on Tue, 30 Jun 2020 07:00:03 +1000 (AEST):
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git tags/fixes-v5.8-rc3-a
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/615bc218d628d90a3afebcfa772aa41865acd301
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v2 05/15] umh: Separate the user mode driver and the user mode helper support
From: Alexei Starovoitov @ 2020-07-01 17:42 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, Linux Kernel Mailing List, David Miller,
Greg Kroah-Hartman, Tetsuo Handa, Kees Cook, Andrew Morton,
Alexei Starovoitov, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
LSM List, Casey Schaufler, Luis Chamberlain
In-Reply-To: <87ftabw3v5.fsf@x220.int.ebiederm.org>
On Wed, Jul 1, 2020 at 10:23 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Mon, Jun 29, 2020 at 1:05 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> 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.
> >>
> >> kernel/umd.c | 146 +++++++++++++++++++++++++++++++++++++++
> >> kernel/umh.c | 139 -------------------------------------
> >
> > I certainly don't object to the split, but I hate the name.
> >
> > We have uml, umd and umh for user mode {linux, drivers, helper}
> > respectively.And honestly, I don't see the point in using an obscure
> > and unreadable TLA for something like this.
> >
> > I really don't think it would hurt to write out even the full name
> > with "usermode_driver.c" or something like that, would it?
> >
> > Then "umd" could be continued to be used as a prefix for the helper
> > functions, by all means, but if we startv renaming files, can we do it
> > properly?
>
> I will take care of it. I have to respin the patchset for a silly bug anyways.
I guess with the header name too: umd.h -> usermode_driver.h ?
^ permalink raw reply
* Re: [PATCH v2 05/15] umh: Separate the user mode driver and the user mode helper support
From: Eric W. Biederman @ 2020-07-01 17:18 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, David Miller, Greg Kroah-Hartman,
Tetsuo Handa, Alexei Starovoitov, Kees Cook, Andrew Morton,
Alexei Starovoitov, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
LSM List, Casey Schaufler, Luis Chamberlain
In-Reply-To: <CAHk-=wihqhksXHkcjuTrYmC-vajeRcNh3s6eeoJNxS7wp77dFQ@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, Jun 29, 2020 at 1:05 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> 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.
>>
>> kernel/umd.c | 146 +++++++++++++++++++++++++++++++++++++++
>> kernel/umh.c | 139 -------------------------------------
>
> I certainly don't object to the split, but I hate the name.
>
> We have uml, umd and umh for user mode {linux, drivers, helper}
> respectively.And honestly, I don't see the point in using an obscure
> and unreadable TLA for something like this.
>
> I really don't think it would hurt to write out even the full name
> with "usermode_driver.c" or something like that, would it?
>
> Then "umd" could be continued to be used as a prefix for the helper
> functions, by all means, but if we startv renaming files, can we do it
> properly?
I will take care of it. I have to respin the patchset for a silly bug anyways.
Eric
^ permalink raw reply
* Re: [PATCH v2 00/15] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-07-01 17:12 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: linux-kernel, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler,
Luis Chamberlain, Linus Torvalds
In-Reply-To: <20200630165256.i7wdfjxmqu73fewc@ast-mbp.dhcp.thefacebook.com>
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Tue, Jun 30, 2020 at 07:29:34AM -0500, Eric W. Biederman wrote:
>>
>> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
>> index 91474884ddb7..3e1874030daa 100644
>> --- a/net/bpfilter/bpfilter_kern.c
>> +++ b/net/bpfilter/bpfilter_kern.c
>> @@ -19,8 +19,8 @@ static void shutdown_umh(void)
>> struct pid *tgid = info->tgid;
>>
>> if (tgid) {
>> - kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
>> - wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
>> + kill_pid(tgid, SIGKILL, 1);
>> + wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>> bpfilter_umh_cleanup(info);
>> }
>> }
>>
>> > And then did:
>> > while true; do iptables -L;rmmod bpfilter; done
>> >
>> > Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().
>>
>> Hmm. The wake up happens just of tgid->wait_pidfd happens just before
>> release_task is called so there is a race. As it is possible to wake
>> up and then go back to sleep before pid_has_task becomes false.
>>
>> So I think I need a friendly helper that does:
>>
>> bool task_has_exited(struct pid *tgid)
>> {
>> bool exited = false;
>>
>> rcu_read_lock();
>> tsk = pid_task(tgid, PIDTYPE_TGID);
>> exited = !!tsk;
>> if (tsk) {
>> exited = !!tsk->exit_state;
>> out:
>> rcu_unlock();
>> return exited;
>> }
>
> All makes sense to me.
> If I understood the race condition such helper should indeed solve it.
> Are you going to add such patch to your series?
> I'll proceed with my work on top of your series and will ignore this
> race for now, but I think it should be fixed before we land this set
> into multiple trees.
Yes. I am just finishing it up now.
Eric
^ permalink raw reply
* Re: [PATCH v3 1/1] fs: move kernel_read_file* to its own include file
From: Greg Kroah-Hartman @ 2020-07-01 16:55 UTC (permalink / raw)
To: Scott Branden
Cc: Christoph Hellwig, Luis Chamberlain, Alexander Viro,
Eric Biederman, Jessica Yu, BCM Kernel Feedback,
Rafael J . Wysocki, James Morris, Serge E . Hallyn, Mimi Zohar,
Dmitry Kasatkin, Kees Cook, Paul Moore, Stephen Smalley,
Eric Paris, linux-kernel, linux-fsdevel, kexec,
linux-security-module, linux-integrity, selinux
In-Reply-To: <20200617161218.18487-1-scott.branden@broadcom.com>
On Wed, Jun 17, 2020 at 09:12:18AM -0700, Scott Branden wrote:
> Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
> include file. That header gets pulled in just about everywhere
> and doesn't really need functions not related to the general fs interface.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-07-01 16:01 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Tetsuo Handa, Christoph Hellwig, Eric W. Biederman, ast, axboe,
bfields, bridge, chainsaw, christian.brauner, chuck.lever, davem,
dhowells, gregkh, jarkko.sakkinen, jmorris, josh, keescook,
keyrings, kuba, lars.ellenberg, linux-fsdevel, linux-kernel,
linux-nfs, linux-security-module, nikolay, philipp.reisner,
ravenexp, roopa, serge, slyfox, viro, yangtiezhu, netdev,
markward, linux-s390
In-Reply-To: <20200701155819.GU4332@42.do-not-panic.com>
On 01.07.20 17:58, Luis Chamberlain wrote:
[...]
>>>
>>> Ah, well that would be a different fix required, becuase again,
>>> br_stp_start() does not untangle the correct error today really.
>>> I also I think it would be odd odd that SIGSEGV or another signal
>>> is what was terminating Christian's bridge stp call, but let's
>>> find out!
>>>
>>> Note we pass 0 to the options to wait so the mistake here could indeed
>>> be that we did not need KWIFSIGNALED(). I was afraid of this prospect...
>>> as it other implications.
>>>
>>> It means we either *open code* all callers, or we handle this in a
>>> unified way on the umh. And if we do handle this in a unified way, it
>>> then begs the question as to *what* do we pass for the signals case and
>>> continued case. Below we just pass the signal, and treat continued as
>>> OK, but treating continued as OK would also be a *new* change as well.
>>>
>>> For instance (this goes just boot tested, but Christian if you can
>>> try this as well that would be appreciated):
>>
>>
>> Does not help, the bridge stays in DOWN state.
>
> OK thanks for testing, that was fast! Does your code go through the
> STP kernel path or userpath? If it is taking the STP kernel path
> then this is not the real culprit to your issue then.
I have no idea and I cannot look into this right now. I can test
patches as compile,reboot and test is almost no effort.
FWIW, this is just the network of a KVM guest of libvirts default network
no longer working, maybe you can reproduce this on x86 as well?
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Luis Chamberlain @ 2020-07-01 15:58 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Tetsuo Handa, Christoph Hellwig, Eric W. Biederman, ast, axboe,
bfields, bridge, chainsaw, christian.brauner, chuck.lever, davem,
dhowells, gregkh, jarkko.sakkinen, jmorris, josh, keescook,
keyrings, kuba, lars.ellenberg, linux-fsdevel, linux-kernel,
linux-nfs, linux-security-module, nikolay, philipp.reisner,
ravenexp, roopa, serge, slyfox, viro, yangtiezhu, netdev,
markward, linux-s390
In-Reply-To: <f9f0f868-e511-990a-2a74-1806ac0cb7ac@de.ibm.com>
On Wed, Jul 01, 2020 at 05:48:48PM +0200, Christian Borntraeger wrote:
>
>
> On 01.07.20 17:38, Luis Chamberlain wrote:
> > On Wed, Jul 01, 2020 at 11:08:57PM +0900, Tetsuo Handa wrote:
> >> On 2020/07/01 22:53, Luis Chamberlain wrote:
> >>>> Well, it is not br_stp_call_user() but br_stp_start() which is expecting
> >>>> to set sub_info->retval for both KWIFEXITED() case and KWIFSIGNALED() case.
> >>>> That is, sub_info->retval needs to carry raw value (i.e. without "umh: fix
> >>>> processed error when UMH_WAIT_PROC is used" will be the correct behavior).
> >>>
> >>> br_stp_start() doesn't check for the raw value, it just checks for err
> >>> or !err. So the patch, "umh: fix processed error when UMH_WAIT_PROC is
> >>> used" propagates the correct error now.
> >>
> >> No. If "/sbin/bridge-stp virbr0 start" terminated due to e.g. SIGSEGV
> >> (for example, by inserting "kill -SEGV $$" into right after "#!/bin/sh" line),
> >> br_stp_start() needs to select BR_KERNEL_STP path. We can't assume that
> >> /sbin/bridge-stp is always terminated by exit() syscall (and hence we can't
> >> ignore KWIFSIGNALED() case in call_usermodehelper_exec_sync()).
> >
> > Ah, well that would be a different fix required, becuase again,
> > br_stp_start() does not untangle the correct error today really.
> > I also I think it would be odd odd that SIGSEGV or another signal
> > is what was terminating Christian's bridge stp call, but let's
> > find out!
> >
> > Note we pass 0 to the options to wait so the mistake here could indeed
> > be that we did not need KWIFSIGNALED(). I was afraid of this prospect...
> > as it other implications.
> >
> > It means we either *open code* all callers, or we handle this in a
> > unified way on the umh. And if we do handle this in a unified way, it
> > then begs the question as to *what* do we pass for the signals case and
> > continued case. Below we just pass the signal, and treat continued as
> > OK, but treating continued as OK would also be a *new* change as well.
> >
> > For instance (this goes just boot tested, but Christian if you can
> > try this as well that would be appreciated):
>
>
> Does not help, the bridge stays in DOWN state.
OK thanks for testing, that was fast! Does your code go through the
STP kernel path or userpath? If it is taking the STP kernel path
then this is not the real culprit to your issue then.
Luis
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-07-01 15:48 UTC (permalink / raw)
To: Luis Chamberlain, Tetsuo Handa
Cc: Christoph Hellwig, Eric W. Biederman, ast, axboe, bfields, bridge,
chainsaw, christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <20200701153859.GT4332@42.do-not-panic.com>
On 01.07.20 17:38, Luis Chamberlain wrote:
> On Wed, Jul 01, 2020 at 11:08:57PM +0900, Tetsuo Handa wrote:
>> On 2020/07/01 22:53, Luis Chamberlain wrote:
>>>> Well, it is not br_stp_call_user() but br_stp_start() which is expecting
>>>> to set sub_info->retval for both KWIFEXITED() case and KWIFSIGNALED() case.
>>>> That is, sub_info->retval needs to carry raw value (i.e. without "umh: fix
>>>> processed error when UMH_WAIT_PROC is used" will be the correct behavior).
>>>
>>> br_stp_start() doesn't check for the raw value, it just checks for err
>>> or !err. So the patch, "umh: fix processed error when UMH_WAIT_PROC is
>>> used" propagates the correct error now.
>>
>> No. If "/sbin/bridge-stp virbr0 start" terminated due to e.g. SIGSEGV
>> (for example, by inserting "kill -SEGV $$" into right after "#!/bin/sh" line),
>> br_stp_start() needs to select BR_KERNEL_STP path. We can't assume that
>> /sbin/bridge-stp is always terminated by exit() syscall (and hence we can't
>> ignore KWIFSIGNALED() case in call_usermodehelper_exec_sync()).
>
> Ah, well that would be a different fix required, becuase again,
> br_stp_start() does not untangle the correct error today really.
> I also I think it would be odd odd that SIGSEGV or another signal
> is what was terminating Christian's bridge stp call, but let's
> find out!
>
> Note we pass 0 to the options to wait so the mistake here could indeed
> be that we did not need KWIFSIGNALED(). I was afraid of this prospect...
> as it other implications.
>
> It means we either *open code* all callers, or we handle this in a
> unified way on the umh. And if we do handle this in a unified way, it
> then begs the question as to *what* do we pass for the signals case and
> continued case. Below we just pass the signal, and treat continued as
> OK, but treating continued as OK would also be a *new* change as well.
>
> For instance (this goes just boot tested, but Christian if you can
> try this as well that would be appreciated):
Does not help, the bridge stays in DOWN state.
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Luis Chamberlain @ 2020-07-01 15:38 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Christian Borntraeger, Christoph Hellwig, Eric W. Biederman, ast,
axboe, bfields, bridge, chainsaw, christian.brauner, chuck.lever,
davem, dhowells, gregkh, jarkko.sakkinen, jmorris, josh, keescook,
keyrings, kuba, lars.ellenberg, linux-fsdevel, linux-kernel,
linux-nfs, linux-security-module, nikolay, philipp.reisner,
ravenexp, roopa, serge, slyfox, viro, yangtiezhu, netdev,
markward, mcgrof, linux-s390
In-Reply-To: <8d714a23-bac4-7631-e5fc-f97c20a46083@i-love.sakura.ne.jp>
On Wed, Jul 01, 2020 at 11:08:57PM +0900, Tetsuo Handa wrote:
> On 2020/07/01 22:53, Luis Chamberlain wrote:
> >> Well, it is not br_stp_call_user() but br_stp_start() which is expecting
> >> to set sub_info->retval for both KWIFEXITED() case and KWIFSIGNALED() case.
> >> That is, sub_info->retval needs to carry raw value (i.e. without "umh: fix
> >> processed error when UMH_WAIT_PROC is used" will be the correct behavior).
> >
> > br_stp_start() doesn't check for the raw value, it just checks for err
> > or !err. So the patch, "umh: fix processed error when UMH_WAIT_PROC is
> > used" propagates the correct error now.
>
> No. If "/sbin/bridge-stp virbr0 start" terminated due to e.g. SIGSEGV
> (for example, by inserting "kill -SEGV $$" into right after "#!/bin/sh" line),
> br_stp_start() needs to select BR_KERNEL_STP path. We can't assume that
> /sbin/bridge-stp is always terminated by exit() syscall (and hence we can't
> ignore KWIFSIGNALED() case in call_usermodehelper_exec_sync()).
Ah, well that would be a different fix required, becuase again,
br_stp_start() does not untangle the correct error today really.
I also I think it would be odd odd that SIGSEGV or another signal
is what was terminating Christian's bridge stp call, but let's
find out!
Note we pass 0 to the options to wait so the mistake here could indeed
be that we did not need KWIFSIGNALED(). I was afraid of this prospect...
as it other implications.
It means we either *open code* all callers, or we handle this in a
unified way on the umh. And if we do handle this in a unified way, it
then begs the question as to *what* do we pass for the signals case and
continued case. Below we just pass the signal, and treat continued as
OK, but treating continued as OK would also be a *new* change as well.
For instance (this goes just boot tested, but Christian if you can
try this as well that would be appreciated):
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index bba06befbff5..d1898f5dd1fc 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -105,10 +105,12 @@ extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
/* Only add helpers for actual use cases in the kernel */
#define KWEXITSTATUS(status) (__KWEXITSTATUS(status))
+#define KWTERMSIG(status) (__KWTERMSIG(status))
+#define KWSTOPSIG(status) (__KWSTOPSIG(status))
#define KWIFEXITED(status) (__KWIFEXITED(status))
-
-/* Nonzero if STATUS indicates normal termination. */
-#define __KWIFEXITED(status) (__KWTERMSIG(status) == 0)
+#define KWIFSIGNALED(status) (__KWIFSIGNALED(status))
+#define KWIFSTOPPED(status) (__KWIFSTOPPED(status))
+#define KWIFCONTINUED(status) (__KWIFCONTINUED(status))
/* If KWIFEXITED(STATUS), the low-order 8 bits of the status. */
#define __KWEXITSTATUS(status) (((status) & 0xff00) >> 8)
@@ -116,6 +118,24 @@ extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
/* If KWIFSIGNALED(STATUS), the terminating signal. */
#define __KWTERMSIG(status) ((status) & 0x7f)
+/* If KWIFSTOPPED(STATUS), the signal that stopped the child. */
+#define __KWSTOPSIG(status) __KWEXITSTATUS(status)
+
+/* Nonzero if STATUS indicates normal termination. */
+#define __KWIFEXITED(status) (__KWTERMSIG(status) == 0)
+
+/* Nonzero if STATUS indicates termination by a signal. */
+#define __KWIFSIGNALED(status) \
+ (((signed char) (((status) & 0x7f) + 1) >> 1) > 0)
+
+/* Nonzero if STATUS indicates the child is stopped. */
+#define __KWIFSTOPPED(status) (((status) & 0xff) == 0x7f)
+
+/* Nonzero if STATUS indicates the child continued after a stop. */
+#define __KWIFCONTINUED(status) ((status) == __KW_CONTINUED)
+
+#define __KW_CONTINUED 0xffff
+
extern void free_task(struct task_struct *tsk);
/* sched_exec is called by processes performing an exec */
diff --git a/kernel/umh.c b/kernel/umh.c
index f81e8698e36e..c98fb1ed90c9 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -156,6 +156,18 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
*/
if (KWIFEXITED(ret))
sub_info->retval = KWEXITSTATUS(ret);
+ /*
+ * Do we really want to be passing the signal, or do we pass
+ * a single error code for all cases?
+ */
+ else if (KWIFSIGNALED(ret))
+ sub_info->retval = KWTERMSIG(ret);
+ /* Same here */
+ else if (KWIFSTOPPED((ret)))
+ sub_info->retval = KWSTOPSIG(ret);
+ /* And are we really sure we want this? */
+ else if (KWIFCONTINUED((ret)))
+ sub_info->retval = 0;
}
/* Restore default kernel sig handler */
^ permalink raw reply related
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-07-01 15:26 UTC (permalink / raw)
To: Luis Chamberlain, Tetsuo Handa
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <20200701135324.GS4332@42.do-not-panic.com>
On 01.07.20 15:53, Luis Chamberlain wrote:
> On Wed, Jul 01, 2020 at 10:24:29PM +0900, Tetsuo Handa wrote:
>> On 2020/07/01 19:08, Christian Borntraeger wrote:
>>>
>>>
>>> On 30.06.20 19:57, Luis Chamberlain wrote:
>>>> On Fri, Jun 26, 2020 at 02:54:10AM +0000, Luis Chamberlain wrote:
>>>>> On Wed, Jun 24, 2020 at 08:37:55PM +0200, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 24.06.20 20:32, Christian Borntraeger wrote:
>>>>>> [...]>
>>>>>>> So the translations look correct. But your change is actually a sematic change
>>>>>>> if(ret) will only trigger if there is an error
>>>>>>> if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
>>>>>>> and we did not do it before.
>>>>>>>
>>>>>>
>>>>>> So the right fix is
>>>>>>
>>>>>> diff --git a/kernel/umh.c b/kernel/umh.c
>>>>>> index f81e8698e36e..a3a3196e84d1 100644
>>>>>> --- a/kernel/umh.c
>>>>>> +++ b/kernel/umh.c
>>>>>> @@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
>>>>>> * the real error code is already in sub_info->retval or
>>>>>> * sub_info->retval is 0 anyway, so don't mess with it then.
>>>>>> */
>>>>>> - if (KWIFEXITED(ret))
>>>>>> + if (KWEXITSTATUS(ret))
>>>>>> sub_info->retval = KWEXITSTATUS(ret);
>>
>> Well, it is not br_stp_call_user() but br_stp_start() which is expecting
>> to set sub_info->retval for both KWIFEXITED() case and KWIFSIGNALED() case.
>> That is, sub_info->retval needs to carry raw value (i.e. without "umh: fix
>> processed error when UMH_WAIT_PROC is used" will be the correct behavior).
>
> br_stp_start() doesn't check for the raw value, it just checks for err
> or !err. So the patch, "umh: fix processed error when UMH_WAIT_PROC is
> used" propagates the correct error now.
>
> Christian, can you try removing the binary temporarily and seeing if
> you get your bridge working?
As I matter of fact I do NOT have /sbin/bridge-stp installed.
^ permalink raw reply
* Re: [PATCH v2 11/11] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Tyler Hicks @ 2020-07-01 14:38 UTC (permalink / raw)
To: Dave Young
Cc: Mimi Zohar, Dmitry Kasatkin, Prakhar Srivastava, kexec,
James Morris, linux-kernel, Lakshmi Ramasubramanian,
linux-security-module, Eric Biederman, linux-integrity,
Serge E . Hallyn
In-Reply-To: <20200701080416.GC3878@dhcp-128-65.nay.redhat.com>
On 2020-07-01 16:04:16, Dave Young wrote:
> Hi,
> On 06/26/20 at 05:39pm, Tyler Hicks wrote:
> > Take the properties of the kexec kernel's inode and the current task
> > ownership into consideration when matching a KEXEC_CMDLINE operation to
> > the rules in the IMA policy. This allows for some uniformity when
> > writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
> > and KEXEC_CMDLINE operations.
> >
> > Prior to this patch, it was not possible to write a set of rules like
> > this:
> >
> > dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> > measure func=KEXEC_KERNEL_CHECK
> > measure func=KEXEC_INITRAMFS_CHECK
> > measure func=KEXEC_CMDLINE
> >
> > The inode information associated with the kernel being loaded by a
> > kexec_kernel_load(2) syscall can now be included in the decision to
> > measure or not
> >
> > Additonally, the uid, euid, and subj_* conditionals can also now be
> > used in KEXEC_CMDLINE rules. There was no technical reason as to why
> > those conditionals weren't being considered previously other than
> > ima_match_rules() didn't have a valid inode to use so it immediately
> > bailed out for KEXEC_CMDLINE operations rather than going through the
> > full list of conditional comparisons.
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: kexec@lists.infradead.org
> > ---
> >
> > * v2
> > - Moved the inode parameter of process_buffer_measurement() to be the
> > first parameter so that it more closely matches process_masurement()
> >
> > include/linux/ima.h | 4 ++--
> > kernel/kexec_file.c | 2 +-
> > security/integrity/ima/ima.h | 2 +-
> > security/integrity/ima/ima_api.c | 2 +-
> > security/integrity/ima/ima_appraise.c | 2 +-
> > security/integrity/ima/ima_asymmetric_keys.c | 2 +-
> > security/integrity/ima/ima_main.c | 23 +++++++++++++++-----
> > security/integrity/ima/ima_policy.c | 17 +++++----------
> > security/integrity/ima/ima_queue_keys.c | 2 +-
> > 9 files changed, 31 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 9164e1534ec9..d15100de6cdd 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > enum kernel_read_file_id id);
> > extern void ima_post_path_mknod(struct dentry *dentry);
> > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > -extern void ima_kexec_cmdline(const void *buf, int size);
> > +extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> >
> > #ifdef CONFIG_IMA_KEXEC
> > extern void ima_add_kexec_buffer(struct kimage *image);
> > @@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > return -EOPNOTSUPP;
> > }
> >
> > -static inline void ima_kexec_cmdline(const void *buf, int size) {}
> > +static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
> > #endif /* CONFIG_IMA */
> >
> > #ifndef CONFIG_IMA_KEXEC
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index bb05fd52de85..07df431c1f21 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> > goto out;
> > }
> >
> > - ima_kexec_cmdline(image->cmdline_buf,
> > + ima_kexec_cmdline(kernel_fd, image->cmdline_buf,
> > image->cmdline_buf_len - 1);
> > }
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 59ec28f5c117..ff2bf57ff0c7 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -265,7 +265,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> > struct evm_ima_xattr_data *xattr_value,
> > int xattr_len, const struct modsig *modsig, int pcr,
> > struct ima_template_desc *template_desc);
> > -void process_buffer_measurement(const void *buf, int size,
> > +void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> > const char *eventname, enum ima_hooks func,
> > int pcr, const char *keyring);
> > void ima_audit_measurement(struct integrity_iint_cache *iint,
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index bf22de8b7ce0..4f39fb93f278 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> >
> > /**
> > * ima_get_action - appraise & measure decision based on policy.
> > - * @inode: pointer to inode to measure
> > + * @inode: pointer to the inode associated with the object being validated
> > * @cred: pointer to credentials structure to validate
> > * @secid: secid of the task being validated
> > * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index a9649b04b9f1..6c52bf7ea7f0 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -328,7 +328,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
> >
> > rc = is_binary_blacklisted(digest, digestsize);
> > if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> > - process_buffer_measurement(digest, digestsize,
> > + process_buffer_measurement(NULL, digest, digestsize,
> > "blacklisted-hash", NONE,
> > pcr, NULL);
> > }
> > diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> > index aaae80c4e376..1c68c500c26f 100644
> > --- a/security/integrity/ima/ima_asymmetric_keys.c
> > +++ b/security/integrity/ima/ima_asymmetric_keys.c
> > @@ -58,7 +58,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> > * if the IMA policy is configured to measure a key linked
> > * to the given keyring.
> > */
> > - process_buffer_measurement(payload, payload_len,
> > + process_buffer_measurement(NULL, payload, payload_len,
> > keyring->description, KEY_CHECK, 0,
> > keyring->description);
> > }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 8351b2fd48e0..8a91711ca79b 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -726,6 +726,7 @@ int ima_load_data(enum kernel_load_data_id id)
> >
> > /*
> > * process_buffer_measurement - Measure the buffer to ima log.
> > + * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
> > * @buf: pointer to the buffer that needs to be added to the log.
> > * @size: size of buffer(in bytes).
> > * @eventname: event name to be used for the buffer entry.
> > @@ -735,7 +736,7 @@ int ima_load_data(enum kernel_load_data_id id)
> > *
> > * Based on policy, the buffer is measured into the ima log.
> > */
> > -void process_buffer_measurement(const void *buf, int size,
> > +void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> > const char *eventname, enum ima_hooks func,
> > int pcr, const char *keyring)
> > {
> > @@ -768,7 +769,7 @@ void process_buffer_measurement(const void *buf, int size,
> > */
> > if (func) {
> > security_task_getsecid(current, &secid);
> > - action = ima_get_action(NULL, current_cred(), secid, 0, func,
> > + action = ima_get_action(inode, current_cred(), secid, 0, func,
> > &pcr, &template, keyring);
> > if (!(action & IMA_MEASURE))
> > return;
> > @@ -823,16 +824,26 @@ void process_buffer_measurement(const void *buf, int size,
> >
> > /**
> > * ima_kexec_cmdline - measure kexec cmdline boot args
> > + * @kernel_fd: file descriptor of the kexec kernel being loaded
> > * @buf: pointer to buffer
> > * @size: size of buffer
> > *
> > * Buffers can only be measured, not appraised.
> > */
> > -void ima_kexec_cmdline(const void *buf, int size)
> > +void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> > {
> > - if (buf && size != 0)
> > - process_buffer_measurement(buf, size, "kexec-cmdline",
> > - KEXEC_CMDLINE, 0, NULL);
> > + struct fd f;
> > +
> > + if (!buf || !size)
> > + return;
> > +
> > + f = fdget(kernel_fd);
> > + if (!f.file)
> > + return;
> > +
> > + process_buffer_measurement(file_inode(f.file), buf, size,
> > + "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
> > + fdput(f);
> > }
> >
> > static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 5eb14b567a31..294323b36d06 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -443,13 +443,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > {
> > int i;
> >
> > - if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
> > - if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
> > - if (func == KEY_CHECK)
> > - return ima_match_keyring(rule, keyring, cred);
> > - return true;
> > - }
> > - return false;
> > + if (func == KEY_CHECK) {
> > + return (rule->flags & IMA_FUNC) && (rule->func == func) &&
> > + ima_match_keyring(rule, keyring, cred);
> > }
> > if ((rule->flags & IMA_FUNC) &&
> > (rule->func != func && func != POST_SETATTR))
> > @@ -1007,10 +1003,9 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > if (entry->action & ~(MEASURE | DONT_MEASURE))
> > return false;
> >
> > - if (entry->flags & ~(IMA_FUNC | IMA_PCR))
> > - return false;
> > -
> > - if (ima_rule_contains_lsm_cond(entry))
> > + if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID |
> > + IMA_FOWNER | IMA_FSUUID |
> > + IMA_EUID | IMA_PCR | IMA_FSNAME))
> > return false;
> >
> > break;
> > diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> > index 56ce24a18b66..69a8626a35c0 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -158,7 +158,7 @@ void ima_process_queued_keys(void)
> >
> > list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
> > if (!timer_expired)
> > - process_buffer_measurement(entry->payload,
> > + process_buffer_measurement(NULL, entry->payload,
> > entry->payload_len,
> > entry->keyring_name,
> > KEY_CHECK, 0,
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> >
>
> Although I still do not understand the deep knowledge of IMA, I
> still wonder to know what is the effect to the behavior changes end user
> visible. Does it work with a kernel built-in commandline? eg no
> cmdlien passed at all.
Ah, very good question. This IMA hook (KEXEC_CMDLINE) only measures the
string passed to the cmdline argument of the kexec_file_load(2) syscall.
However, kernel commandline options injected into a kernel with the
CONFIG_CMDLINE or CONFIG_CMDLINE_EXTEND Kconfig options would still be
measured, as part of the vmlinux as a whole, by the KEXEC_KERNEL_CHECK
IMA hook.
Tyler
>
> Thanks
> Dave
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Tetsuo Handa @ 2020-07-01 14:08 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christian Borntraeger, Christoph Hellwig, ast, axboe, bfields,
bridge, chainsaw, christian.brauner, chuck.lever, davem, dhowells,
gregkh, jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <20200701135324.GS4332@42.do-not-panic.com>
On 2020/07/01 22:53, Luis Chamberlain wrote:
>> Well, it is not br_stp_call_user() but br_stp_start() which is expecting
>> to set sub_info->retval for both KWIFEXITED() case and KWIFSIGNALED() case.
>> That is, sub_info->retval needs to carry raw value (i.e. without "umh: fix
>> processed error when UMH_WAIT_PROC is used" will be the correct behavior).
>
> br_stp_start() doesn't check for the raw value, it just checks for err
> or !err. So the patch, "umh: fix processed error when UMH_WAIT_PROC is
> used" propagates the correct error now.
No. If "/sbin/bridge-stp virbr0 start" terminated due to e.g. SIGSEGV
(for example, by inserting "kill -SEGV $$" into right after "#!/bin/sh" line),
br_stp_start() needs to select BR_KERNEL_STP path. We can't assume that
/sbin/bridge-stp is always terminated by exit() syscall (and hence we can't
ignore KWIFSIGNALED() case in call_usermodehelper_exec_sync()).
^ permalink raw reply
* Re: [RFC PATCH 0/7] x86: introduce system calls addess space isolation
From: 黄金海 @ 2020-07-01 14:05 UTC (permalink / raw)
To: rppt
Cc: James.Bottomley, alexandre.chartre, bp, dave.hansen, hpa, jwadams,
keescook, linux-kernel, linux-mm, linux-security-module, luto,
mingo, peterz, pjt, tglx, x86
How about performance when running with ASI?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox