* [PATCH bpf-next v5 7/7] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-03-23 16:44 UTC (permalink / raw)
To: linux-kernel, bpf, linux-security-module
Cc: Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Paul Turner, Jann Horn, Florent Revest, Brendan Jackman,
Greg Kroah-Hartman
In-Reply-To: <20200323164415.12943-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
* Load/attach a BPF program to the file_mprotect (int) and
bprm_committed_creds (void) LSM hooks.
* Perform an action that triggers the hook.
* Verify if the audit event was received using a shared global
result variable.
Signed-off-by: KP Singh <kpsingh@google.com>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
Reviewed-by: Thomas Garnier <thgarnie@google.com>
---
tools/testing/selftests/bpf/lsm_helpers.h | 19 +++
.../selftests/bpf/prog_tests/lsm_test.c | 112 ++++++++++++++++++
.../selftests/bpf/progs/lsm_int_hook.c | 54 +++++++++
.../selftests/bpf/progs/lsm_void_hook.c | 41 +++++++
4 files changed, 226 insertions(+)
create mode 100644 tools/testing/selftests/bpf/lsm_helpers.h
create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_test.c
create mode 100644 tools/testing/selftests/bpf/progs/lsm_int_hook.c
create mode 100644 tools/testing/selftests/bpf/progs/lsm_void_hook.c
diff --git a/tools/testing/selftests/bpf/lsm_helpers.h b/tools/testing/selftests/bpf/lsm_helpers.h
new file mode 100644
index 000000000000..3de230df93db
--- /dev/null
+++ b/tools/testing/selftests/bpf/lsm_helpers.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+#ifndef _LSM_HELPERS_H
+#define _LSM_HELPERS_H
+
+struct lsm_prog_result {
+ /* This ensures that the LSM Hook only monitors the PID requested
+ * by the loader
+ */
+ __u32 monitored_pid;
+ /* The number of calls to the prog for the monitored PID.
+ */
+ __u32 count;
+};
+
+#endif /* _LSM_HELPERS_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_test.c b/tools/testing/selftests/bpf/prog_tests/lsm_test.c
new file mode 100644
index 000000000000..5fd6b8f569f7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_test.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#include <test_progs.h>
+#include <sys/mman.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <malloc.h>
+#include <stdlib.h>
+
+#include "lsm_helpers.h"
+#include "lsm_void_hook.skel.h"
+#include "lsm_int_hook.skel.h"
+
+char *LS_ARGS[] = {"true", NULL};
+
+int heap_mprotect(void)
+{
+ void *buf;
+ long sz;
+
+ sz = sysconf(_SC_PAGESIZE);
+ if (sz < 0)
+ return sz;
+
+ buf = memalign(sz, 2 * sz);
+ if (buf == NULL)
+ return -ENOMEM;
+
+ return mprotect(buf, sz, PROT_READ | PROT_EXEC);
+}
+
+int exec_ls(struct lsm_prog_result *result)
+{
+ int child_pid;
+
+ child_pid = fork();
+ if (child_pid == 0) {
+ result->monitored_pid = getpid();
+ execvp(LS_ARGS[0], LS_ARGS);
+ return -EINVAL;
+ } else if (child_pid > 0)
+ return wait(NULL);
+
+ return -EINVAL;
+}
+
+void test_lsm_void_hook(void)
+{
+ struct lsm_prog_result *result;
+ struct lsm_void_hook *skel = NULL;
+ int err, duration = 0;
+
+ skel = lsm_void_hook__open_and_load();
+ if (CHECK(!skel, "skel_load", "lsm_void_hook skeleton failed\n"))
+ goto close_prog;
+
+ err = lsm_void_hook__attach(skel);
+ if (CHECK(err, "attach", "lsm_void_hook attach failed: %d\n", err))
+ goto close_prog;
+
+ result = &skel->bss->result;
+
+ err = exec_ls(result);
+ if (CHECK(err < 0, "exec_ls", "err %d errno %d\n", err, errno))
+ goto close_prog;
+
+ if (CHECK(result->count != 1, "count", "count = %d", result->count))
+ goto close_prog;
+
+ CHECK_FAIL(result->count != 1);
+
+close_prog:
+ lsm_void_hook__destroy(skel);
+}
+
+void test_lsm_int_hook(void)
+{
+ struct lsm_prog_result *result;
+ struct lsm_int_hook *skel = NULL;
+ int err, duration = 0;
+
+ skel = lsm_int_hook__open_and_load();
+ if (CHECK(!skel, "skel_load", "lsm_int_hook skeleton failed\n"))
+ goto close_prog;
+
+ err = lsm_int_hook__attach(skel);
+ if (CHECK(err, "attach", "lsm_int_hook attach failed: %d\n", err))
+ goto close_prog;
+
+ result = &skel->bss->result;
+ result->monitored_pid = getpid();
+
+ err = heap_mprotect();
+ if (CHECK(errno != EPERM, "heap_mprotect", "want errno=EPERM, got %d\n",
+ errno))
+ goto close_prog;
+
+ CHECK_FAIL(result->count != 1);
+
+close_prog:
+ lsm_int_hook__destroy(skel);
+}
+
+void test_lsm_test(void)
+{
+ test_lsm_void_hook();
+ test_lsm_int_hook();
+}
diff --git a/tools/testing/selftests/bpf/progs/lsm_int_hook.c b/tools/testing/selftests/bpf/progs/lsm_int_hook.c
new file mode 100644
index 000000000000..1c5028ddca61
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lsm_int_hook.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <errno.h>
+#include "lsm_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct lsm_prog_result result = {
+ .monitored_pid = 0,
+ .count = 0,
+};
+
+/*
+ * Define some of the structs used in the BPF program.
+ * Only the field names and their sizes need to be the
+ * same as the kernel type, the order is irrelevant.
+ */
+struct mm_struct {
+ unsigned long start_brk, brk;
+} __attribute__((preserve_access_index));
+
+struct vm_area_struct {
+ unsigned long vm_start, vm_end;
+ struct mm_struct *vm_mm;
+} __attribute__((preserve_access_index));
+
+SEC("lsm/file_mprotect")
+int BPF_PROG(test_int_hook, struct vm_area_struct *vma,
+ unsigned long reqprot, unsigned long prot, int ret)
+{
+ if (ret != 0)
+ return ret;
+
+ __u32 pid = bpf_get_current_pid_tgid();
+ int is_heap = 0;
+
+ is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
+ vma->vm_end <= vma->vm_mm->brk);
+
+ if (is_heap && result.monitored_pid == pid) {
+ result.count++;
+ ret = -EPERM;
+ }
+
+ return ret;
+}
diff --git a/tools/testing/selftests/bpf/progs/lsm_void_hook.c b/tools/testing/selftests/bpf/progs/lsm_void_hook.c
new file mode 100644
index 000000000000..4d01a8536413
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lsm_void_hook.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <errno.h>
+#include "lsm_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct lsm_prog_result result = {
+ .monitored_pid = 0,
+ .count = 0,
+};
+
+/*
+ * Define some of the structs used in the BPF program.
+ * Only the field names and their sizes need to be the
+ * same as the kernel type, the order is irrelevant.
+ */
+struct linux_binprm {
+ const char *filename;
+} __attribute__((preserve_access_index));
+
+SEC("lsm/bprm_committed_creds")
+int BPF_PROG(test_void_hook, struct linux_binprm *bprm)
+{
+ __u32 pid = bpf_get_current_pid_tgid();
+ char fmt[] = "lsm(bprm_committed_creds): process executed %s\n";
+
+ bpf_trace_printk(fmt, sizeof(fmt), bprm->filename);
+ if (result.monitored_pid == pid)
+ result.count++;
+
+ return 0;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v5 2/7] security: Refactor declaration of LSM hooks
From: KP Singh @ 2020-03-23 16:44 UTC (permalink / raw)
To: linux-kernel, bpf, linux-security-module
Cc: Brendan Jackman, Florent Revest, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman, Greg Kroah-Hartman
In-Reply-To: <20200323164415.12943-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
The information about the different types of LSM hooks is scattered
in two locations i.e. union security_list_options and
struct security_hook_heads. Rather than duplicating this information
even further for BPF_PROG_TYPE_LSM, define all the hooks with the
LSM_HOOK macro in lsm_hook_names.h which is then used to generate all
the data structures required by the LSM framework.
Signed-off-by: KP Singh <kpsingh@google.com>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
---
include/linux/lsm_hook_names.h | 354 +++++++++++++++++++
include/linux/lsm_hooks.h | 622 +--------------------------------
2 files changed, 360 insertions(+), 616 deletions(-)
create mode 100644 include/linux/lsm_hook_names.h
diff --git a/include/linux/lsm_hook_names.h b/include/linux/lsm_hook_names.h
new file mode 100644
index 000000000000..412e4ca24c9b
--- /dev/null
+++ b/include/linux/lsm_hook_names.h
@@ -0,0 +1,354 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Linux Security Module Hook declarations.
+ *
+ * Copyright (C) 2001 WireX Communications, Inc <chris@wirex.com>
+ * Copyright (C) 2001 Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright (C) 2001 Networks Associates Technology, Inc <ssmalley@nai.com>
+ * Copyright (C) 2001 James Morris <jmorris@intercode.com.au>
+ * Copyright (C) 2001 Silicon Graphics, Inc. (Trust Technology Group)
+ * Copyright (C) 2015 Intel Corporation.
+ * Copyright (C) 2015 Casey Schaufler <casey@schaufler-ca.com>
+ * Copyright (C) 2016 Mellanox Techonologies
+ * Copyright (C) 2020 Google LLC.
+ */
+
+/* The macro LSM_HOOK is used to define the data structures required by the
+ * the LSM framework using the pattern:
+ *
+ * struct security_hook_heads {
+ * #define LSM_HOOK(RET, NAME, ...) struct hlist_head NAME;
+ * #include <linux/lsm_hook_names.h>
+ * #undef LSM_HOOK
+ * };
+ */
+LSM_HOOK(int, binder_set_context_mgr, struct task_struct *mgr)
+LSM_HOOK(int, binder_transaction, struct task_struct *from,
+ struct task_struct *to)
+LSM_HOOK(int, binder_transfer_binder, struct task_struct *from,
+ struct task_struct *to)
+LSM_HOOK(int, binder_transfer_file, struct task_struct *from,
+ struct task_struct *to, struct file *file)
+LSM_HOOK(int, ptrace_access_check, struct task_struct *child, unsigned int mode)
+LSM_HOOK(int, ptrace_traceme, struct task_struct *parent)
+LSM_HOOK(int, capget, struct task_struct *target, kernel_cap_t *effective,
+ kernel_cap_t *inheritable, kernel_cap_t *permitted)
+LSM_HOOK(int, capset, struct cred *new, const struct cred *old,
+ const kernel_cap_t *effective, const kernel_cap_t *inheritable,
+ const kernel_cap_t *permitted)
+LSM_HOOK(int, capable, const struct cred *cred, struct user_namespace *ns,
+ int cap, unsigned int opts)
+LSM_HOOK(int, quotactl, int cmds, int type, int id, struct super_block *sb)
+LSM_HOOK(int, quota_on, struct dentry *dentry)
+LSM_HOOK(int, syslog, int type)
+LSM_HOOK(int, settime, const struct timespec64 *ts, const struct timezone *tz)
+LSM_HOOK(int, vm_enough_memory, struct mm_struct *mm, long pages)
+LSM_HOOK(int, bprm_set_creds, struct linux_binprm *bprm)
+LSM_HOOK(int, bprm_check_security, struct linux_binprm *bprm)
+LSM_HOOK(void, bprm_committing_creds, struct linux_binprm *bprm)
+LSM_HOOK(void, bprm_committed_creds, struct linux_binprm *bprm)
+LSM_HOOK(int, fs_context_dup, struct fs_context *fc, struct fs_context *src_sc)
+LSM_HOOK(int, fs_context_parse_param, struct fs_context *fc,
+ struct fs_parameter *param)
+LSM_HOOK(int, sb_alloc_security, struct super_block *sb)
+LSM_HOOK(void, sb_free_security, struct super_block *sb)
+LSM_HOOK(void, sb_free_mnt_opts, void *mnt_opts)
+LSM_HOOK(int, sb_eat_lsm_opts, char *orig, void **mnt_opts)
+LSM_HOOK(int, sb_remount, struct super_block *sb, void *mnt_opts)
+LSM_HOOK(int, sb_kern_mount, struct super_block *sb)
+LSM_HOOK(int, sb_show_options, struct seq_file *m, struct super_block *sb)
+LSM_HOOK(int, sb_statfs, struct dentry *dentry)
+LSM_HOOK(int, sb_mount, const char *dev_name, const struct path *path,
+ const char *type, unsigned long flags, void *data)
+LSM_HOOK(int, sb_umount, struct vfsmount *mnt, int flags)
+LSM_HOOK(int, sb_pivotroot, const struct path *old_path,
+ const struct path *new_path)
+LSM_HOOK(int, sb_set_mnt_opts, struct super_block *sb, void *mnt_opts,
+ unsigned long kern_flags, unsigned long *set_kern_flags)
+LSM_HOOK(int, sb_clone_mnt_opts, const struct super_block *oldsb,
+ struct super_block *newsb, unsigned long kern_flags,
+ unsigned long *set_kern_flags)
+LSM_HOOK(int, sb_add_mnt_opt, const char *option, const char *val, int len,
+ void **mnt_opts)
+LSM_HOOK(int, move_mount, const struct path *from_path,
+ const struct path *to_path)
+LSM_HOOK(int, dentry_init_security, struct dentry *dentry, int mode,
+ const struct qstr *name, void **ctx, u32 *ctxlen)
+LSM_HOOK(int, dentry_create_files_as, struct dentry *dentry, int mode,
+ struct qstr *name, const struct cred *old, struct cred *new)
+#ifdef CONFIG_SECURITY_PATH
+LSM_HOOK(int, path_unlink, const struct path *dir, struct dentry *dentry)
+LSM_HOOK(int, path_mkdir, const struct path *dir, struct dentry *dentry,
+ umode_t mode)
+LSM_HOOK(int, path_rmdir, const struct path *dir, struct dentry *dentry)
+LSM_HOOK(int, path_mknod, const struct path *dir, struct dentry *dentry,
+ umode_t mode, unsigned int dev)
+LSM_HOOK(int, path_truncate, const struct path *path)
+LSM_HOOK(int, path_symlink, const struct path *dir, struct dentry *dentry,
+ const char *old_name)
+LSM_HOOK(int, path_link, struct dentry *old_dentry, const struct path *new_dir,
+ struct dentry *new_dentry)
+LSM_HOOK(int, path_rename, const struct path *old_dir,
+ struct dentry *old_dentry, const struct path *new_dir,
+ struct dentry *new_dentry)
+LSM_HOOK(int, path_chmod, const struct path *path, umode_t mode)
+LSM_HOOK(int, path_chown, const struct path *path, kuid_t uid, kgid_t gid)
+LSM_HOOK(int, path_chroot, const struct path *path)
+#endif
+
+/* Needed for inode based security check */
+LSM_HOOK(int, path_notify, const struct path *path, u64 mask,
+ unsigned int obj_type)
+LSM_HOOK(int, inode_alloc_security, struct inode *inode)
+LSM_HOOK(void, inode_free_security, struct inode *inode)
+LSM_HOOK(int, inode_init_security, struct inode *inode, struct inode *dir,
+ const struct qstr *qstr, const char **name, void **value, size_t *len)
+LSM_HOOK(int, inode_create, struct inode *dir, struct dentry *dentry,
+ umode_t mode)
+LSM_HOOK(int, inode_link, struct dentry *old_dentry, struct inode *dir,
+ struct dentry *new_dentry)
+LSM_HOOK(int, inode_unlink, struct inode *dir, struct dentry *dentry)
+LSM_HOOK(int, inode_symlink, struct inode *dir, struct dentry *dentry,
+ const char *old_name)
+LSM_HOOK(int, inode_mkdir, struct inode *dir, struct dentry *dentry,
+ umode_t mode)
+LSM_HOOK(int, inode_rmdir, struct inode *dir, struct dentry *dentry)
+LSM_HOOK(int, inode_mknod, struct inode *dir, struct dentry *dentry,
+ umode_t mode, dev_t dev)
+LSM_HOOK(int, inode_rename, struct inode *old_dir, struct dentry *old_dentry,
+ struct inode *new_dir, struct dentry *new_dentry)
+LSM_HOOK(int, inode_readlink, struct dentry *dentry)
+LSM_HOOK(int, inode_follow_link, struct dentry *dentry, struct inode *inode,
+ bool rcu)
+LSM_HOOK(int, inode_permission, struct inode *inode, int mask)
+LSM_HOOK(int, inode_setattr, struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(int, inode_getattr, const struct path *path)
+LSM_HOOK(int, inode_setxattr, struct dentry *dentry, const char *name,
+ const void *value, size_t size, int flags)
+LSM_HOOK(void, inode_post_setxattr, struct dentry *dentry, const char *name,
+ const void *value, size_t size, int flags)
+LSM_HOOK(int, inode_getxattr, struct dentry *dentry, const char *name)
+LSM_HOOK(int, inode_listxattr, struct dentry *dentry)
+LSM_HOOK(int, inode_removexattr, struct dentry *dentry, const char *name)
+LSM_HOOK(int, inode_need_killpriv, struct dentry *dentry)
+LSM_HOOK(int, inode_killpriv, struct dentry *dentry)
+LSM_HOOK(int, inode_getsecurity, struct inode *inode, const char *name,
+ void **buffer, bool alloc)
+LSM_HOOK(int, inode_setsecurity, struct inode *inode, const char *name,
+ const void *value, size_t size, int flags)
+LSM_HOOK(int, inode_listsecurity, struct inode *inode, char *buffer,
+ size_t buffer_size)
+LSM_HOOK(void, inode_getsecid, struct inode *inode, u32 *secid)
+LSM_HOOK(int, inode_copy_up, struct dentry *src, struct cred **new)
+LSM_HOOK(int, inode_copy_up_xattr, const char *name)
+LSM_HOOK(int, kernfs_init_security, struct kernfs_node *kn_dir,
+ struct kernfs_node *kn)
+LSM_HOOK(int, file_permission, struct file *file, int mask)
+LSM_HOOK(int, file_alloc_security, struct file *file)
+LSM_HOOK(void, file_free_security, struct file *file)
+LSM_HOOK(int, file_ioctl, struct file *file, unsigned int cmd,
+ unsigned long arg)
+LSM_HOOK(int, mmap_addr, unsigned long addr)
+LSM_HOOK(int, mmap_file, struct file *file, unsigned long reqprot,
+ unsigned long prot, unsigned long flags)
+LSM_HOOK(int, file_mprotect, struct vm_area_struct *vma, unsigned long reqprot,
+ unsigned long prot)
+LSM_HOOK(int, file_lock, struct file *file, unsigned int cmd)
+LSM_HOOK(int, file_fcntl, struct file *file, unsigned int cmd,
+ unsigned long arg)
+LSM_HOOK(void, file_set_fowner, struct file *file)
+LSM_HOOK(int, file_send_sigiotask, struct task_struct *tsk,
+ struct fown_struct *fown, int sig)
+LSM_HOOK(int, file_receive, struct file *file)
+LSM_HOOK(int, file_open, struct file *file)
+LSM_HOOK(int, task_alloc, struct task_struct *task, unsigned long clone_flags)
+LSM_HOOK(void, task_free, struct task_struct *task)
+LSM_HOOK(int, cred_alloc_blank, struct cred *cred, gfp_t gfp)
+LSM_HOOK(void, cred_free, struct cred *cred)
+LSM_HOOK(int, cred_prepare, struct cred *new, const struct cred *old, gfp_t gfp)
+LSM_HOOK(void, cred_transfer, struct cred *new, const struct cred *old)
+LSM_HOOK(void, cred_getsecid, const struct cred *c, u32 *secid)
+LSM_HOOK(int, kernel_act_as, struct cred *new, u32 secid)
+LSM_HOOK(int, kernel_create_files_as, struct cred *new, struct inode *inode)
+LSM_HOOK(int, kernel_module_request, char *kmod_name)
+LSM_HOOK(int, kernel_load_data, enum kernel_load_data_id id)
+LSM_HOOK(int, kernel_read_file, struct file *file, enum kernel_read_file_id id)
+LSM_HOOK(int, kernel_post_read_file, struct file *file, char *buf, loff_t size,
+ enum kernel_read_file_id id)
+LSM_HOOK(int, task_fix_setuid, struct cred *new, const struct cred *old,
+ int flags)
+LSM_HOOK(int, task_setpgid, struct task_struct *p, pid_t pgid)
+LSM_HOOK(int, task_getpgid, struct task_struct *p)
+LSM_HOOK(int, task_getsid, struct task_struct *p)
+LSM_HOOK(void, task_getsecid, struct task_struct *p, u32 *secid)
+LSM_HOOK(int, task_setnice, struct task_struct *p, int nice)
+LSM_HOOK(int, task_setioprio, struct task_struct *p, int ioprio)
+LSM_HOOK(int, task_getioprio, struct task_struct *p)
+LSM_HOOK(int, task_prlimit, const struct cred *cred, const struct cred *tcred,
+ unsigned int flags)
+LSM_HOOK(int, task_setrlimit, struct task_struct *p, unsigned int resource,
+ struct rlimit *new_rlim)
+LSM_HOOK(int, task_setscheduler, struct task_struct *p)
+LSM_HOOK(int, task_getscheduler, struct task_struct *p)
+LSM_HOOK(int, task_movememory, struct task_struct *p)
+LSM_HOOK(int, task_kill, struct task_struct *p, struct kernel_siginfo *info,
+ int sig, const struct cred *cred)
+LSM_HOOK(int, task_prctl, int option, unsigned long arg2, unsigned long arg3,
+ unsigned long arg4, unsigned long arg5)
+LSM_HOOK(void, task_to_inode, struct task_struct *p, struct inode *inode)
+LSM_HOOK(int, ipc_permission, struct kern_ipc_perm *ipcp, short flag)
+LSM_HOOK(void, ipc_getsecid, struct kern_ipc_perm *ipcp, u32 *secid)
+LSM_HOOK(int, msg_msg_alloc_security, struct msg_msg *msg)
+LSM_HOOK(void, msg_msg_free_security, struct msg_msg *msg)
+LSM_HOOK(int, msg_queue_alloc_security, struct kern_ipc_perm *perm)
+LSM_HOOK(void, msg_queue_free_security, struct kern_ipc_perm *perm)
+LSM_HOOK(int, msg_queue_associate, struct kern_ipc_perm *perm, int msqflg)
+LSM_HOOK(int, msg_queue_msgctl, struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, msg_queue_msgsnd, struct kern_ipc_perm *perm, struct msg_msg *msg,
+ int msqflg)
+LSM_HOOK(int, msg_queue_msgrcv, struct kern_ipc_perm *perm, struct msg_msg *msg,
+ struct task_struct *target, long type, int mode)
+LSM_HOOK(int, shm_alloc_security, struct kern_ipc_perm *perm)
+LSM_HOOK(void, shm_free_security, struct kern_ipc_perm *perm)
+LSM_HOOK(int, shm_associate, struct kern_ipc_perm *perm, int shmflg)
+LSM_HOOK(int, shm_shmctl, struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, shm_shmat, struct kern_ipc_perm *perm, char __user *shmaddr,
+ int shmflg)
+LSM_HOOK(int, sem_alloc_security, struct kern_ipc_perm *perm)
+LSM_HOOK(void, sem_free_security, struct kern_ipc_perm *perm)
+LSM_HOOK(int, sem_associate, struct kern_ipc_perm *perm, int semflg)
+LSM_HOOK(int, sem_semctl, struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, sem_semop, struct kern_ipc_perm *perm, struct sembuf *sops,
+ unsigned nsops, int alter)
+LSM_HOOK(int, netlink_send, struct sock *sk, struct sk_buff *skb)
+LSM_HOOK(void, d_instantiate, struct dentry *dentry, struct inode *inode)
+LSM_HOOK(int, getprocattr, struct task_struct *p, char *name, char **value)
+LSM_HOOK(int, setprocattr, const char *name, void *value, size_t size)
+LSM_HOOK(int, ismaclabel, const char *name)
+LSM_HOOK(int, secid_to_secctx, u32 secid, char **secdata, u32 *seclen)
+LSM_HOOK(int, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
+LSM_HOOK(void, release_secctx, char *secdata, u32 seclen)
+LSM_HOOK(void, inode_invalidate_secctx, struct inode *inode)
+LSM_HOOK(int, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
+LSM_HOOK(int, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
+LSM_HOOK(int, inode_getsecctx, struct inode *inode, void **ctx, u32 *ctxlen)
+#ifdef CONFIG_SECURITY_NETWORK
+LSM_HOOK(int, unix_stream_connect, struct sock *sock, struct sock *other,
+ struct sock *newsk)
+LSM_HOOK(int, unix_may_send, struct socket *sock, struct socket *other)
+LSM_HOOK(int, socket_create, int family, int type, int protocol, int kern)
+LSM_HOOK(int, socket_post_create, struct socket *sock, int family, int type,
+ int protocol, int kern)
+LSM_HOOK(int, socket_socketpair, struct socket *socka, struct socket *sockb)
+LSM_HOOK(int, socket_bind, struct socket *sock, struct sockaddr *address,
+ int addrlen)
+LSM_HOOK(int, socket_connect, struct socket *sock, struct sockaddr *address,
+ int addrlen)
+LSM_HOOK(int, socket_listen, struct socket *sock, int backlog)
+LSM_HOOK(int, socket_accept, struct socket *sock, struct socket *newsock)
+LSM_HOOK(int, socket_sendmsg, struct socket *sock, struct msghdr *msg, int size)
+LSM_HOOK(int, socket_recvmsg, struct socket *sock, struct msghdr *msg, int size,
+ int flags)
+LSM_HOOK(int, socket_getsockname, struct socket *sock)
+LSM_HOOK(int, socket_getpeername, struct socket *sock)
+LSM_HOOK(int, socket_getsockopt, struct socket *sock, int level, int optname)
+LSM_HOOK(int, socket_setsockopt, struct socket *sock, int level, int optname)
+LSM_HOOK(int, socket_shutdown, struct socket *sock, int how)
+LSM_HOOK(int, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
+LSM_HOOK(int, socket_getpeersec_stream, struct socket *sock,
+ char __user *optval, int __user *optlen, unsigned len)
+LSM_HOOK(int, socket_getpeersec_dgram, struct socket *sock, struct sk_buff *skb,
+ u32 *secid)
+LSM_HOOK(int, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
+LSM_HOOK(void, sk_free_security, struct sock *sk)
+LSM_HOOK(void, sk_clone_security, const struct sock *sk, struct sock *newsk)
+LSM_HOOK(void, sk_getsecid, struct sock *sk, u32 *secid)
+LSM_HOOK(void, sock_graft, struct sock *sk, struct socket *parent)
+LSM_HOOK(int, inet_conn_request, struct sock *sk, struct sk_buff *skb,
+ struct request_sock *req)
+LSM_HOOK(void, inet_csk_clone, struct sock *newsk,
+ const struct request_sock *req)
+LSM_HOOK(void, inet_conn_established, struct sock *sk, struct sk_buff *skb)
+LSM_HOOK(int, secmark_relabel_packet, u32 secid)
+LSM_HOOK(void, secmark_refcount_inc, void)
+LSM_HOOK(void, secmark_refcount_dec, void)
+LSM_HOOK(void, req_classify_flow, const struct request_sock *req,
+ struct flowi *fl)
+LSM_HOOK(int, tun_dev_alloc_security, void **security)
+LSM_HOOK(void, tun_dev_free_security, void *security)
+LSM_HOOK(int, tun_dev_create, void)
+LSM_HOOK(int, tun_dev_attach_queue, void *security)
+LSM_HOOK(int, tun_dev_attach, struct sock *sk, void *security)
+LSM_HOOK(int, tun_dev_open, void *security)
+LSM_HOOK(int, sctp_assoc_request, struct sctp_endpoint *ep, struct sk_buff *skb)
+LSM_HOOK(int, sctp_bind_connect, struct sock *sk, int optname,
+ struct sockaddr *address, int addrlen)
+LSM_HOOK(void, sctp_sk_clone, struct sctp_endpoint *ep, struct sock *sk,
+ struct sock *newsk)
+#endif /* CONFIG_SECURITY_NETWORK */
+
+#ifdef CONFIG_SECURITY_INFINIBAND
+LSM_HOOK(int, ib_pkey_access, void *sec, u64 subnet_prefix, u16 pkey)
+LSM_HOOK(int, ib_endport_manage_subnet, void *sec, const char *dev_name,
+ u8 port_num)
+LSM_HOOK(int, ib_alloc_security, void **sec)
+LSM_HOOK(void, ib_free_security, void *sec)
+#endif /* CONFIG_SECURITY_INFINIBAND */
+
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+LSM_HOOK(int, xfrm_policy_alloc_security, struct xfrm_sec_ctx **ctxp,
+ struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp)
+LSM_HOOK(int, xfrm_policy_clone_security, struct xfrm_sec_ctx *old_ctx,
+ struct xfrm_sec_ctx **new_ctx)
+LSM_HOOK(void, xfrm_policy_free_security, struct xfrm_sec_ctx *ctx)
+LSM_HOOK(int, xfrm_policy_delete_security, struct xfrm_sec_ctx *ctx)
+LSM_HOOK(int, xfrm_state_alloc, struct xfrm_state *x,
+ struct xfrm_user_sec_ctx *sec_ctx)
+LSM_HOOK(int, xfrm_state_alloc_acquire, struct xfrm_state *x,
+ struct xfrm_sec_ctx *polsec, u32 secid)
+LSM_HOOK(void, xfrm_state_free_security, struct xfrm_state *x)
+LSM_HOOK(int, xfrm_state_delete_security, struct xfrm_state *x)
+LSM_HOOK(int, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid,
+ u8 dir)
+LSM_HOOK(int, xfrm_state_pol_flow_match, struct xfrm_state *x,
+ struct xfrm_policy *xp, const struct flowi *fl)
+LSM_HOOK(int, xfrm_decode_session, struct sk_buff *skb, u32 *secid, int ckall)
+#endif /* CONFIG_SECURITY_NETWORK_XFRM */
+
+/* key management security hooks */
+#ifdef CONFIG_KEYS
+LSM_HOOK(int, key_alloc, struct key *key, const struct cred *cred,
+ unsigned long flags)
+LSM_HOOK(void, key_free, struct key *key)
+LSM_HOOK(int, key_permission, key_ref_t key_ref, const struct cred *cred,
+ unsigned perm)
+LSM_HOOK(int, key_getsecurity, struct key *key, char **_buffer)
+#endif /* CONFIG_KEYS */
+
+#ifdef CONFIG_AUDIT
+LSM_HOOK(int, audit_rule_init, u32 field, u32 op, char *rulestr, void **lsmrule)
+LSM_HOOK(int, audit_rule_known, struct audit_krule *krule)
+LSM_HOOK(int, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule)
+LSM_HOOK(void, audit_rule_free, void *lsmrule)
+#endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+LSM_HOOK(int, bpf, int cmd, union bpf_attr *attr, unsigned int size)
+LSM_HOOK(int, bpf_map, struct bpf_map *map, fmode_t fmode)
+LSM_HOOK(int, bpf_prog, struct bpf_prog *prog)
+LSM_HOOK(int, bpf_map_alloc_security, struct bpf_map *map)
+LSM_HOOK(void, bpf_map_free_security, struct bpf_map *map)
+LSM_HOOK(int, bpf_prog_alloc_security, struct bpf_prog_aux *aux)
+LSM_HOOK(void, bpf_prog_free_security, struct bpf_prog_aux *aux)
+#endif /* CONFIG_BPF_SYSCALL */
+
+LSM_HOOK(int, locked_down, enum lockdown_reason what)
+
+#ifdef CONFIG_PERF_EVENTS
+LSM_HOOK(int, perf_event_open, struct perf_event_attr *attr, int type)
+LSM_HOOK(int, perf_event_alloc, struct perf_event *event)
+LSM_HOOK(void, perf_event_free, struct perf_event *event)
+LSM_HOOK(int, perf_event_read, struct perf_event *event)
+LSM_HOOK(int, perf_event_write, struct perf_event *event)
+#endif
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..905954c650ff 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1456,625 +1456,15 @@
* @what: kernel feature being accessed
*/
union security_list_options {
- int (*binder_set_context_mgr)(struct task_struct *mgr);
- int (*binder_transaction)(struct task_struct *from,
- struct task_struct *to);
- int (*binder_transfer_binder)(struct task_struct *from,
- struct task_struct *to);
- int (*binder_transfer_file)(struct task_struct *from,
- struct task_struct *to,
- struct file *file);
-
- int (*ptrace_access_check)(struct task_struct *child,
- unsigned int mode);
- int (*ptrace_traceme)(struct task_struct *parent);
- int (*capget)(struct task_struct *target, kernel_cap_t *effective,
- kernel_cap_t *inheritable, kernel_cap_t *permitted);
- int (*capset)(struct cred *new, const struct cred *old,
- const kernel_cap_t *effective,
- const kernel_cap_t *inheritable,
- const kernel_cap_t *permitted);
- int (*capable)(const struct cred *cred,
- struct user_namespace *ns,
- int cap,
- unsigned int opts);
- int (*quotactl)(int cmds, int type, int id, struct super_block *sb);
- int (*quota_on)(struct dentry *dentry);
- int (*syslog)(int type);
- int (*settime)(const struct timespec64 *ts, const struct timezone *tz);
- int (*vm_enough_memory)(struct mm_struct *mm, long pages);
-
- int (*bprm_set_creds)(struct linux_binprm *bprm);
- int (*bprm_check_security)(struct linux_binprm *bprm);
- void (*bprm_committing_creds)(struct linux_binprm *bprm);
- void (*bprm_committed_creds)(struct linux_binprm *bprm);
-
- int (*fs_context_dup)(struct fs_context *fc, struct fs_context *src_sc);
- int (*fs_context_parse_param)(struct fs_context *fc, struct fs_parameter *param);
-
- int (*sb_alloc_security)(struct super_block *sb);
- void (*sb_free_security)(struct super_block *sb);
- void (*sb_free_mnt_opts)(void *mnt_opts);
- int (*sb_eat_lsm_opts)(char *orig, void **mnt_opts);
- int (*sb_remount)(struct super_block *sb, void *mnt_opts);
- int (*sb_kern_mount)(struct super_block *sb);
- int (*sb_show_options)(struct seq_file *m, struct super_block *sb);
- int (*sb_statfs)(struct dentry *dentry);
- int (*sb_mount)(const char *dev_name, const struct path *path,
- const char *type, unsigned long flags, void *data);
- int (*sb_umount)(struct vfsmount *mnt, int flags);
- int (*sb_pivotroot)(const struct path *old_path, const struct path *new_path);
- int (*sb_set_mnt_opts)(struct super_block *sb,
- void *mnt_opts,
- unsigned long kern_flags,
- unsigned long *set_kern_flags);
- int (*sb_clone_mnt_opts)(const struct super_block *oldsb,
- struct super_block *newsb,
- unsigned long kern_flags,
- unsigned long *set_kern_flags);
- int (*sb_add_mnt_opt)(const char *option, const char *val, int len,
- void **mnt_opts);
- int (*move_mount)(const struct path *from_path, const struct path *to_path);
- int (*dentry_init_security)(struct dentry *dentry, int mode,
- const struct qstr *name, void **ctx,
- u32 *ctxlen);
- int (*dentry_create_files_as)(struct dentry *dentry, int mode,
- struct qstr *name,
- const struct cred *old,
- struct cred *new);
-
-
-#ifdef CONFIG_SECURITY_PATH
- int (*path_unlink)(const struct path *dir, struct dentry *dentry);
- int (*path_mkdir)(const struct path *dir, struct dentry *dentry,
- umode_t mode);
- int (*path_rmdir)(const struct path *dir, struct dentry *dentry);
- int (*path_mknod)(const struct path *dir, struct dentry *dentry,
- umode_t mode, unsigned int dev);
- int (*path_truncate)(const struct path *path);
- int (*path_symlink)(const struct path *dir, struct dentry *dentry,
- const char *old_name);
- int (*path_link)(struct dentry *old_dentry, const struct path *new_dir,
- struct dentry *new_dentry);
- int (*path_rename)(const struct path *old_dir, struct dentry *old_dentry,
- const struct path *new_dir,
- struct dentry *new_dentry);
- int (*path_chmod)(const struct path *path, umode_t mode);
- int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
- int (*path_chroot)(const struct path *path);
-#endif
- /* Needed for inode based security check */
- int (*path_notify)(const struct path *path, u64 mask,
- unsigned int obj_type);
- int (*inode_alloc_security)(struct inode *inode);
- void (*inode_free_security)(struct inode *inode);
- int (*inode_init_security)(struct inode *inode, struct inode *dir,
- const struct qstr *qstr,
- const char **name, void **value,
- size_t *len);
- int (*inode_create)(struct inode *dir, struct dentry *dentry,
- umode_t mode);
- int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
- struct dentry *new_dentry);
- int (*inode_unlink)(struct inode *dir, struct dentry *dentry);
- int (*inode_symlink)(struct inode *dir, struct dentry *dentry,
- const char *old_name);
- int (*inode_mkdir)(struct inode *dir, struct dentry *dentry,
- umode_t mode);
- int (*inode_rmdir)(struct inode *dir, struct dentry *dentry);
- int (*inode_mknod)(struct inode *dir, struct dentry *dentry,
- umode_t mode, dev_t dev);
- int (*inode_rename)(struct inode *old_dir, struct dentry *old_dentry,
- struct inode *new_dir,
- struct dentry *new_dentry);
- int (*inode_readlink)(struct dentry *dentry);
- int (*inode_follow_link)(struct dentry *dentry, struct inode *inode,
- bool rcu);
- int (*inode_permission)(struct inode *inode, int mask);
- int (*inode_setattr)(struct dentry *dentry, struct iattr *attr);
- int (*inode_getattr)(const struct path *path);
- int (*inode_setxattr)(struct dentry *dentry, const char *name,
- const void *value, size_t size, int flags);
- void (*inode_post_setxattr)(struct dentry *dentry, const char *name,
- const void *value, size_t size,
- int flags);
- int (*inode_getxattr)(struct dentry *dentry, const char *name);
- int (*inode_listxattr)(struct dentry *dentry);
- int (*inode_removexattr)(struct dentry *dentry, const char *name);
- int (*inode_need_killpriv)(struct dentry *dentry);
- int (*inode_killpriv)(struct dentry *dentry);
- int (*inode_getsecurity)(struct inode *inode, const char *name,
- void **buffer, bool alloc);
- int (*inode_setsecurity)(struct inode *inode, const char *name,
- const void *value, size_t size,
- int flags);
- int (*inode_listsecurity)(struct inode *inode, char *buffer,
- size_t buffer_size);
- void (*inode_getsecid)(struct inode *inode, u32 *secid);
- int (*inode_copy_up)(struct dentry *src, struct cred **new);
- int (*inode_copy_up_xattr)(const char *name);
-
- int (*kernfs_init_security)(struct kernfs_node *kn_dir,
- struct kernfs_node *kn);
-
- int (*file_permission)(struct file *file, int mask);
- int (*file_alloc_security)(struct file *file);
- void (*file_free_security)(struct file *file);
- int (*file_ioctl)(struct file *file, unsigned int cmd,
- unsigned long arg);
- int (*mmap_addr)(unsigned long addr);
- int (*mmap_file)(struct file *file, unsigned long reqprot,
- unsigned long prot, unsigned long flags);
- int (*file_mprotect)(struct vm_area_struct *vma, unsigned long reqprot,
- unsigned long prot);
- int (*file_lock)(struct file *file, unsigned int cmd);
- int (*file_fcntl)(struct file *file, unsigned int cmd,
- unsigned long arg);
- void (*file_set_fowner)(struct file *file);
- int (*file_send_sigiotask)(struct task_struct *tsk,
- struct fown_struct *fown, int sig);
- int (*file_receive)(struct file *file);
- int (*file_open)(struct file *file);
-
- int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
- void (*task_free)(struct task_struct *task);
- int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
- void (*cred_free)(struct cred *cred);
- int (*cred_prepare)(struct cred *new, const struct cred *old,
- gfp_t gfp);
- void (*cred_transfer)(struct cred *new, const struct cred *old);
- void (*cred_getsecid)(const struct cred *c, u32 *secid);
- int (*kernel_act_as)(struct cred *new, u32 secid);
- int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
- int (*kernel_module_request)(char *kmod_name);
- int (*kernel_load_data)(enum kernel_load_data_id id);
- int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
- int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
- enum kernel_read_file_id id);
- int (*task_fix_setuid)(struct cred *new, const struct cred *old,
- int flags);
- int (*task_setpgid)(struct task_struct *p, pid_t pgid);
- int (*task_getpgid)(struct task_struct *p);
- int (*task_getsid)(struct task_struct *p);
- void (*task_getsecid)(struct task_struct *p, u32 *secid);
- int (*task_setnice)(struct task_struct *p, int nice);
- int (*task_setioprio)(struct task_struct *p, int ioprio);
- int (*task_getioprio)(struct task_struct *p);
- int (*task_prlimit)(const struct cred *cred, const struct cred *tcred,
- unsigned int flags);
- int (*task_setrlimit)(struct task_struct *p, unsigned int resource,
- struct rlimit *new_rlim);
- int (*task_setscheduler)(struct task_struct *p);
- int (*task_getscheduler)(struct task_struct *p);
- int (*task_movememory)(struct task_struct *p);
- int (*task_kill)(struct task_struct *p, struct kernel_siginfo *info,
- int sig, const struct cred *cred);
- int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3,
- unsigned long arg4, unsigned long arg5);
- void (*task_to_inode)(struct task_struct *p, struct inode *inode);
-
- int (*ipc_permission)(struct kern_ipc_perm *ipcp, short flag);
- void (*ipc_getsecid)(struct kern_ipc_perm *ipcp, u32 *secid);
-
- int (*msg_msg_alloc_security)(struct msg_msg *msg);
- void (*msg_msg_free_security)(struct msg_msg *msg);
-
- int (*msg_queue_alloc_security)(struct kern_ipc_perm *perm);
- void (*msg_queue_free_security)(struct kern_ipc_perm *perm);
- int (*msg_queue_associate)(struct kern_ipc_perm *perm, int msqflg);
- int (*msg_queue_msgctl)(struct kern_ipc_perm *perm, int cmd);
- int (*msg_queue_msgsnd)(struct kern_ipc_perm *perm, struct msg_msg *msg,
- int msqflg);
- int (*msg_queue_msgrcv)(struct kern_ipc_perm *perm, struct msg_msg *msg,
- struct task_struct *target, long type,
- int mode);
-
- int (*shm_alloc_security)(struct kern_ipc_perm *perm);
- void (*shm_free_security)(struct kern_ipc_perm *perm);
- int (*shm_associate)(struct kern_ipc_perm *perm, int shmflg);
- int (*shm_shmctl)(struct kern_ipc_perm *perm, int cmd);
- int (*shm_shmat)(struct kern_ipc_perm *perm, char __user *shmaddr,
- int shmflg);
-
- int (*sem_alloc_security)(struct kern_ipc_perm *perm);
- void (*sem_free_security)(struct kern_ipc_perm *perm);
- int (*sem_associate)(struct kern_ipc_perm *perm, int semflg);
- int (*sem_semctl)(struct kern_ipc_perm *perm, int cmd);
- int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
- unsigned nsops, int alter);
-
- int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
-
- void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
-
- int (*getprocattr)(struct task_struct *p, char *name, char **value);
- int (*setprocattr)(const char *name, void *value, size_t size);
- int (*ismaclabel)(const char *name);
- int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
- int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
- void (*release_secctx)(char *secdata, u32 seclen);
-
- void (*inode_invalidate_secctx)(struct inode *inode);
- int (*inode_notifysecctx)(struct inode *inode, void *ctx, u32 ctxlen);
- int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
- int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
-
-#ifdef CONFIG_SECURITY_NETWORK
- int (*unix_stream_connect)(struct sock *sock, struct sock *other,
- struct sock *newsk);
- int (*unix_may_send)(struct socket *sock, struct socket *other);
-
- int (*socket_create)(int family, int type, int protocol, int kern);
- int (*socket_post_create)(struct socket *sock, int family, int type,
- int protocol, int kern);
- int (*socket_socketpair)(struct socket *socka, struct socket *sockb);
- int (*socket_bind)(struct socket *sock, struct sockaddr *address,
- int addrlen);
- int (*socket_connect)(struct socket *sock, struct sockaddr *address,
- int addrlen);
- int (*socket_listen)(struct socket *sock, int backlog);
- int (*socket_accept)(struct socket *sock, struct socket *newsock);
- int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg,
- int size);
- int (*socket_recvmsg)(struct socket *sock, struct msghdr *msg,
- int size, int flags);
- int (*socket_getsockname)(struct socket *sock);
- int (*socket_getpeername)(struct socket *sock);
- int (*socket_getsockopt)(struct socket *sock, int level, int optname);
- int (*socket_setsockopt)(struct socket *sock, int level, int optname);
- int (*socket_shutdown)(struct socket *sock, int how);
- int (*socket_sock_rcv_skb)(struct sock *sk, struct sk_buff *skb);
- int (*socket_getpeersec_stream)(struct socket *sock,
- char __user *optval,
- int __user *optlen, unsigned len);
- int (*socket_getpeersec_dgram)(struct socket *sock,
- struct sk_buff *skb, u32 *secid);
- int (*sk_alloc_security)(struct sock *sk, int family, gfp_t priority);
- void (*sk_free_security)(struct sock *sk);
- void (*sk_clone_security)(const struct sock *sk, struct sock *newsk);
- void (*sk_getsecid)(struct sock *sk, u32 *secid);
- void (*sock_graft)(struct sock *sk, struct socket *parent);
- int (*inet_conn_request)(struct sock *sk, struct sk_buff *skb,
- struct request_sock *req);
- void (*inet_csk_clone)(struct sock *newsk,
- const struct request_sock *req);
- void (*inet_conn_established)(struct sock *sk, struct sk_buff *skb);
- int (*secmark_relabel_packet)(u32 secid);
- void (*secmark_refcount_inc)(void);
- void (*secmark_refcount_dec)(void);
- void (*req_classify_flow)(const struct request_sock *req,
- struct flowi *fl);
- int (*tun_dev_alloc_security)(void **security);
- void (*tun_dev_free_security)(void *security);
- int (*tun_dev_create)(void);
- int (*tun_dev_attach_queue)(void *security);
- int (*tun_dev_attach)(struct sock *sk, void *security);
- int (*tun_dev_open)(void *security);
- int (*sctp_assoc_request)(struct sctp_endpoint *ep,
- struct sk_buff *skb);
- int (*sctp_bind_connect)(struct sock *sk, int optname,
- struct sockaddr *address, int addrlen);
- void (*sctp_sk_clone)(struct sctp_endpoint *ep, struct sock *sk,
- struct sock *newsk);
-#endif /* CONFIG_SECURITY_NETWORK */
-
-#ifdef CONFIG_SECURITY_INFINIBAND
- int (*ib_pkey_access)(void *sec, u64 subnet_prefix, u16 pkey);
- int (*ib_endport_manage_subnet)(void *sec, const char *dev_name,
- u8 port_num);
- int (*ib_alloc_security)(void **sec);
- void (*ib_free_security)(void *sec);
-#endif /* CONFIG_SECURITY_INFINIBAND */
-
-#ifdef CONFIG_SECURITY_NETWORK_XFRM
- int (*xfrm_policy_alloc_security)(struct xfrm_sec_ctx **ctxp,
- struct xfrm_user_sec_ctx *sec_ctx,
- gfp_t gfp);
- int (*xfrm_policy_clone_security)(struct xfrm_sec_ctx *old_ctx,
- struct xfrm_sec_ctx **new_ctx);
- void (*xfrm_policy_free_security)(struct xfrm_sec_ctx *ctx);
- int (*xfrm_policy_delete_security)(struct xfrm_sec_ctx *ctx);
- int (*xfrm_state_alloc)(struct xfrm_state *x,
- struct xfrm_user_sec_ctx *sec_ctx);
- int (*xfrm_state_alloc_acquire)(struct xfrm_state *x,
- struct xfrm_sec_ctx *polsec,
- u32 secid);
- void (*xfrm_state_free_security)(struct xfrm_state *x);
- int (*xfrm_state_delete_security)(struct xfrm_state *x);
- int (*xfrm_policy_lookup)(struct xfrm_sec_ctx *ctx, u32 fl_secid,
- u8 dir);
- int (*xfrm_state_pol_flow_match)(struct xfrm_state *x,
- struct xfrm_policy *xp,
- const struct flowi *fl);
- int (*xfrm_decode_session)(struct sk_buff *skb, u32 *secid, int ckall);
-#endif /* CONFIG_SECURITY_NETWORK_XFRM */
-
- /* key management security hooks */
-#ifdef CONFIG_KEYS
- int (*key_alloc)(struct key *key, const struct cred *cred,
- unsigned long flags);
- void (*key_free)(struct key *key);
- int (*key_permission)(key_ref_t key_ref, const struct cred *cred,
- unsigned perm);
- int (*key_getsecurity)(struct key *key, char **_buffer);
-#endif /* CONFIG_KEYS */
-
-#ifdef CONFIG_AUDIT
- int (*audit_rule_init)(u32 field, u32 op, char *rulestr,
- void **lsmrule);
- int (*audit_rule_known)(struct audit_krule *krule);
- int (*audit_rule_match)(u32 secid, u32 field, u32 op, void *lsmrule);
- void (*audit_rule_free)(void *lsmrule);
-#endif /* CONFIG_AUDIT */
-
-#ifdef CONFIG_BPF_SYSCALL
- int (*bpf)(int cmd, union bpf_attr *attr,
- unsigned int size);
- int (*bpf_map)(struct bpf_map *map, fmode_t fmode);
- int (*bpf_prog)(struct bpf_prog *prog);
- int (*bpf_map_alloc_security)(struct bpf_map *map);
- void (*bpf_map_free_security)(struct bpf_map *map);
- int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
- void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
-#endif /* CONFIG_BPF_SYSCALL */
- int (*locked_down)(enum lockdown_reason what);
-#ifdef CONFIG_PERF_EVENTS
- int (*perf_event_open)(struct perf_event_attr *attr, int type);
- int (*perf_event_alloc)(struct perf_event *event);
- void (*perf_event_free)(struct perf_event *event);
- int (*perf_event_read)(struct perf_event *event);
- int (*perf_event_write)(struct perf_event *event);
-
-#endif
+ #define LSM_HOOK(RET, NAME, ...) RET (*NAME)(__VA_ARGS__);
+ #include "lsm_hook_names.h"
+ #undef LSM_HOOK
};
struct security_hook_heads {
- struct hlist_head binder_set_context_mgr;
- struct hlist_head binder_transaction;
- struct hlist_head binder_transfer_binder;
- struct hlist_head binder_transfer_file;
- struct hlist_head ptrace_access_check;
- struct hlist_head ptrace_traceme;
- struct hlist_head capget;
- struct hlist_head capset;
- struct hlist_head capable;
- struct hlist_head quotactl;
- struct hlist_head quota_on;
- struct hlist_head syslog;
- struct hlist_head settime;
- struct hlist_head vm_enough_memory;
- struct hlist_head bprm_set_creds;
- struct hlist_head bprm_check_security;
- struct hlist_head bprm_committing_creds;
- struct hlist_head bprm_committed_creds;
- struct hlist_head fs_context_dup;
- struct hlist_head fs_context_parse_param;
- struct hlist_head sb_alloc_security;
- struct hlist_head sb_free_security;
- struct hlist_head sb_free_mnt_opts;
- struct hlist_head sb_eat_lsm_opts;
- struct hlist_head sb_remount;
- struct hlist_head sb_kern_mount;
- struct hlist_head sb_show_options;
- struct hlist_head sb_statfs;
- struct hlist_head sb_mount;
- struct hlist_head sb_umount;
- struct hlist_head sb_pivotroot;
- struct hlist_head sb_set_mnt_opts;
- struct hlist_head sb_clone_mnt_opts;
- struct hlist_head sb_add_mnt_opt;
- struct hlist_head move_mount;
- struct hlist_head dentry_init_security;
- struct hlist_head dentry_create_files_as;
-#ifdef CONFIG_SECURITY_PATH
- struct hlist_head path_unlink;
- struct hlist_head path_mkdir;
- struct hlist_head path_rmdir;
- struct hlist_head path_mknod;
- struct hlist_head path_truncate;
- struct hlist_head path_symlink;
- struct hlist_head path_link;
- struct hlist_head path_rename;
- struct hlist_head path_chmod;
- struct hlist_head path_chown;
- struct hlist_head path_chroot;
-#endif
- /* Needed for inode based modules as well */
- struct hlist_head path_notify;
- struct hlist_head inode_alloc_security;
- struct hlist_head inode_free_security;
- struct hlist_head inode_init_security;
- struct hlist_head inode_create;
- struct hlist_head inode_link;
- struct hlist_head inode_unlink;
- struct hlist_head inode_symlink;
- struct hlist_head inode_mkdir;
- struct hlist_head inode_rmdir;
- struct hlist_head inode_mknod;
- struct hlist_head inode_rename;
- struct hlist_head inode_readlink;
- struct hlist_head inode_follow_link;
- struct hlist_head inode_permission;
- struct hlist_head inode_setattr;
- struct hlist_head inode_getattr;
- struct hlist_head inode_setxattr;
- struct hlist_head inode_post_setxattr;
- struct hlist_head inode_getxattr;
- struct hlist_head inode_listxattr;
- struct hlist_head inode_removexattr;
- struct hlist_head inode_need_killpriv;
- struct hlist_head inode_killpriv;
- struct hlist_head inode_getsecurity;
- struct hlist_head inode_setsecurity;
- struct hlist_head inode_listsecurity;
- struct hlist_head inode_getsecid;
- struct hlist_head inode_copy_up;
- struct hlist_head inode_copy_up_xattr;
- struct hlist_head kernfs_init_security;
- struct hlist_head file_permission;
- struct hlist_head file_alloc_security;
- struct hlist_head file_free_security;
- struct hlist_head file_ioctl;
- struct hlist_head mmap_addr;
- struct hlist_head mmap_file;
- struct hlist_head file_mprotect;
- struct hlist_head file_lock;
- struct hlist_head file_fcntl;
- struct hlist_head file_set_fowner;
- struct hlist_head file_send_sigiotask;
- struct hlist_head file_receive;
- struct hlist_head file_open;
- struct hlist_head task_alloc;
- struct hlist_head task_free;
- struct hlist_head cred_alloc_blank;
- struct hlist_head cred_free;
- struct hlist_head cred_prepare;
- struct hlist_head cred_transfer;
- struct hlist_head cred_getsecid;
- struct hlist_head kernel_act_as;
- struct hlist_head kernel_create_files_as;
- struct hlist_head kernel_load_data;
- struct hlist_head kernel_read_file;
- struct hlist_head kernel_post_read_file;
- struct hlist_head kernel_module_request;
- struct hlist_head task_fix_setuid;
- struct hlist_head task_setpgid;
- struct hlist_head task_getpgid;
- struct hlist_head task_getsid;
- struct hlist_head task_getsecid;
- struct hlist_head task_setnice;
- struct hlist_head task_setioprio;
- struct hlist_head task_getioprio;
- struct hlist_head task_prlimit;
- struct hlist_head task_setrlimit;
- struct hlist_head task_setscheduler;
- struct hlist_head task_getscheduler;
- struct hlist_head task_movememory;
- struct hlist_head task_kill;
- struct hlist_head task_prctl;
- struct hlist_head task_to_inode;
- struct hlist_head ipc_permission;
- struct hlist_head ipc_getsecid;
- struct hlist_head msg_msg_alloc_security;
- struct hlist_head msg_msg_free_security;
- struct hlist_head msg_queue_alloc_security;
- struct hlist_head msg_queue_free_security;
- struct hlist_head msg_queue_associate;
- struct hlist_head msg_queue_msgctl;
- struct hlist_head msg_queue_msgsnd;
- struct hlist_head msg_queue_msgrcv;
- struct hlist_head shm_alloc_security;
- struct hlist_head shm_free_security;
- struct hlist_head shm_associate;
- struct hlist_head shm_shmctl;
- struct hlist_head shm_shmat;
- struct hlist_head sem_alloc_security;
- struct hlist_head sem_free_security;
- struct hlist_head sem_associate;
- struct hlist_head sem_semctl;
- struct hlist_head sem_semop;
- struct hlist_head netlink_send;
- struct hlist_head d_instantiate;
- struct hlist_head getprocattr;
- struct hlist_head setprocattr;
- struct hlist_head ismaclabel;
- struct hlist_head secid_to_secctx;
- struct hlist_head secctx_to_secid;
- struct hlist_head release_secctx;
- struct hlist_head inode_invalidate_secctx;
- struct hlist_head inode_notifysecctx;
- struct hlist_head inode_setsecctx;
- struct hlist_head inode_getsecctx;
-#ifdef CONFIG_SECURITY_NETWORK
- struct hlist_head unix_stream_connect;
- struct hlist_head unix_may_send;
- struct hlist_head socket_create;
- struct hlist_head socket_post_create;
- struct hlist_head socket_socketpair;
- struct hlist_head socket_bind;
- struct hlist_head socket_connect;
- struct hlist_head socket_listen;
- struct hlist_head socket_accept;
- struct hlist_head socket_sendmsg;
- struct hlist_head socket_recvmsg;
- struct hlist_head socket_getsockname;
- struct hlist_head socket_getpeername;
- struct hlist_head socket_getsockopt;
- struct hlist_head socket_setsockopt;
- struct hlist_head socket_shutdown;
- struct hlist_head socket_sock_rcv_skb;
- struct hlist_head socket_getpeersec_stream;
- struct hlist_head socket_getpeersec_dgram;
- struct hlist_head sk_alloc_security;
- struct hlist_head sk_free_security;
- struct hlist_head sk_clone_security;
- struct hlist_head sk_getsecid;
- struct hlist_head sock_graft;
- struct hlist_head inet_conn_request;
- struct hlist_head inet_csk_clone;
- struct hlist_head inet_conn_established;
- struct hlist_head secmark_relabel_packet;
- struct hlist_head secmark_refcount_inc;
- struct hlist_head secmark_refcount_dec;
- struct hlist_head req_classify_flow;
- struct hlist_head tun_dev_alloc_security;
- struct hlist_head tun_dev_free_security;
- struct hlist_head tun_dev_create;
- struct hlist_head tun_dev_attach_queue;
- struct hlist_head tun_dev_attach;
- struct hlist_head tun_dev_open;
- struct hlist_head sctp_assoc_request;
- struct hlist_head sctp_bind_connect;
- struct hlist_head sctp_sk_clone;
-#endif /* CONFIG_SECURITY_NETWORK */
-#ifdef CONFIG_SECURITY_INFINIBAND
- struct hlist_head ib_pkey_access;
- struct hlist_head ib_endport_manage_subnet;
- struct hlist_head ib_alloc_security;
- struct hlist_head ib_free_security;
-#endif /* CONFIG_SECURITY_INFINIBAND */
-#ifdef CONFIG_SECURITY_NETWORK_XFRM
- struct hlist_head xfrm_policy_alloc_security;
- struct hlist_head xfrm_policy_clone_security;
- struct hlist_head xfrm_policy_free_security;
- struct hlist_head xfrm_policy_delete_security;
- struct hlist_head xfrm_state_alloc;
- struct hlist_head xfrm_state_alloc_acquire;
- struct hlist_head xfrm_state_free_security;
- struct hlist_head xfrm_state_delete_security;
- struct hlist_head xfrm_policy_lookup;
- struct hlist_head xfrm_state_pol_flow_match;
- struct hlist_head xfrm_decode_session;
-#endif /* CONFIG_SECURITY_NETWORK_XFRM */
-#ifdef CONFIG_KEYS
- struct hlist_head key_alloc;
- struct hlist_head key_free;
- struct hlist_head key_permission;
- struct hlist_head key_getsecurity;
-#endif /* CONFIG_KEYS */
-#ifdef CONFIG_AUDIT
- struct hlist_head audit_rule_init;
- struct hlist_head audit_rule_known;
- struct hlist_head audit_rule_match;
- struct hlist_head audit_rule_free;
-#endif /* CONFIG_AUDIT */
-#ifdef CONFIG_BPF_SYSCALL
- struct hlist_head bpf;
- struct hlist_head bpf_map;
- struct hlist_head bpf_prog;
- struct hlist_head bpf_map_alloc_security;
- struct hlist_head bpf_map_free_security;
- struct hlist_head bpf_prog_alloc_security;
- struct hlist_head bpf_prog_free_security;
-#endif /* CONFIG_BPF_SYSCALL */
- struct hlist_head locked_down;
-#ifdef CONFIG_PERF_EVENTS
- struct hlist_head perf_event_open;
- struct hlist_head perf_event_alloc;
- struct hlist_head perf_event_free;
- struct hlist_head perf_event_read;
- struct hlist_head perf_event_write;
-#endif
+ #define LSM_HOOK(RET, NAME, ...) struct hlist_head NAME;
+ #include "lsm_hook_names.h"
+ #undef LSM_HOOK
} __randomize_layout;
/*
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v5 3/7] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-03-23 16:44 UTC (permalink / raw)
To: linux-kernel, bpf, linux-security-module
Cc: Brendan Jackman, Florent Revest, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman, Greg Kroah-Hartman
In-Reply-To: <20200323164415.12943-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
When CONFIG_BPF_LSM is enabled, nops functions, bpf_lsm_<hook_name>, are
generated for each LSM hook. These nops are initialized as LSM hooks in
a subsequent patch.
Signed-off-by: KP Singh <kpsingh@google.com>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
---
include/linux/bpf_lsm.h | 21 +++++++++++++++++++++
kernel/bpf/bpf_lsm.c | 19 +++++++++++++++++++
2 files changed, 40 insertions(+)
create mode 100644 include/linux/bpf_lsm.h
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
new file mode 100644
index 000000000000..c6423a140220
--- /dev/null
+++ b/include/linux/bpf_lsm.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#ifndef _LINUX_BPF_LSM_H
+#define _LINUX_BPF_LSM_H
+
+#include <linux/bpf.h>
+#include <linux/lsm_hooks.h>
+
+#ifdef CONFIG_BPF_LSM
+
+#define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__);
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK
+
+#endif /* CONFIG_BPF_LSM */
+
+#endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 82875039ca90..530d137f7a84 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -7,6 +7,25 @@
#include <linux/filter.h>
#include <linux/bpf.h>
#include <linux/btf.h>
+#include <linux/lsm_hooks.h>
+#include <linux/bpf_lsm.h>
+
+/* For every LSM hook that allows attachment of BPF programs, declare a NOP
+ * function where a BPF program can be attached as an fexit trampoline.
+ */
+#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
+
+#define LSM_HOOK_int(NAME, ...) \
+noinline __weak int bpf_lsm_##NAME(__VA_ARGS__) \
+{ \
+ return 0; \
+}
+
+#define LSM_HOOK_void(NAME, ...) \
+noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
+
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK
const struct bpf_prog_ops lsm_prog_ops = {
};
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v5 0/8] MAC and Audit policy using eBPF (KRSI)
From: KP Singh @ 2020-03-23 16:44 UTC (permalink / raw)
To: linux-kernel, bpf, linux-security-module
Cc: Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Paul Turner, Jann Horn, Florent Revest, Brendan Jackman,
Greg Kroah-Hartman
From: KP Singh <kpsingh@google.com>
# v4 -> v5
https://lwn.net/Articles/813057/
* Removed static keys and special casing of BPF calls from the LSM
framework.
* Initialized the BPF callbacks (nops) as proper LSM hooks.
* Updated to using the newly introduced BPF_TRAMP_MODIFY_RETURN
trampolines in https://lkml.org/lkml/2020/3/4/877
* Addressed Andrii's feedback and rebased.
# v3 -> v4
* Moved away from allocating a separate security_hook_heads and adding a
new special case for arch_prepare_bpf_trampoline to using BPF fexit
trampolines called from the right place in the LSM hook and toggled by
static keys based on the discussion in:
https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
* Since the code does not deal with security_hook_heads anymore, it goes
from "being a BPF LSM" to "BPF program attachment to LSM hooks".
* Added a new test case which ensures that the BPF programs' return value
is reflected by the LSM hook.
# v2 -> v3 does not change the overall design and has some minor fixes:
* LSM_ORDER_LAST is introduced to represent the behaviour of the BPF LSM
* Fixed the inadvertent clobbering of the LSM Hook error codes
* Added GPL license requirement to the commit log
* The lsm_hook_idx is now the more conventional 0-based index
* Some changes were split into a separate patch ("Load btf_vmlinux only
once per object")
https://lore.kernel.org/bpf/20200117212825.11755-1-kpsingh@chromium.org/
* Addressed Andrii's feedback on the BTF implementation
* Documentation update for using generated vmlinux.h to simplify
programs
* Rebase
# Changes since v1
https://lore.kernel.org/bpf/20191220154208.15895-1-kpsingh@chromium.org
* Eliminate the requirement to maintain LSM hooks separately in
security/bpf/hooks.h Use BPF trampolines to dynamically allocate
security hooks
* Drop the use of securityfs as bpftool provides the required
introspection capabilities. Update the tests to use the bpf_skeleton
and global variables
* Use O_CLOEXEC anonymous fds to represent BPF attachment in line with
the other BPF programs with the possibility to use bpf program pinning
in the future to provide "permanent attachment".
* Drop the logic based on prog names for handling re-attachment.
* Drop bpf_lsm_event_output from this series and send it as a separate
patch.
# Motivation
Google does analysis of rich runtime security data collected from
internal Linux deployments to detect and thwart threats in real-time.
Currently, this is done in custom kernel modules but we would like to
replace this with something that's upstream and useful to others.
The current kernel infrastructure for providing telemetry (Audit, Perf
etc.) is disjoint from access enforcement (i.e. LSMs). Augmenting the
information provided by audit requires kernel changes to audit, its
policy language and user-space components. Furthermore, building a MAC
policy based on the newly added telemetry data requires changes to
various LSMs and their respective policy languages.
This patchset allows BPF programs to be attached to LSM hooks This
facilitates a unified and dynamic (not requiring re-compilation of the
kernel) audit and MAC policy.
# Why an LSM?
Linux Security Modules target security behaviours rather than the
kernel's API. For example, it's easy to miss out a newly added system
call for executing processes (eg. execve, execveat etc.) but the LSM
framework ensures that all process executions trigger the relevant hooks
irrespective of how the process was executed.
Allowing users to implement LSM hooks at runtime also benefits the LSM
eco-system by enabling a quick feedback loop from the security community
about the kind of behaviours that the LSM Framework should be targeting.
# How does it work?
The patchset introduces a new eBPF (https://docs.cilium.io/en/v1.6/bpf/)
program type BPF_PROG_TYPE_LSM which can only be attached to LSM hooks.
Loading and attachment of BPF programs requires CAP_SYS_ADMIN.
The new LSM registers nop functions (bpf_lsm_<hook_name>) as LSM hook
callbacks. Their purpose is to provide a definite point where BPF
programs can be attached as BPF_TRAMP_MODIFY_RETURN trampoline programs
for hooks that return an int, and BPF_TRAMP_FEXIT trampoline programs
for void LSM hooks.
Audit logs can be written using a format chosen by the eBPF program to
the perf events buffer or to global eBPF variables or maps and can be
further processed in user-space.
# BTF Based Design
The current design uses BTF:
* https://facebookmicrosites.github.io/bpf/blog/2018/11/14/btf-enhancement.html
* https://lwn.net/Articles/803258
which allows verifiable read-only structure accesses by field names
rather than fixed offsets. This allows accessing the hook parameters
using a dynamically created context which provides a certain degree of
ABI stability:
// Only declare the structure and fields intended to be used
// in the program
struct vm_area_struct {
unsigned long vm_start;
} __attribute__((preserve_access_index));
// Declare the eBPF program mprotect_audit which attaches to
// to the file_mprotect LSM hook and accepts three arguments.
SEC("lsm/file_mprotect")
int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
unsigned long reqprot, unsigned long prot, int ret)
{
unsigned long vm_start = vma->vm_start;
return 0;
}
By relocating field offsets, BTF makes a large portion of kernel data
structures readily accessible across kernel versions without requiring a
large corpus of BPF helper functions and requiring recompilation with
every kernel version. The BTF type information is also used by the BPF
verifier to validate memory accesses within the BPF program and also
prevents arbitrary writes to the kernel memory.
The limitations of BTF compatibility are described in BPF Co-Re
(http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf, i.e. field
renames, #defines and changes to the signature of LSM hooks). This
design imposes that the MAC policy (eBPF programs) be updated when the
inspected kernel structures change outside of BTF compatibility
guarantees. In practice, this is only required when a structure field
used by a current policy is removed (or renamed) or when the used LSM
hooks change. We expect the maintenance cost of these changes to be
acceptable as compared to the design presented in the RFC.
(https://lore.kernel.org/bpf/20190910115527.5235-1-kpsingh@chromium.org/).
# Usage Examples
A simple example and some documentation is included in the patchset.
In order to better illustrate the capabilities of the framework some
more advanced prototype (not-ready for review) code has also been
published separately:
* Logging execution events (including environment variables and
arguments)
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c
* Detecting deletion of running executables:
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
* Detection of writes to /proc/<pid>/mem:
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c
We have updated Google's internal telemetry infrastructure and have
started deploying this LSM on our Linux Workstations. This gives us more
confidence in the real-world applications of such a system.
KP Singh (8):
bpf: Introduce BPF_PROG_TYPE_LSM
security: Refactor declaration of LSM hooks
bpf: lsm: provide attachment points for BPF LSM programs
bpf: lsm: Implement attach, detach and execution
bpf: lsm: Initialize the BPF LSM hooks
tools/libbpf: Add support for BPF_PROG_TYPE_LSM
bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
bpf: lsm: Add Documentation
Documentation/bpf/bpf_lsm.rst | 150 +++++
Documentation/bpf/index.rst | 1 +
MAINTAINERS | 1 +
include/linux/bpf.h | 7 +
include/linux/bpf_lsm.h | 32 +
include/linux/bpf_types.h | 4 +
include/linux/lsm_hook_names.h | 354 ++++++++++
include/linux/lsm_hooks.h | 622 +-----------------
include/uapi/linux/bpf.h | 2 +
init/Kconfig | 10 +
kernel/bpf/Makefile | 1 +
kernel/bpf/bpf_lsm.c | 65 ++
kernel/bpf/btf.c | 9 +-
kernel/bpf/syscall.c | 26 +-
kernel/bpf/trampoline.c | 17 +-
kernel/bpf/verifier.c | 19 +-
kernel/trace/bpf_trace.c | 12 +-
security/Kconfig | 10 +-
security/Makefile | 2 +
security/bpf/Makefile | 5 +
security/bpf/hooks.c | 55 ++
tools/include/uapi/linux/bpf.h | 2 +
tools/lib/bpf/bpf.c | 3 +-
tools/lib/bpf/libbpf.c | 41 +-
tools/lib/bpf/libbpf.h | 4 +
tools/lib/bpf/libbpf.map | 3 +
tools/lib/bpf/libbpf_probes.c | 1 +
tools/testing/selftests/bpf/lsm_helpers.h | 19 +
.../selftests/bpf/prog_tests/lsm_test.c | 112 ++++
.../selftests/bpf/progs/lsm_int_hook.c | 54 ++
.../selftests/bpf/progs/lsm_void_hook.c | 41 ++
31 files changed, 1038 insertions(+), 646 deletions(-)
create mode 100644 Documentation/bpf/bpf_lsm.rst
create mode 100644 include/linux/bpf_lsm.h
create mode 100644 include/linux/lsm_hook_names.h
create mode 100644 kernel/bpf/bpf_lsm.c
create mode 100644 security/bpf/Makefile
create mode 100644 security/bpf/hooks.c
create mode 100644 tools/testing/selftests/bpf/lsm_helpers.h
create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_test.c
create mode 100644 tools/testing/selftests/bpf/progs/lsm_int_hook.c
create mode 100644 tools/testing/selftests/bpf/progs/lsm_void_hook.c
--
2.20.1
^ permalink raw reply
* [PATCH bpf-next v5 1/7] bpf: Introduce BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-03-23 16:44 UTC (permalink / raw)
To: linux-kernel, bpf, linux-security-module
Cc: Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Paul Turner, Jann Horn, Florent Revest, Brendan Jackman,
Greg Kroah-Hartman
In-Reply-To: <20200323164415.12943-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
Introduce types and configs for bpf programs that can be attached to
LSM hooks. The programs can be enabled by the config option
CONFIG_BPF_LSM.
Signed-off-by: KP Singh <kpsingh@google.com>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
Reviewed-by: Thomas Garnier <thgarnie@google.com>
---
MAINTAINERS | 1 +
include/linux/bpf.h | 3 +++
include/linux/bpf_types.h | 4 ++++
include/uapi/linux/bpf.h | 2 ++
init/Kconfig | 10 ++++++++++
kernel/bpf/Makefile | 1 +
kernel/bpf/bpf_lsm.c | 17 +++++++++++++++++
kernel/trace/bpf_trace.c | 12 ++++++------
tools/include/uapi/linux/bpf.h | 2 ++
tools/lib/bpf/libbpf_probes.c | 1 +
10 files changed, 47 insertions(+), 6 deletions(-)
create mode 100644 kernel/bpf/bpf_lsm.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5dbee41045bc..3197fe9256b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3147,6 +3147,7 @@ R: Martin KaFai Lau <kafai@fb.com>
R: Song Liu <songliubraving@fb.com>
R: Yonghong Song <yhs@fb.com>
R: Andrii Nakryiko <andriin@fb.com>
+R: KP Singh <kpsingh@chromium.org>
L: netdev@vger.kernel.org
L: bpf@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bdb981c204fa..af81ec7b783c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1513,6 +1513,9 @@ extern const struct bpf_func_proto bpf_tcp_sock_proto;
extern const struct bpf_func_proto bpf_jiffies64_proto;
extern const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto;
+const struct bpf_func_proto *bpf_tracing_func_proto(
+ enum bpf_func_id func_id, const struct bpf_prog *prog);
+
/* Shared helpers among cBPF and eBPF. */
void bpf_user_rnd_init_once(void);
u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index c81d4ece79a4..ba0c2d56f8a3 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -70,6 +70,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_STRUCT_OPS, bpf_struct_ops,
void *, void *)
BPF_PROG_TYPE(BPF_PROG_TYPE_EXT, bpf_extension,
void *, void *)
+#ifdef CONFIG_BPF_LSM
+BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
+ void *, void *)
+#endif /* CONFIG_BPF_LSM */
#endif
BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 5d01c5c7e598..9f2673c58788 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -181,6 +181,7 @@ enum bpf_prog_type {
BPF_PROG_TYPE_TRACING,
BPF_PROG_TYPE_STRUCT_OPS,
BPF_PROG_TYPE_EXT,
+ BPF_PROG_TYPE_LSM,
};
enum bpf_attach_type {
@@ -211,6 +212,7 @@ enum bpf_attach_type {
BPF_TRACE_FENTRY,
BPF_TRACE_FEXIT,
BPF_MODIFY_RETURN,
+ BPF_LSM_MAC,
__MAX_BPF_ATTACH_TYPE
};
diff --git a/init/Kconfig b/init/Kconfig
index 20a6ac33761c..b2c9a833bc58 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1616,6 +1616,16 @@ config KALLSYMS_BASE_RELATIVE
# end of the "standard kernel features (expert users)" menu
# syscall, maps, verifier
+
+config BPF_LSM
+ bool "LSM Instrumentation with BPF"
+ depends on BPF_SYSCALL
+ help
+ Enables instrumentation of the security hooks with eBPF programs for
+ implementing dynamic MAC and Audit Policies.
+
+ If you are unsure how to answer this question, answer N.
+
config BPF_SYSCALL
bool "Enable bpf() system call"
select BPF
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 046ce5d98033..f2d7be596966 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -29,4 +29,5 @@ obj-$(CONFIG_DEBUG_INFO_BTF) += sysfs_btf.o
endif
ifeq ($(CONFIG_BPF_JIT),y)
obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o
+obj-${CONFIG_BPF_LSM} += bpf_lsm.o
endif
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
new file mode 100644
index 000000000000..82875039ca90
--- /dev/null
+++ b/kernel/bpf/bpf_lsm.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#include <linux/filter.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+
+const struct bpf_prog_ops lsm_prog_ops = {
+};
+
+const struct bpf_verifier_ops lsm_verifier_ops = {
+ .get_func_proto = bpf_tracing_func_proto,
+ .is_valid_access = btf_ctx_access,
+};
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index e619eedb5919..37ffceab608f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -779,8 +779,8 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
.arg1_type = ARG_ANYTHING,
};
-static const struct bpf_func_proto *
-tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+const struct bpf_func_proto *
+bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
@@ -865,7 +865,7 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_override_return_proto;
#endif
default:
- return tracing_func_proto(func_id, prog);
+ return bpf_tracing_func_proto(func_id, prog);
}
}
@@ -975,7 +975,7 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_tp;
default:
- return tracing_func_proto(func_id, prog);
+ return bpf_tracing_func_proto(func_id, prog);
}
}
@@ -1082,7 +1082,7 @@ pe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_read_branch_records:
return &bpf_read_branch_records_proto;
default:
- return tracing_func_proto(func_id, prog);
+ return bpf_tracing_func_proto(func_id, prog);
}
}
@@ -1210,7 +1210,7 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_raw_tp;
default:
- return tracing_func_proto(func_id, prog);
+ return bpf_tracing_func_proto(func_id, prog);
}
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 5d01c5c7e598..9f2673c58788 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -181,6 +181,7 @@ enum bpf_prog_type {
BPF_PROG_TYPE_TRACING,
BPF_PROG_TYPE_STRUCT_OPS,
BPF_PROG_TYPE_EXT,
+ BPF_PROG_TYPE_LSM,
};
enum bpf_attach_type {
@@ -211,6 +212,7 @@ enum bpf_attach_type {
BPF_TRACE_FENTRY,
BPF_TRACE_FEXIT,
BPF_MODIFY_RETURN,
+ BPF_LSM_MAC,
__MAX_BPF_ATTACH_TYPE
};
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index b782ebef6ac9..2c92059c0c90 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -108,6 +108,7 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
case BPF_PROG_TYPE_TRACING:
case BPF_PROG_TYPE_STRUCT_OPS:
case BPF_PROG_TYPE_EXT:
+ case BPF_PROG_TYPE_LSM:
default:
break;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v8 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-22 1:11 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200322011125.24327-1-longman@redhat.com>
By allocating a kernel buffer with a user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.
Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.
To reduce this possibility, we set a threshold (PAGE_SIZE) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.
To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.
In the extremely unlikely scenario that the key keeps on being changed
and made longer (still <= buflen) in between 2 __keyctl_read_key()
calls, the __keyctl_read_key() calling loop in keyctl_read_key() may
have to be iterated a large number of times, but definitely not infinite.
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/internal.h | 12 +++++++++
security/keys/keyctl.c | 58 +++++++++++++++++++++++++++++-----------
2 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..6d0ca48ae9a5 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
struct iovec;
@@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
#endif
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+ if (addr) {
+ memset((void *)addr, 0, len);
+ kvfree(addr);
+ }
+}
#endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 434ed9defd3a..0062e422e0fd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
- payload = kmalloc(plen, GFP_KERNEL);
+ payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
- kzfree(payload);
+ __kvzfree(payload, plen);
error:
return ret;
}
@@ -827,7 +827,8 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
- char *key_data;
+ char *key_data = NULL;
+ size_t key_data_len;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
@@ -878,24 +879,51 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* Allocating a temporary buffer to hold the keys before
* transferring them to user buffer to avoid potential
* deadlock involving page fault and mmap_sem.
+ *
+ * key_data_len = (buflen <= PAGE_SIZE)
+ * ? buflen : actual length of key data
+ *
+ * This prevents allocating arbitrary large buffer which can
+ * be much larger than the actual key length. In the latter case,
+ * at least 2 passes of this loop is required.
*/
- key_data = kmalloc(buflen, GFP_KERNEL);
+ key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
+ for (;;) {
+ if (key_data_len) {
+ key_data = kvmalloc(key_data_len, GFP_KERNEL);
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto key_put_out;
+ }
+ }
- if (!key_data) {
- ret = -ENOMEM;
- goto key_put_out;
- }
- ret = __keyctl_read_key(key, key_data, buflen);
+ ret = __keyctl_read_key(key, key_data, key_data_len);
+
+ /*
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
+ */
+ if (ret <= 0 || ret > buflen)
+ break;
+
+ /*
+ * The key may change (unlikely) in between 2 consecutive
+ * __keyctl_read_key() calls. In this case, we reallocate
+ * a larger buffer and redo the key read when
+ * key_data_len < ret <= buflen.
+ */
+ if (ret > key_data_len) {
+ if (unlikely(key_data))
+ __kvzfree(key_data, key_data_len);
+ key_data_len = ret;
+ continue; /* Allocate buffer */
+ }
- /*
- * Read methods will just return the required length without
- * any copying if the provided length isn't large enough.
- */
- if (ret > 0 && ret <= buflen) {
if (copy_to_user(buffer, key_data, ret))
ret = -EFAULT;
+ break;
}
- kzfree(key_data);
+ __kvzfree(key_data, key_data_len);
key_put_out:
key_put(key);
--
2.18.1
^ permalink raw reply related
* [PATCH v8 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-22 1:11 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200322011125.24327-1-longman@redhat.com>
A lockdep circular locking dependency report was seen when running a
keyutils test:
[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.
That requires taking out the __user modifier from all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,
1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.
Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 +++------
security/keys/big_key.c | 11 ++--
security/keys/encrypted-keys/encrypted.c | 7 +--
security/keys/keyctl.c | 73 ++++++++++++++++++-----
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +--
security/keys/trusted-keys/trusted_tpm1.c | 14 +----
security/keys/user_defined.c | 5 +-
12 files changed, 85 insertions(+), 74 deletions(-)
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
-extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
+extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
#endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
extern void user_revoke(struct key *key);
extern void user_destroy(struct key *key);
extern void user_describe(const struct key *user, struct seq_file *m);
-extern long user_read(const struct key *key,
- char __user *buffer, size_t buflen);
+extern long user_read(const struct key *key, char *buffer, size_t buflen);
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
{
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
- long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+ long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
* - the key's semaphore is read-locked
*/
static long dns_resolver_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
int err = PTR_ERR(key->payload.data[dns_key_error]);
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
static void rxrpc_destroy(struct key *);
static void rxrpc_destroy_s(struct key *);
static void rxrpc_describe(const struct key *, struct seq_file *);
-static long rxrpc_read(const struct key *, char __user *, size_t);
+static long rxrpc_read(const struct key *, char *, size_t);
/*
* rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
* - this returns the result in XDR form
*/
static long rxrpc_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
const struct rxrpc_key_token *token;
const struct krb5_principal *princ;
size_t size;
- __be32 __user *xdr, *oldxdr;
+ __be32 *xdr, *oldxdr;
u32 cnlen, toksize, ntoks, tok, zero;
u16 toksizes[AFSTOKEN_MAX];
int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
if (!buffer || buflen < size)
return size;
- xdr = (__be32 __user *) buffer;
+ xdr = (__be32 *)buffer;
zero = 0;
#define ENCODE(x) \
do { \
- __be32 y = htonl(x); \
- if (put_user(y, xdr++) < 0) \
- goto fault; \
+ *xdr++ = htonl(x); \
} while(0)
#define ENCODE_DATA(l, s) \
do { \
u32 _l = (l); \
ENCODE(l); \
- if (copy_to_user(xdr, (s), _l) != 0) \
- goto fault; \
- if (_l & 3 && \
- copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
- goto fault; \
+ memcpy(xdr, (s), _l); \
+ if (_l & 3) \
+ memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
xdr += (_l + 3) >> 2; \
} while(0)
#define ENCODE64(x) \
do { \
__be64 y = cpu_to_be64(x); \
- if (copy_to_user(xdr, &y, 8) != 0) \
- goto fault; \
+ memcpy(xdr, &y, 8); \
xdr += 8 >> 2; \
} while(0)
#define ENCODE_STR(s) \
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
ASSERTCMP((char __user *) xdr - buffer, ==, size);
_leave(" = %zu", size);
return size;
-
-fault:
- _leave(" = -EFAULT");
- return -EFAULT;
}
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
- /* copy decrypted data to user */
- if (copy_to_user(buffer, buf->virt, datalen) != 0)
- ret = -EFAULT;
+ /* copy out decrypted data */
+ memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
- if (copy_to_user(buffer, key->payload.data[big_key_data],
- datalen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
- if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
- ret = -EFAULT;
+ memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..434ed9defd3a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
return ret;
}
+/*
+ * Call the read method
+ */
+static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
+{
+ long ret;
+
+ down_read(&key->sem);
+ ret = key_validate(key);
+ if (ret == 0)
+ ret = key->type->read(key, buffer, buflen);
+ up_read(&key->sem);
+ return ret;
+}
+
/*
* Read a key's payload.
*
@@ -812,26 +827,27 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
+ char *key_data;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
if (IS_ERR(key_ref)) {
ret = -ENOKEY;
- goto error;
+ goto out;
}
key = key_ref_to_ptr(key_ref);
ret = key_read_state(key);
if (ret < 0)
- goto error2; /* Negatively instantiated */
+ goto key_put_out; /* Negatively instantiated */
/* see if we can read it directly */
ret = key_permission(key_ref, KEY_NEED_READ);
if (ret == 0)
goto can_read_key;
if (ret != -EACCES)
- goto error2;
+ goto key_put_out;
/* we can't; see if it's searchable from this process's keyrings
* - we automatically take account of the fact that it may be
@@ -839,26 +855,51 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (!is_key_possessed(key_ref)) {
ret = -EACCES;
- goto error2;
+ goto key_put_out;
}
/* the key is probably readable - now try to read it */
can_read_key:
- ret = -EOPNOTSUPP;
- if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
- * to protect against the key being updated or revoked.
- */
- down_read(&key->sem);
- ret = key_validate(key);
- if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
- up_read(&key->sem);
+ if (!key->type->read) {
+ ret = -EOPNOTSUPP;
+ goto key_put_out;
}
-error2:
+ if (!buffer || !buflen) {
+ /* Get the key length from the read method */
+ ret = __keyctl_read_key(key, NULL, 0);
+ goto key_put_out;
+ }
+
+ /*
+ * Read the data with the semaphore held (since we might sleep)
+ * to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ */
+ key_data = kmalloc(buflen, GFP_KERNEL);
+
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto key_put_out;
+ }
+ ret = __keyctl_read_key(key, key_data, buflen);
+
+ /*
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
+ */
+ if (ret > 0 && ret <= buflen) {
+ if (copy_to_user(buffer, key_data, ret))
+ ret = -EFAULT;
+ }
+ kzfree(key_data);
+
+key_put_out:
key_put(key);
-error:
+out:
return ret;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
- ctx->buffer++;
+ *ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
- if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, rka->callout_info, buflen);
}
return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
- char *ascii_buf;
char *bufp;
int i;
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
+ bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
- if (copy_to_user(buffer, upayload->data, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, upayload->data, buflen);
}
return ret;
--
2.18.1
^ permalink raw reply related
* [PATCH v8 0/2] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-22 1:11 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
v8:
- Change the do-while loop in patch 2 to a for loop to make "continue"
work.
v7:
- Restructure code in keyctl_read_key() to reduce nesting.
- Restructure patch 2 to use loop instead of backward jump as suggested
by Jarkko.
v6:
- Make some variable name changes and revise comments as suggested by
Jarkko. No functional change from v5.
v5:
- Merge v4 patches 2 and 3 into 1 to avoid sparse warning. Merge some of
commit logs into patch 1 as well. There is no further change.
v4:
- Remove the __user annotation from big_key_read() and user_read() in
patch 1.
- Add a new patch 2 to remove __user annotation from rxrpc_read().
- Add a new patch 3 to remove __user annotation from dns_resolver_read().
- Merge the original patches 2 and 3 into a single patch 4 and refactor
it as suggested by Jarkko and Eric.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (2):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Avoid false positive ENOMEM error on key read
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 ++----
security/keys/big_key.c | 11 +--
security/keys/encrypted-keys/encrypted.c | 7 +-
security/keys/internal.h | 12 +++
security/keys/keyctl.c | 103 ++++++++++++++++++----
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +-
security/keys/trusted-keys/trusted_tpm1.c | 14 +--
security/keys/user_defined.c | 5 +-
13 files changed, 126 insertions(+), 75 deletions(-)
Code diff from v6:
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ded69108db0d..0062e422e0fd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -827,26 +827,28 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
+ char *key_data = NULL;
+ size_t key_data_len;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
if (IS_ERR(key_ref)) {
ret = -ENOKEY;
- goto error;
+ goto out;
}
key = key_ref_to_ptr(key_ref);
ret = key_read_state(key);
if (ret < 0)
- goto error2; /* Negatively instantiated */
+ goto key_put_out; /* Negatively instantiated */
/* see if we can read it directly */
ret = key_permission(key_ref, KEY_NEED_READ);
if (ret == 0)
goto can_read_key;
if (ret != -EACCES)
- goto error2;
+ goto key_put_out;
/* we can't; see if it's searchable from this process's keyrings
* - we automatically take account of the fact that it may be
@@ -854,75 +856,78 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (!is_key_possessed(key_ref)) {
ret = -EACCES;
- goto error2;
+ goto key_put_out;
}
/* the key is probably readable - now try to read it */
can_read_key:
if (!key->type->read) {
ret = -EOPNOTSUPP;
- goto error2;
+ goto key_put_out;
}
if (!buffer || !buflen) {
/* Get the key length from the read method */
ret = __keyctl_read_key(key, NULL, 0);
- } else {
-
- /*
- * Read the data with the semaphore held (since we might sleep)
- * to protect against the key being updated or revoked.
- *
- * Allocating a temporary buffer to hold the keys before
- * transferring them to user buffer to avoid potential
- * deadlock involving page fault and mmap_sem.
- */
- char *key_data = NULL;
- size_t key_data_len = buflen;
+ goto key_put_out;
+ }
- /*
- * When the user-supplied key length is larger than
- * PAGE_SIZE, we get the actual key length first before
- * allocating a right-sized key data buffer.
- */
- if (buflen <= PAGE_SIZE) {
-allocbuf:
+ /*
+ * Read the data with the semaphore held (since we might sleep)
+ * to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ *
+ * key_data_len = (buflen <= PAGE_SIZE)
+ * ? buflen : actual length of key data
+ *
+ * This prevents allocating arbitrary large buffer which can
+ * be much larger than the actual key length. In the latter case,
+ * at least 2 passes of this loop is required.
+ */
+ key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
+ for (;;) {
+ if (key_data_len) {
key_data = kvmalloc(key_data_len, GFP_KERNEL);
if (!key_data) {
ret = -ENOMEM;
- goto error2;
+ goto key_put_out;
}
}
+
ret = __keyctl_read_key(key, key_data, key_data_len);
/*
- * Read methods will just return the required length
- * without any copying if the provided length isn't big
- * enough.
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
*/
- if (ret > 0 && ret <= buflen) {
- /*
- * The key may change (unlikely) in between 2
- * consecutive __keyctl_read_key() calls. We will
- * need to allocate a larger buffer and redo the key
- * read when key_data_len < ret <= buflen.
- */
- if (!key_data || unlikely(ret > key_data_len)) {
- if (unlikely(key_data))
- __kvzfree(key_data, key_data_len);
- key_data_len = ret;
- goto allocbuf;
- }
+ if (ret <= 0 || ret > buflen)
+ break;
- if (copy_to_user(buffer, key_data, ret))
- ret = -EFAULT;
+ /*
+ * The key may change (unlikely) in between 2 consecutive
+ * __keyctl_read_key() calls. In this case, we reallocate
+ * a larger buffer and redo the key read when
+ * key_data_len < ret <= buflen.
+ */
+ if (ret > key_data_len) {
+ if (unlikely(key_data))
+ __kvzfree(key_data, key_data_len);
+ key_data_len = ret;
+ continue; /* Allocate buffer */
}
- __kvzfree(key_data, key_data_len);
+
+ if (copy_to_user(buffer, key_data, ret))
+ ret = -EFAULT;
+ break;
}
+ __kvzfree(key_data, key_data_len);
-error2:
+key_put_out:
key_put(key);
-error:
+out:
return ret;
}
--
2.18.1
^ permalink raw reply related
* Re: [PATCH v7 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-22 0:46 UTC (permalink / raw)
To: Tetsuo Handa
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski, keyrings,
linux-kernel, linux-security-module, linux-integrity, netdev,
linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <e3d7a227-8915-5c00-cd34-fe2db7fc7121@I-love.SAKURA.ne.jp>
On 3/21/20 8:31 PM, Tetsuo Handa wrote:
> On 2020/03/22 3:49, Waiman Long wrote:
>> + do {
>> + if (ret > key_data_len) {
>> + if (unlikely(key_data))
>> + __kvzfree(key_data, key_data_len);
>> + key_data_len = ret;
>> + continue; /* Allocate buffer */
> Excuse me, but "continue;" inside "do { ... } while (0);" means "break;"
> because "while (0)" is evaluated before continuing the loop.
You are right. My mistake. Will send out a new one for patch 2.
-Longman
^ permalink raw reply
* Re: [PATCH v7 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Tetsuo Handa @ 2020-03-22 0:31 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski, keyrings,
linux-kernel, linux-security-module, linux-integrity, netdev,
linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <20200321184932.16579-3-longman@redhat.com>
On 2020/03/22 3:49, Waiman Long wrote:
> + do {
> + if (ret > key_data_len) {
> + if (unlikely(key_data))
> + __kvzfree(key_data, key_data_len);
> + key_data_len = ret;
> + continue; /* Allocate buffer */
Excuse me, but "continue;" inside "do { ... } while (0);" means "break;"
because "while (0)" is evaluated before continuing the loop.
----------
#include <stdio.h>
int main(int argc, char *argv[])
{
do {
printf("step 1\n");
if (1) {
printf("step 2\n");
continue;
}
printf("step 3\n");
} while (0);
printf("step 4\n");
return 0;
}
----------
----------
step 1
step 2
step 4
----------
> + }
> + } while (0);
^ permalink raw reply
* [PATCH v7 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-21 18:49 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200321184932.16579-1-longman@redhat.com>
A lockdep circular locking dependency report was seen when running a
keyutils test:
[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.
That requires taking out the __user modifier from all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,
1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.
Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 +++------
security/keys/big_key.c | 11 ++--
security/keys/encrypted-keys/encrypted.c | 7 +--
security/keys/keyctl.c | 73 ++++++++++++++++++-----
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +--
security/keys/trusted-keys/trusted_tpm1.c | 14 +----
security/keys/user_defined.c | 5 +-
12 files changed, 85 insertions(+), 74 deletions(-)
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
-extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
+extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
#endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
extern void user_revoke(struct key *key);
extern void user_destroy(struct key *key);
extern void user_describe(const struct key *user, struct seq_file *m);
-extern long user_read(const struct key *key,
- char __user *buffer, size_t buflen);
+extern long user_read(const struct key *key, char *buffer, size_t buflen);
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
{
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
- long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+ long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
* - the key's semaphore is read-locked
*/
static long dns_resolver_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
int err = PTR_ERR(key->payload.data[dns_key_error]);
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
static void rxrpc_destroy(struct key *);
static void rxrpc_destroy_s(struct key *);
static void rxrpc_describe(const struct key *, struct seq_file *);
-static long rxrpc_read(const struct key *, char __user *, size_t);
+static long rxrpc_read(const struct key *, char *, size_t);
/*
* rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
* - this returns the result in XDR form
*/
static long rxrpc_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
const struct rxrpc_key_token *token;
const struct krb5_principal *princ;
size_t size;
- __be32 __user *xdr, *oldxdr;
+ __be32 *xdr, *oldxdr;
u32 cnlen, toksize, ntoks, tok, zero;
u16 toksizes[AFSTOKEN_MAX];
int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
if (!buffer || buflen < size)
return size;
- xdr = (__be32 __user *) buffer;
+ xdr = (__be32 *)buffer;
zero = 0;
#define ENCODE(x) \
do { \
- __be32 y = htonl(x); \
- if (put_user(y, xdr++) < 0) \
- goto fault; \
+ *xdr++ = htonl(x); \
} while(0)
#define ENCODE_DATA(l, s) \
do { \
u32 _l = (l); \
ENCODE(l); \
- if (copy_to_user(xdr, (s), _l) != 0) \
- goto fault; \
- if (_l & 3 && \
- copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
- goto fault; \
+ memcpy(xdr, (s), _l); \
+ if (_l & 3) \
+ memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
xdr += (_l + 3) >> 2; \
} while(0)
#define ENCODE64(x) \
do { \
__be64 y = cpu_to_be64(x); \
- if (copy_to_user(xdr, &y, 8) != 0) \
- goto fault; \
+ memcpy(xdr, &y, 8); \
xdr += 8 >> 2; \
} while(0)
#define ENCODE_STR(s) \
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
ASSERTCMP((char __user *) xdr - buffer, ==, size);
_leave(" = %zu", size);
return size;
-
-fault:
- _leave(" = -EFAULT");
- return -EFAULT;
}
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
- /* copy decrypted data to user */
- if (copy_to_user(buffer, buf->virt, datalen) != 0)
- ret = -EFAULT;
+ /* copy out decrypted data */
+ memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
- if (copy_to_user(buffer, key->payload.data[big_key_data],
- datalen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
- if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
- ret = -EFAULT;
+ memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..434ed9defd3a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
return ret;
}
+/*
+ * Call the read method
+ */
+static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
+{
+ long ret;
+
+ down_read(&key->sem);
+ ret = key_validate(key);
+ if (ret == 0)
+ ret = key->type->read(key, buffer, buflen);
+ up_read(&key->sem);
+ return ret;
+}
+
/*
* Read a key's payload.
*
@@ -812,26 +827,27 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
+ char *key_data;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
if (IS_ERR(key_ref)) {
ret = -ENOKEY;
- goto error;
+ goto out;
}
key = key_ref_to_ptr(key_ref);
ret = key_read_state(key);
if (ret < 0)
- goto error2; /* Negatively instantiated */
+ goto key_put_out; /* Negatively instantiated */
/* see if we can read it directly */
ret = key_permission(key_ref, KEY_NEED_READ);
if (ret == 0)
goto can_read_key;
if (ret != -EACCES)
- goto error2;
+ goto key_put_out;
/* we can't; see if it's searchable from this process's keyrings
* - we automatically take account of the fact that it may be
@@ -839,26 +855,51 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (!is_key_possessed(key_ref)) {
ret = -EACCES;
- goto error2;
+ goto key_put_out;
}
/* the key is probably readable - now try to read it */
can_read_key:
- ret = -EOPNOTSUPP;
- if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
- * to protect against the key being updated or revoked.
- */
- down_read(&key->sem);
- ret = key_validate(key);
- if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
- up_read(&key->sem);
+ if (!key->type->read) {
+ ret = -EOPNOTSUPP;
+ goto key_put_out;
}
-error2:
+ if (!buffer || !buflen) {
+ /* Get the key length from the read method */
+ ret = __keyctl_read_key(key, NULL, 0);
+ goto key_put_out;
+ }
+
+ /*
+ * Read the data with the semaphore held (since we might sleep)
+ * to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ */
+ key_data = kmalloc(buflen, GFP_KERNEL);
+
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto key_put_out;
+ }
+ ret = __keyctl_read_key(key, key_data, buflen);
+
+ /*
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
+ */
+ if (ret > 0 && ret <= buflen) {
+ if (copy_to_user(buffer, key_data, ret))
+ ret = -EFAULT;
+ }
+ kzfree(key_data);
+
+key_put_out:
key_put(key);
-error:
+out:
return ret;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
- ctx->buffer++;
+ *ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
- if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, rka->callout_info, buflen);
}
return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
- char *ascii_buf;
char *bufp;
int i;
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
+ bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
- if (copy_to_user(buffer, upayload->data, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, upayload->data, buflen);
}
return ret;
--
2.18.1
^ permalink raw reply related
* [PATCH v7 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-21 18:49 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200321184932.16579-1-longman@redhat.com>
By allocating a kernel buffer with a user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.
Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.
To reduce this possibility, we set a threshold (PAGE_SIZE) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.
To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.
In the extremely unlikely scenario that the key keeps on being changed
and made longer (still <= buflen) in between 2 __keyctl_read_key()
calls, the __keyctl_read_key() calling loop in keyctl_read_key() may
have to be iterated a large number of times, but definitely not infinite.
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/internal.h | 12 ++++++++
security/keys/keyctl.c | 59 +++++++++++++++++++++++++++++-----------
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..6d0ca48ae9a5 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
struct iovec;
@@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
#endif
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+ if (addr) {
+ memset((void *)addr, 0, len);
+ kvfree(addr);
+ }
+}
#endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 434ed9defd3a..2f72bbe2962b 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
- payload = kmalloc(plen, GFP_KERNEL);
+ payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
- kzfree(payload);
+ __kvzfree(payload, plen);
error:
return ret;
}
@@ -827,7 +827,8 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
- char *key_data;
+ char *key_data = NULL;
+ size_t key_data_len;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
@@ -878,24 +879,50 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* Allocating a temporary buffer to hold the keys before
* transferring them to user buffer to avoid potential
* deadlock involving page fault and mmap_sem.
+ *
+ * key_data_len = (buflen <= PAGE_SIZE)
+ * ? buflen : actual length of key data
+ *
+ * This prevents allocating arbitrary large buffer which can
+ * be much larger than the actual key length. In the latter case,
+ * at least 2 passes of this loop is required.
*/
- key_data = kmalloc(buflen, GFP_KERNEL);
+ key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
+ do {
+ if (key_data_len) {
+ key_data = kvmalloc(key_data_len, GFP_KERNEL);
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto key_put_out;
+ }
+ }
- if (!key_data) {
- ret = -ENOMEM;
- goto key_put_out;
- }
- ret = __keyctl_read_key(key, key_data, buflen);
+ ret = __keyctl_read_key(key, key_data, key_data_len);
+
+ /*
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
+ */
+ if (ret <= 0 || ret > buflen)
+ break;
+
+ /*
+ * The key may change (unlikely) in between 2 consecutive
+ * __keyctl_read_key() calls. In this case, we reallocate
+ * a larger buffer and redo the key read when
+ * key_data_len < ret <= buflen.
+ */
+ if (ret > key_data_len) {
+ if (unlikely(key_data))
+ __kvzfree(key_data, key_data_len);
+ key_data_len = ret;
+ continue; /* Allocate buffer */
+ }
- /*
- * Read methods will just return the required length without
- * any copying if the provided length isn't large enough.
- */
- if (ret > 0 && ret <= buflen) {
if (copy_to_user(buffer, key_data, ret))
ret = -EFAULT;
- }
- kzfree(key_data);
+ } while (0);
+ __kvzfree(key_data, key_data_len);
key_put_out:
key_put(key);
--
2.18.1
^ permalink raw reply related
* [PATCH v7 0/2] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-21 18:49 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
v7:
- Restructure code in keyctl_read_key() to reduce nesting.
- Restructure patch 2 to use loop instead of backward jump as suggested
by Jarkko.
v6:
- Make some variable name changes and revise comments as suggested by
Jarkko. No functional change from v5.
v5:
- Merge v4 patches 2 and 3 into 1 to avoid sparse warning. Merge some of
commit logs into patch 1 as well. There is no further change.
v4:
- Remove the __user annotation from big_key_read() and user_read() in
patch 1.
- Add a new patch 2 to remove __user annotation from rxrpc_read().
- Add a new patch 3 to remove __user annotation from dns_resolver_read().
- Merge the original patches 2 and 3 into a single patch 4 and refactor
it as suggested by Jarkko and Eric.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (2):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Avoid false positive ENOMEM error on key read
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 ++----
security/keys/big_key.c | 11 +--
security/keys/encrypted-keys/encrypted.c | 7 +-
security/keys/internal.h | 12 +++
security/keys/keyctl.c | 104 ++++++++++++++++++----
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +-
security/keys/trusted-keys/trusted_tpm1.c | 14 +--
security/keys/user_defined.c | 5 +-
13 files changed, 126 insertions(+), 76 deletions(-)
Code diff from v6:
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ded69108db0d..2f72bbe2962b 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -827,26 +827,28 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
+ char *key_data = NULL;
+ size_t key_data_len;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
if (IS_ERR(key_ref)) {
ret = -ENOKEY;
- goto error;
+ goto out;
}
key = key_ref_to_ptr(key_ref);
ret = key_read_state(key);
if (ret < 0)
- goto error2; /* Negatively instantiated */
+ goto key_put_out; /* Negatively instantiated */
/* see if we can read it directly */
ret = key_permission(key_ref, KEY_NEED_READ);
if (ret == 0)
goto can_read_key;
if (ret != -EACCES)
- goto error2;
+ goto key_put_out;
/* we can't; see if it's searchable from this process's keyrings
* - we automatically take account of the fact that it may be
@@ -854,75 +856,77 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (!is_key_possessed(key_ref)) {
ret = -EACCES;
- goto error2;
+ goto key_put_out;
}
/* the key is probably readable - now try to read it */
can_read_key:
if (!key->type->read) {
ret = -EOPNOTSUPP;
- goto error2;
+ goto key_put_out;
}
if (!buffer || !buflen) {
/* Get the key length from the read method */
ret = __keyctl_read_key(key, NULL, 0);
- } else {
-
- /*
- * Read the data with the semaphore held (since we might sleep)
- * to protect against the key being updated or revoked.
- *
- * Allocating a temporary buffer to hold the keys before
- * transferring them to user buffer to avoid potential
- * deadlock involving page fault and mmap_sem.
- */
- char *key_data = NULL;
- size_t key_data_len = buflen;
+ goto key_put_out;
+ }
- /*
- * When the user-supplied key length is larger than
- * PAGE_SIZE, we get the actual key length first before
- * allocating a right-sized key data buffer.
- */
- if (buflen <= PAGE_SIZE) {
-allocbuf:
+ /*
+ * Read the data with the semaphore held (since we might sleep)
+ * to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ *
+ * key_data_len = (buflen <= PAGE_SIZE)
+ * ? buflen : actual length of key data
+ *
+ * This prevents allocating arbitrary large buffer which can
+ * be much larger than the actual key length. In the latter case,
+ * at least 2 passes of this loop is required.
+ */
+ key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
+ do {
+ if (key_data_len) {
key_data = kvmalloc(key_data_len, GFP_KERNEL);
if (!key_data) {
ret = -ENOMEM;
- goto error2;
+ goto key_put_out;
}
}
+
ret = __keyctl_read_key(key, key_data, key_data_len);
/*
- * Read methods will just return the required length
- * without any copying if the provided length isn't big
- * enough.
+ * Read methods will just return the required length without
+ * any copying if the provided length isn't large enough.
*/
- if (ret > 0 && ret <= buflen) {
- /*
- * The key may change (unlikely) in between 2
- * consecutive __keyctl_read_key() calls. We will
- * need to allocate a larger buffer and redo the key
- * read when key_data_len < ret <= buflen.
- */
- if (!key_data || unlikely(ret > key_data_len)) {
- if (unlikely(key_data))
- __kvzfree(key_data, key_data_len);
- key_data_len = ret;
- goto allocbuf;
- }
+ if (ret <= 0 || ret > buflen)
+ break;
- if (copy_to_user(buffer, key_data, ret))
- ret = -EFAULT;
+ /*
+ * The key may change (unlikely) in between 2 consecutive
+ * __keyctl_read_key() calls. In this case, we reallocate
+ * a larger buffer and redo the key read when
+ * key_data_len < ret <= buflen.
+ */
+ if (ret > key_data_len) {
+ if (unlikely(key_data))
+ __kvzfree(key_data, key_data_len);
+ key_data_len = ret;
+ continue; /* Allocate buffer */
}
- __kvzfree(key_data, key_data_len);
- }
-error2:
+ if (copy_to_user(buffer, key_data, ret))
+ ret = -EFAULT;
+ } while (0);
+ __kvzfree(key_data, key_data_len);
+
+key_put_out:
key_put(key);
-error:
+out:
return ret;
}
--
2.18.1
^ permalink raw reply related
* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-21 1:51 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320221918.GA5284@linux.intel.com>
On 3/20/20 6:19 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 20, 2020 at 03:19:03PM -0400, Waiman Long wrote:
>> By allocating a kernel buffer with a user-supplied buffer length, it
>> is possible that a false positive ENOMEM error may be returned because
>> the user-supplied length is just too large even if the system do have
>> enough memory to hold the actual key data.
>>
>> Moreover, if the buffer length is larger than the maximum amount of
>> memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
>> pages), a warning message will also be printed.
>>
>> To reduce this possibility, we set a threshold (page size) over which we
>> do check the actual key length first before allocating a buffer of the
>> right size to hold it. The threshold is arbitrary, it is just used to
>> trigger a buffer length check. It does not limit the actual key length
>> as long as there is enough memory to satisfy the memory request.
>>
>> To further avoid large buffer allocation failure due to page
>> fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
>> pages can be used when there is not a large enough contiguous set of
>> pages available for allocation.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>> security/keys/internal.h | 12 ++++++++++++
>> security/keys/keyctl.c | 39 +++++++++++++++++++++++++++++++--------
>> 2 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/security/keys/internal.h b/security/keys/internal.h
>> index ba3e2da14cef..6d0ca48ae9a5 100644
>> --- a/security/keys/internal.h
>> +++ b/security/keys/internal.h
>> @@ -16,6 +16,8 @@
>> #include <linux/keyctl.h>
>> #include <linux/refcount.h>
>> #include <linux/compat.h>
>> +#include <linux/mm.h>
>> +#include <linux/vmalloc.h>
>>
>> struct iovec;
>>
>> @@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
>>
>> #endif
>>
>> +/*
>> + * Helper function to clear and free a kvmalloc'ed memory object.
>> + */
>> +static inline void __kvzfree(const void *addr, size_t len)
>> +{
>> + if (addr) {
>> + memset((void *)addr, 0, len);
>> + kvfree(addr);
>> + }
>> +}
>> #endif /* _INTERNAL_H */
>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> index 5a0794cb8815..ded69108db0d 100644
>> --- a/security/keys/keyctl.c
>> +++ b/security/keys/keyctl.c
>> @@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
>> payload = NULL;
>> if (plen) {
>> ret = -ENOMEM;
>> - payload = kmalloc(plen, GFP_KERNEL);
>> + payload = kvmalloc(plen, GFP_KERNEL);
>> if (!payload)
>> goto error;
>>
>> @@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
>>
>> key_ref_put(key_ref);
>> error2:
>> - kzfree(payload);
>> + __kvzfree(payload, plen);
>> error:
>> return ret;
>> }
>> @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>> * transferring them to user buffer to avoid potential
>> * deadlock involving page fault and mmap_sem.
>> */
>> - char *key_data = kmalloc(buflen, GFP_KERNEL);
>> + char *key_data = NULL;
>> + size_t key_data_len = buflen;
>>
>> - if (!key_data) {
>> - ret = -ENOMEM;
>> - goto error2;
>> + /*
>> + * When the user-supplied key length is larger than
>> + * PAGE_SIZE, we get the actual key length first before
>> + * allocating a right-sized key data buffer.
>> + */
>> + if (buflen <= PAGE_SIZE) {
>> +allocbuf:
> Would move this label before condition instead of jumping inside the
> nested block since it will always evaluate correctly.
Yes, you are right. That was not the case for initial version and I
didn't recheck it.
> To this version haven't really gotten why you don't use a legit loop
> construct but instead jump from one random nested location to another
> random nested location? This construct will be somewhat nasty to
> maintain. The construct is weird enough that you should have rather
> good explanation in the long description why such a mess.
I did that to avoid deep nesting. I can rewrite it to remove the the
goto statement.
>
>> + key_data = kvmalloc(key_data_len, GFP_KERNEL);
>> + if (!key_data) {
>> + ret = -ENOMEM;
>> + goto error2;
>> + }
>> }
>> - ret = __keyctl_read_key(key, key_data, buflen);
>> + ret = __keyctl_read_key(key, key_data, key_data_len);
>>
>> /*
>> * Read methods will just return the required length
>> @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>> * enough.
>> */
>> if (ret > 0 && ret <= buflen) {
>> + /*
>> + * The key may change (unlikely) in between 2
>> + * consecutive __keyctl_read_key() calls. We will
>> + * need to allocate a larger buffer and redo the key
>> + * read when key_data_len < ret <= buflen.
>> + */
>> + if (!key_data || unlikely(ret > key_data_len)) {
>> + if (unlikely(key_data))
>> + __kvzfree(key_data, key_data_len);
>> + key_data_len = ret;
>> + goto allocbuf;
>> + }
>> +
>> if (copy_to_user(buffer, key_data, ret))
>> ret = -EFAULT;
>> }
>> - kzfree(key_data);
>> + __kvzfree(key_data, key_data_len);
>> }
>>
>> error2:
>> --
>> 2.18.1
>>
> Doesn't this go to infinite loop if actual key size is at least
> PAGE_SIZE + 1? Where is the guarantee that this cannot happen?
I think you may have the wrong impression that it caps the buffer length
to PAGE_SIZE. That is not true. key_data_len can be greater than
PAGE_SIZE. I run tests that include one that creates a big key of almost
1Mb. So for buflen <= PAGE_SIZE, key_data_len = buflen. For buflen >
PAGE_SIZE, key_data_len = the actual key length which can be >
PAGE_SIZE. This patch just tries to avoid allocating arbitrary large
buffer with a length much larger than the actual key length.
I will try to make the code easier to read.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-21 0:58 UTC (permalink / raw)
To: David Howells
Cc: Waiman Long, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <3984029.1584748510@warthog.procyon.org.uk>
On Fri, Mar 20, 2020 at 11:55:10PM +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
> > /* Key data can change as we don not hold key->sem. */
>
> I think you mean "we don't".
Oops, typo. Yes, thanks.
/Jarkko
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: David Howells @ 2020-03-20 23:55 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, Waiman Long, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320143547.GB3629@linux.intel.com>
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> /* Key data can change as we don not hold key->sem. */
I think you mean "we don't".
David
^ permalink raw reply
* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-20 22:21 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320221918.GA5284@linux.intel.com>
On Sat, Mar 21, 2020 at 12:19:27AM +0200, Jarkko Sakkinen wrote:
> Would move this label before condition instead of jumping inside the
> nested block since it will always evaluate correctly.
>
> To this version haven't really gotten why you don't use a legit loop
> construct but instead jump from one random nested location to another
> random nested location? This construct will be somewhat nasty to
> maintain. The construct is weird enough that you should have rather
> good explanation in the long description why such a mess.
What I'm saying that if I fix a bug, the first version of the fix
would probably look something like this is right now. They I think
how to write it right. We don't want fixes that just happen to work.
Right now I'm worried to take this in since I'm not confident that
I haven't some possible corner case, or might still have gotten
something just plain wrong.
/Jarkko
^ permalink raw reply
* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-20 22:19 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320191903.19494-3-longman@redhat.com>
On Fri, Mar 20, 2020 at 03:19:03PM -0400, Waiman Long wrote:
> By allocating a kernel buffer with a user-supplied buffer length, it
> is possible that a false positive ENOMEM error may be returned because
> the user-supplied length is just too large even if the system do have
> enough memory to hold the actual key data.
>
> Moreover, if the buffer length is larger than the maximum amount of
> memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
> pages), a warning message will also be printed.
>
> To reduce this possibility, we set a threshold (page size) over which we
> do check the actual key length first before allocating a buffer of the
> right size to hold it. The threshold is arbitrary, it is just used to
> trigger a buffer length check. It does not limit the actual key length
> as long as there is enough memory to satisfy the memory request.
>
> To further avoid large buffer allocation failure due to page
> fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
> pages can be used when there is not a large enough contiguous set of
> pages available for allocation.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> security/keys/internal.h | 12 ++++++++++++
> security/keys/keyctl.c | 39 +++++++++++++++++++++++++++++++--------
> 2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index ba3e2da14cef..6d0ca48ae9a5 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -16,6 +16,8 @@
> #include <linux/keyctl.h>
> #include <linux/refcount.h>
> #include <linux/compat.h>
> +#include <linux/mm.h>
> +#include <linux/vmalloc.h>
>
> struct iovec;
>
> @@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
>
> #endif
>
> +/*
> + * Helper function to clear and free a kvmalloc'ed memory object.
> + */
> +static inline void __kvzfree(const void *addr, size_t len)
> +{
> + if (addr) {
> + memset((void *)addr, 0, len);
> + kvfree(addr);
> + }
> +}
> #endif /* _INTERNAL_H */
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 5a0794cb8815..ded69108db0d 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
> payload = NULL;
> if (plen) {
> ret = -ENOMEM;
> - payload = kmalloc(plen, GFP_KERNEL);
> + payload = kvmalloc(plen, GFP_KERNEL);
> if (!payload)
> goto error;
>
> @@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
>
> key_ref_put(key_ref);
> error2:
> - kzfree(payload);
> + __kvzfree(payload, plen);
> error:
> return ret;
> }
> @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
> * transferring them to user buffer to avoid potential
> * deadlock involving page fault and mmap_sem.
> */
> - char *key_data = kmalloc(buflen, GFP_KERNEL);
> + char *key_data = NULL;
> + size_t key_data_len = buflen;
>
> - if (!key_data) {
> - ret = -ENOMEM;
> - goto error2;
> + /*
> + * When the user-supplied key length is larger than
> + * PAGE_SIZE, we get the actual key length first before
> + * allocating a right-sized key data buffer.
> + */
> + if (buflen <= PAGE_SIZE) {
> +allocbuf:
Would move this label before condition instead of jumping inside the
nested block since it will always evaluate correctly.
To this version haven't really gotten why you don't use a legit loop
construct but instead jump from one random nested location to another
random nested location? This construct will be somewhat nasty to
maintain. The construct is weird enough that you should have rather
good explanation in the long description why such a mess.
> + key_data = kvmalloc(key_data_len, GFP_KERNEL);
> + if (!key_data) {
> + ret = -ENOMEM;
> + goto error2;
> + }
> }
> - ret = __keyctl_read_key(key, key_data, buflen);
> + ret = __keyctl_read_key(key, key_data, key_data_len);
>
> /*
> * Read methods will just return the required length
> @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
> * enough.
> */
> if (ret > 0 && ret <= buflen) {
> + /*
> + * The key may change (unlikely) in between 2
> + * consecutive __keyctl_read_key() calls. We will
> + * need to allocate a larger buffer and redo the key
> + * read when key_data_len < ret <= buflen.
> + */
> + if (!key_data || unlikely(ret > key_data_len)) {
> + if (unlikely(key_data))
> + __kvzfree(key_data, key_data_len);
> + key_data_len = ret;
> + goto allocbuf;
> + }
> +
> if (copy_to_user(buffer, key_data, ret))
> ret = -EFAULT;
> }
> - kzfree(key_data);
> + __kvzfree(key_data, key_data_len);
> }
>
> error2:
> --
> 2.18.1
>
Doesn't this go to infinite loop if actual key size is at least
PAGE_SIZE + 1? Where is the guarantee that this cannot happen?
/Jarkko
^ permalink raw reply
* [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200320191903.19494-1-longman@redhat.com>
A lockdep circular locking dependency report was seen when running a
keyutils test:
[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.
That requires taking out the __user modifier from all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,
1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.
Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 ++++-------
security/keys/big_key.c | 11 ++---
security/keys/encrypted-keys/encrypted.c | 7 ++-
security/keys/keyctl.c | 57 +++++++++++++++++++----
security/keys/keyring.c | 6 +--
security/keys/request_key_auth.c | 7 ++-
security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
security/keys/user_defined.c | 5 +-
12 files changed, 77 insertions(+), 66 deletions(-)
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
-extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
+extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
#endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
extern void user_revoke(struct key *key);
extern void user_destroy(struct key *key);
extern void user_describe(const struct key *user, struct seq_file *m);
-extern long user_read(const struct key *key,
- char __user *buffer, size_t buflen);
+extern long user_read(const struct key *key, char *buffer, size_t buflen);
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
{
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
- long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+ long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
* - the key's semaphore is read-locked
*/
static long dns_resolver_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
int err = PTR_ERR(key->payload.data[dns_key_error]);
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
static void rxrpc_destroy(struct key *);
static void rxrpc_destroy_s(struct key *);
static void rxrpc_describe(const struct key *, struct seq_file *);
-static long rxrpc_read(const struct key *, char __user *, size_t);
+static long rxrpc_read(const struct key *, char *, size_t);
/*
* rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
* - this returns the result in XDR form
*/
static long rxrpc_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
const struct rxrpc_key_token *token;
const struct krb5_principal *princ;
size_t size;
- __be32 __user *xdr, *oldxdr;
+ __be32 *xdr, *oldxdr;
u32 cnlen, toksize, ntoks, tok, zero;
u16 toksizes[AFSTOKEN_MAX];
int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
if (!buffer || buflen < size)
return size;
- xdr = (__be32 __user *) buffer;
+ xdr = (__be32 *)buffer;
zero = 0;
#define ENCODE(x) \
do { \
- __be32 y = htonl(x); \
- if (put_user(y, xdr++) < 0) \
- goto fault; \
+ *xdr++ = htonl(x); \
} while(0)
#define ENCODE_DATA(l, s) \
do { \
u32 _l = (l); \
ENCODE(l); \
- if (copy_to_user(xdr, (s), _l) != 0) \
- goto fault; \
- if (_l & 3 && \
- copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
- goto fault; \
+ memcpy(xdr, (s), _l); \
+ if (_l & 3) \
+ memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
xdr += (_l + 3) >> 2; \
} while(0)
#define ENCODE64(x) \
do { \
__be64 y = cpu_to_be64(x); \
- if (copy_to_user(xdr, &y, 8) != 0) \
- goto fault; \
+ memcpy(xdr, &y, 8); \
xdr += 8 >> 2; \
} while(0)
#define ENCODE_STR(s) \
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
ASSERTCMP((char __user *) xdr - buffer, ==, size);
_leave(" = %zu", size);
return size;
-
-fault:
- _leave(" = -EFAULT");
- return -EFAULT;
}
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
- /* copy decrypted data to user */
- if (copy_to_user(buffer, buf->virt, datalen) != 0)
- ret = -EFAULT;
+ /* copy out decrypted data */
+ memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
- if (copy_to_user(buffer, key->payload.data[big_key_data],
- datalen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
- if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
- ret = -EFAULT;
+ memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..5a0794cb8815 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
return ret;
}
+/*
+ * Call the read method
+ */
+static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
+{
+ long ret;
+
+ down_read(&key->sem);
+ ret = key_validate(key);
+ if (ret == 0)
+ ret = key->type->read(key, buffer, buflen);
+ up_read(&key->sem);
+ return ret;
+}
+
/*
* Read a key's payload.
*
@@ -844,16 +859,42 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
/* the key is probably readable - now try to read it */
can_read_key:
- ret = -EOPNOTSUPP;
- if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
+ if (!key->type->read) {
+ ret = -EOPNOTSUPP;
+ goto error2;
+ }
+
+ if (!buffer || !buflen) {
+ /* Get the key length from the read method */
+ ret = __keyctl_read_key(key, NULL, 0);
+ } else {
+
+ /*
+ * Read the data with the semaphore held (since we might sleep)
* to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ */
+ char *key_data = kmalloc(buflen, GFP_KERNEL);
+
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto error2;
+ }
+ ret = __keyctl_read_key(key, key_data, buflen);
+
+ /*
+ * Read methods will just return the required length
+ * without any copying if the provided length isn't big
+ * enough.
*/
- down_read(&key->sem);
- ret = key_validate(key);
- if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
- up_read(&key->sem);
+ if (ret > 0 && ret <= buflen) {
+ if (copy_to_user(buffer, key_data, ret))
+ ret = -EFAULT;
+ }
+ kzfree(key_data);
}
error2:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
- ctx->buffer++;
+ *ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
- if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, rka->callout_info, buflen);
}
return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
- char *ascii_buf;
char *bufp;
int i;
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
+ bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
- if (copy_to_user(buffer, upayload->data, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, upayload->data, buflen);
}
return ret;
--
2.18.1
^ permalink raw reply related
* [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
v6:
- Make some variable name changes and revise comments as suggested by
Jarkko. No functional change from v5.
v5:
- Merge v4 patches 2 and 3 into 1 to avoid sparse warning. Merge some of
commit logs into patch 1 as well. There is no further change.
v4:
- Remove the __user annotation from big_key_read() and user_read() in
patch 1.
- Add a new patch 2 to remove __user annotation from rxrpc_read().
- Add a new patch 3 to remove __user annotation from dns_resolver_read().
- Merge the original patches 2 and 3 into a single patch 4 and refactor
it as suggested by Jarkko and Eric.
v3:
- Reorganize the keyctl_read_key() code to make it more readable as
suggested by Jarkko Sakkinen.
- Add patch 3 to use kvmalloc() for safer large buffer allocation as
suggested by David Howells.
v2:
- Handle NULL buffer and buflen properly in patch 1.
- Fix a bug in big_key.c.
- Add patch 2 to handle arbitrary large user-supplied buflen.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (2):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Avoid false positive ENOMEM error on key read
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 +++-----
security/keys/big_key.c | 11 ++-
security/keys/encrypted-keys/encrypted.c | 7 +-
security/keys/internal.h | 12 ++++
security/keys/keyctl.c | 84 ++++++++++++++++++++---
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +-
security/keys/trusted-keys/trusted_tpm1.c | 14 +---
security/keys/user_defined.c | 5 +-
13 files changed, 114 insertions(+), 68 deletions(-)
--
2.18.1
^ permalink raw reply
* [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200320191903.19494-1-longman@redhat.com>
By allocating a kernel buffer with a user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.
Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.
To reduce this possibility, we set a threshold (page size) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.
To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/internal.h | 12 ++++++++++++
security/keys/keyctl.c | 39 +++++++++++++++++++++++++++++++--------
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..6d0ca48ae9a5 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
struct iovec;
@@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
#endif
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+ if (addr) {
+ memset((void *)addr, 0, len);
+ kvfree(addr);
+ }
+}
#endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 5a0794cb8815..ded69108db0d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
- payload = kmalloc(plen, GFP_KERNEL);
+ payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
- kzfree(payload);
+ __kvzfree(payload, plen);
error:
return ret;
}
@@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* transferring them to user buffer to avoid potential
* deadlock involving page fault and mmap_sem.
*/
- char *key_data = kmalloc(buflen, GFP_KERNEL);
+ char *key_data = NULL;
+ size_t key_data_len = buflen;
- if (!key_data) {
- ret = -ENOMEM;
- goto error2;
+ /*
+ * When the user-supplied key length is larger than
+ * PAGE_SIZE, we get the actual key length first before
+ * allocating a right-sized key data buffer.
+ */
+ if (buflen <= PAGE_SIZE) {
+allocbuf:
+ key_data = kvmalloc(key_data_len, GFP_KERNEL);
+ if (!key_data) {
+ ret = -ENOMEM;
+ goto error2;
+ }
}
- ret = __keyctl_read_key(key, key_data, buflen);
+ ret = __keyctl_read_key(key, key_data, key_data_len);
/*
* Read methods will just return the required length
@@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* enough.
*/
if (ret > 0 && ret <= buflen) {
+ /*
+ * The key may change (unlikely) in between 2
+ * consecutive __keyctl_read_key() calls. We will
+ * need to allocate a larger buffer and redo the key
+ * read when key_data_len < ret <= buflen.
+ */
+ if (!key_data || unlikely(ret > key_data_len)) {
+ if (unlikely(key_data))
+ __kvzfree(key_data, key_data_len);
+ key_data_len = ret;
+ goto allocbuf;
+ }
+
if (copy_to_user(buffer, key_data, ret))
ret = -EFAULT;
}
- kzfree(key_data);
+ __kvzfree(key_data, key_data_len);
}
error2:
--
2.18.1
^ permalink raw reply related
* Re: [PATCH] security, keys: Optimize barrier usage for Rmw atomic bitops
From: Davidlohr Bueso @ 2020-03-20 17:09 UTC (permalink / raw)
To: dhowells; +Cc: linux-security-module, linux-kernel, Davidlohr Bueso
In-Reply-To: <20200129180625.24486-1-dave@stgolabs.net>
ping?
On Wed, 29 Jan 2020, Davidlohr Bueso wrote:
>For both set and clear_bit, we can avoid the unnecessary barriers
>on non LL/SC architectures, such as x86. Instead, use the
>smp_mb__{before,after}_atomic() calls.
>
>Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>---
> security/keys/gc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/security/keys/gc.c b/security/keys/gc.c
>index 671dd730ecfc..ce7b4c22e3c4 100644
>--- a/security/keys/gc.c
>+++ b/security/keys/gc.c
>@@ -102,7 +102,7 @@ void key_gc_keytype(struct key_type *ktype)
>
> key_gc_dead_keytype = ktype;
> set_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
>- smp_mb();
>+ smp_mb__after_atomic();
> set_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags);
>
> kdebug("schedule");
>@@ -308,7 +308,7 @@ static void key_garbage_collector(struct work_struct *work)
>
> if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
> kdebug("dead wake");
>- smp_mb();
>+ smp_mb__before_atomic();
> clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
> }
>--
>2.16.4
>
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-20 15:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320143547.GB3629@linux.intel.com>
On 3/20/20 10:35 AM, Jarkko Sakkinen wrote:
> On Fri, Mar 20, 2020 at 09:27:03AM -0400, Waiman Long wrote:
>> On 3/19/20 10:07 PM, Jarkko Sakkinen wrote:
>>> On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote:
>>>> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote:
>>>>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
>>>>>> + * It is possible, though unlikely, that the key
>>>>>> + * changes in between the up_read->down_read period.
>>>>>> + * If the key becomes longer, we will have to
>>>>>> + * allocate a larger buffer and redo the key read
>>>>>> + * again.
>>>>>> + */
>>>>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>>>>> Shouldn't you check that tmpbuflen stays below buflen (why else
>>>>> you had made copy of buflen otherwise)?
>>>> The check above this thunk:
>>>>
>>>> if ((ret > 0) && (ret <= buflen)) {
>>>>
>>>> will make sure that ret will not be larger than buflen. So tmpbuflen > >> will never be bigger than buflen. > > Ah right, of course, thanks.
>>> What would go wrong if the condition was instead
>>> ((ret > 0) && (ret <= tmpbuflen))?
>> That if statement is a check to see if the actual key length is longer
>> than the user-supplied buffer (buflen). If that is the case, it will
>> just return the expected length without storing anything into the user
>> buffer. For the case that buflen >= ret > tmpbuflen, the revised check
>> above will incorrectly skip the storing step causing the caller to
>> incorrectly think the key is there in the buffer.
>>
>> Maybe I should clarify that a bit more in the comment.
> OK, right because it is possible in-between tmpbuflen could be
> larger. Got it.
>
> I think that longish key_data and key_data_len would be better
> names than tmpbuf and tpmbuflen.
>
> Also the comments are somewat overkill IMHO.
>
> I'd replace them along the lines of
>
> /* Cap the user supplied buffer length to PAGE_SIZE. */
>
> /* Key data can change as we don not hold key->sem. */
I am fine with the rename, will sent out a v6 soon.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-20 14:35 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <7dbc524f-6c16-026a-a372-2e80b40eab30@redhat.com>
On Fri, Mar 20, 2020 at 09:27:03AM -0400, Waiman Long wrote:
> On 3/19/20 10:07 PM, Jarkko Sakkinen wrote:
> > On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote:
> >> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote:
> >>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
> >>>> + * It is possible, though unlikely, that the key
> >>>> + * changes in between the up_read->down_read period.
> >>>> + * If the key becomes longer, we will have to
> >>>> + * allocate a larger buffer and redo the key read
> >>>> + * again.
> >>>> + */
> >>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
> >>> Shouldn't you check that tmpbuflen stays below buflen (why else
> >>> you had made copy of buflen otherwise)?
> >> The check above this thunk:
> >>
> >> if ((ret > 0) && (ret <= buflen)) {
> >>
> >> will make sure that ret will not be larger than buflen. So tmpbuflen > >> will never be bigger than buflen. > > Ah right, of course, thanks.
> >
> > What would go wrong if the condition was instead
> > ((ret > 0) && (ret <= tmpbuflen))?
>
> That if statement is a check to see if the actual key length is longer
> than the user-supplied buffer (buflen). If that is the case, it will
> just return the expected length without storing anything into the user
> buffer. For the case that buflen >= ret > tmpbuflen, the revised check
> above will incorrectly skip the storing step causing the caller to
> incorrectly think the key is there in the buffer.
>
> Maybe I should clarify that a bit more in the comment.
OK, right because it is possible in-between tmpbuflen could be
larger. Got it.
I think that longish key_data and key_data_len would be better
names than tmpbuf and tpmbuflen.
Also the comments are somewat overkill IMHO.
I'd replace them along the lines of
/* Cap the user supplied buffer length to PAGE_SIZE. */
/* Key data can change as we don not hold key->sem. */
/Jarkko
^ permalink raw reply
* Re: [PATCH v5 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-20 13:56 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <3251035.1584692419@warthog.procyon.org.uk>
On 3/20/20 4:20 AM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> + if ((ret > 0) && (ret <= buflen)) {
> That's a bit excessive on the bracketage, btw, but don't worry about it unless
> you respin the patches.
Got it.
Thanks,
Longman
^ 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