* [PATCH v7 08/11] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
If "hidepid=4" mount option is set then do not instantiate pids that
we can not ptrace. "hidepid=4" means that procfs should only contain
pids that the caller can ptrace.
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 15 +++++++++++++++
fs/proc/root.c | 14 +++++++++++---
include/linux/proc_fs.h | 1 +
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index f4f1bcb28603..b55d205a7f6e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -699,6 +699,14 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
struct task_struct *task,
int hide_pid_min)
{
+ /*
+ * If 'hidpid' mount option is set force a ptrace check,
+ * we indicate that we are using a filesystem syscall
+ * by passing PTRACE_MODE_READ_FSCREDS
+ */
+ if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE)
+ return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+
if (proc_fs_hide_pid(fs_info) < hide_pid_min)
return true;
if (in_group_p(proc_fs_pid_gid(fs_info)))
@@ -3274,7 +3282,14 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
if (!task)
goto out;
+ /* Limit procfs to only ptracable tasks */
+ if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE) {
+ if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
+ goto out_put_task;
+ }
+
result = proc_pid_instantiate(dentry, task, NULL);
+out_put_task:
put_task_struct(task);
out:
return result;
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 3bb8df360cf7..57276cb65528 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -52,6 +52,15 @@ static const struct fs_parameter_description proc_fs_parameters = {
.specs = proc_param_specs,
};
+static inline int
+valid_hidepid(unsigned int value)
+{
+ return (value == HIDEPID_OFF ||
+ value == HIDEPID_NO_ACCESS ||
+ value == HIDEPID_INVISIBLE ||
+ value == HIDEPID_NOT_PTRACABLE);
+}
+
static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
{
struct proc_fs_context *ctx = fc->fs_private;
@@ -68,10 +77,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
break;
case Opt_hidepid:
+ if (!valid_hidepid(result.uint_32))
+ return invalf(fc, "proc: unknown value of hidepid.\n");
ctx->hidepid = result.uint_32;
- if (ctx->hidepid < HIDEPID_OFF ||
- ctx->hidepid > HIDEPID_INVISIBLE)
- return invalf(fc, "proc: hidepid value must be between 0 and 2.\n");
break;
default:
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index f307940f8311..6822548405a7 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -17,6 +17,7 @@ enum {
HIDEPID_OFF = 0,
HIDEPID_NO_ACCESS = 1,
HIDEPID_INVISIBLE = 2,
+ HIDEPID_NOT_PTRACABLE = 4, /* Limit pids to only ptracable pids */
};
struct proc_fs_info {
--
2.24.1
^ permalink raw reply related
* [PATCH v7 06/11] proc: support mounting procfs instances inside same pid namespace
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This patch allows to have multiple procfs instances inside the
same pid namespace. The aim here is lightweight sandboxes, and to allow
that we have to modernize procfs internals.
1) The main aim of this work is to have on embedded systems one
supervisor for apps. Right now we have some lightweight sandbox support,
however if we create pid namespacess we have to manages all the
processes inside too, where our goal is to be able to run a bunch of
apps each one inside its own mount namespace without being able to
notice each other. We only want to use mount namespaces, and we want
procfs to behave more like a real mount point.
2) Linux Security Modules have multiple ptrace paths inside some
subsystems, however inside procfs, the implementation does not guarantee
that the ptrace() check which triggers the security_ptrace_check() hook
will always run. We have the 'hidepid' mount option that can be used to
force the ptrace_may_access() check inside has_pid_permissions() to run.
The problem is that 'hidepid' is per pid namespace and not attached to
the mount point, any remount or modification of 'hidepid' will propagate
to all other procfs mounts.
This also does not allow to support Yama LSM easily in desktop and user
sessions. Yama ptrace scope which restricts ptrace and some other
syscalls to be allowed only on inferiors, can be updated to have a
per-task context, where the context will be inherited during fork(),
clone() and preserved across execve(). If we support multiple private
procfs instances, then we may force the ptrace_may_access() on
/proc/<pids>/ to always run inside that new procfs instances. This will
allow to specifiy on user sessions if we should populate procfs with
pids that the user can ptrace or not.
By using Yama ptrace scope, some restricted users will only be able to see
inferiors inside /proc, they won't even be able to see their other
processes. Some software like Chromium, Firefox's crash handler, Wine
and others are already using Yama to restrict which processes can be
ptracable. With this change this will give the possibility to restrict
/proc/<pids>/ but more importantly this will give desktop users a
generic and usuable way to specifiy which users should see all processes
and which users can not.
Side notes:
* This covers the lack of seccomp where it is not able to parse
arguments, it is easy to install a seccomp filter on direct syscalls
that operate on pids, however /proc/<pid>/ is a Linux ABI using
filesystem syscalls. With this change LSMs should be able to analyze
open/read/write/close...
In the new patchset version I removed the 'newinstance' option
as Eric W. Biederman suggested.
Cc: Kees Cook <keescook@chromium.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/root.c | 41 ++++++++++++++++++-----------------------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/fs/proc/root.c b/fs/proc/root.c
index efd76c004e86..5d5cba4c899b 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -82,7 +82,7 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
return 0;
}
-static void proc_apply_options(struct super_block *s,
+static void proc_apply_options(struct proc_fs_info *fs_info,
struct fs_context *fc,
struct pid_namespace *pid_ns,
struct user_namespace *user_ns)
@@ -90,15 +90,17 @@ static void proc_apply_options(struct super_block *s,
struct proc_fs_context *ctx = fc->fs_private;
if (pid_ns->proc_mnt) {
- struct proc_fs_info *fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
- proc_fs_set_pid_gid(ctx->fs_info, proc_fs_pid_gid(fs_info));
- proc_fs_set_hide_pid(ctx->fs_info, proc_fs_hide_pid(fs_info));
+ struct proc_fs_info *pidns_fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
+
+ proc_fs_set_pid_gid(fs_info, proc_fs_pid_gid(pidns_fs_info));
+ proc_fs_set_hide_pid(fs_info, proc_fs_hide_pid(pidns_fs_info));
}
if (ctx->mask & (1 << Opt_gid))
- proc_fs_set_pid_gid(ctx->fs_info, make_kgid(user_ns, ctx->gid));
+ proc_fs_set_pid_gid(fs_info, make_kgid(user_ns, ctx->gid));
+
if (ctx->mask & (1 << Opt_hidepid))
- proc_fs_set_hide_pid(ctx->fs_info, ctx->hidepid);
+ proc_fs_set_hide_pid(fs_info, ctx->hidepid);
}
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
@@ -108,7 +110,7 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
struct inode *root_inode;
int ret;
- proc_apply_options(s, fc, pid_ns, current_user_ns());
+ proc_apply_options(ctx->fs_info, fc, pid_ns, current_user_ns());
/* User space would break if executables or devices appear on proc */
s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
@@ -118,6 +120,7 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
s->s_magic = PROC_SUPER_MAGIC;
s->s_op = &proc_sops;
s->s_time_gran = 1;
+ s->s_fs_info = ctx->fs_info;
/*
* procfs isn't actually a stacking filesystem; however, there is
@@ -157,15 +160,13 @@ static int proc_reconfigure(struct fs_context *fc)
sync_filesystem(sb);
- proc_apply_options(sb, fc, pid, current_user_ns());
+ proc_apply_options(fs_info, fc, pid, current_user_ns());
return 0;
}
static int proc_get_tree(struct fs_context *fc)
{
- struct proc_fs_context *ctx = fc->fs_private;
-
- return get_tree_keyed(fc, proc_fill_super, ctx->fs_info);
+ return get_tree_nodev(fc, proc_fill_super);
}
static void proc_fs_context_free(struct fs_context *fc)
@@ -186,25 +187,19 @@ static const struct fs_context_operations proc_fs_context_ops = {
static int proc_init_fs_context(struct fs_context *fc)
{
struct proc_fs_context *ctx;
- struct pid_namespace *pid_ns;
ctx = kzalloc(sizeof(struct proc_fs_context), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
- pid_ns = get_pid_ns(task_active_pid_ns(current));
-
- if (!pid_ns->proc_mnt) {
- ctx->fs_info = kzalloc(sizeof(struct proc_fs_info), GFP_KERNEL);
- if (!ctx->fs_info) {
- kfree(ctx);
- return -ENOMEM;
- }
- ctx->fs_info->pid_ns = pid_ns;
- } else {
- ctx->fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
+ ctx->fs_info = kzalloc(sizeof(struct proc_fs_info), GFP_KERNEL);
+ if (!ctx->fs_info) {
+ kfree(ctx);
+ return -ENOMEM;
}
+ ctx->fs_info->pid_ns = get_pid_ns(task_active_pid_ns(current));
+
put_user_ns(fc->user_ns);
fc->user_ns = get_user_ns(ctx->fs_info->pid_ns->user_ns);
fc->fs_private = ctx;
--
2.24.1
^ permalink raw reply related
* [PATCH v7 07/11] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This allows to flush dcache entries of a task on multiple procfs mounts
per pid namespace.
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 23 ++++++++++++++-----
fs/proc/root.c | 14 ++++++++++++
include/linux/pid_namespace.h | 42 +++++++++++++++++++++++++++++++++++
include/linux/proc_fs.h | 2 ++
4 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4ccb280a3e79..f4f1bcb28603 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
.permission = proc_pid_permission,
};
-static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
+static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
{
struct dentry *dentry, *leader, *dir;
char buf[10 + 1];
@@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
name.name = buf;
name.len = snprintf(buf, sizeof(buf), "%u", pid);
/* no ->d_hash() rejects on procfs */
- dentry = d_hash_and_lookup(mnt->mnt_root, &name);
+ dentry = d_hash_and_lookup(mnt_root, &name);
if (dentry) {
d_invalidate(dentry);
dput(dentry);
@@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
name.name = buf;
name.len = snprintf(buf, sizeof(buf), "%u", tgid);
- leader = d_hash_and_lookup(mnt->mnt_root, &name);
+ leader = d_hash_and_lookup(mnt_root, &name);
if (!leader)
goto out;
@@ -3208,14 +3208,27 @@ void proc_flush_task(struct task_struct *task)
int i;
struct pid *pid, *tgid;
struct upid *upid;
+ struct pid_namespace *pid_ns;
+ struct dentry *mnt_root;
+ struct proc_fs_info *fs_info;
pid = task_pid(task);
tgid = task_tgid(task);
for (i = 0; i <= pid->level; i++) {
upid = &pid->numbers[i];
- proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
- tgid->numbers[i].nr);
+
+ pid_ns = upid->ns;
+
+ pidns_proc_lock_shared(pid_ns);
+ list_for_each_entry(fs_info, &pid_ns->proc_mounts, pidns_entry) {
+ mnt_root = fs_info->m_super->s_root;
+ proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
+ }
+ pidns_proc_unlock_shared(pid_ns);
+
+ mnt_root = pid_ns->proc_mnt->mnt_root;
+ proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
}
}
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 5d5cba4c899b..3bb8df360cf7 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -112,6 +112,12 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
proc_apply_options(ctx->fs_info, fc, pid_ns, current_user_ns());
+ ctx->fs_info->m_super = s;
+
+ pidns_proc_lock(pid_ns);
+ list_add_tail(&ctx->fs_info->pidns_entry, &pid_ns->proc_mounts);
+ pidns_proc_unlock(pid_ns);
+
/* User space would break if executables or devices appear on proc */
s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
@@ -215,6 +221,11 @@ static void proc_kill_sb(struct super_block *sb)
dput(fs_info->proc_self);
if (fs_info->proc_thread_self)
dput(fs_info->proc_thread_self);
+
+ pidns_proc_lock(fs_info->pid_ns);
+ list_del(&fs_info->pidns_entry);
+ pidns_proc_unlock(fs_info->pid_ns);
+
kill_anon_super(sb);
put_pid_ns(fs_info->pid_ns);
kfree(fs_info);
@@ -336,6 +347,9 @@ int pid_ns_prepare_proc(struct pid_namespace *ns)
ctx->fs_info->pid_ns = ns;
}
+ init_rwsem(&ns->rw_proc_mounts);
+ INIT_LIST_HEAD(&ns->proc_mounts);
+
mnt = fc_mount(fc);
put_fs_context(fc);
if (IS_ERR(mnt))
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 66f47f1afe0d..297b39604312 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -26,6 +26,8 @@ struct pid_namespace {
struct pid_namespace *parent;
#ifdef CONFIG_PROC_FS
struct vfsmount *proc_mnt; /* Internal proc mounted during each new pidns */
+ struct rw_semaphore rw_proc_mounts;
+ struct list_head proc_mounts; /* list of separated procfs mounts */
#endif
#ifdef CONFIG_BSD_PROCESS_ACCT
struct fs_pin *bacct;
@@ -90,4 +92,44 @@ extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
void pidhash_init(void);
void pid_idr_init(void);
+#ifdef CONFIG_PROC_FS
+static inline void pidns_proc_lock(struct pid_namespace *pid_ns)
+{
+ down_write(&pid_ns->rw_proc_mounts);
+}
+
+static inline void pidns_proc_unlock(struct pid_namespace *pid_ns)
+{
+ up_write(&pid_ns->rw_proc_mounts);
+}
+
+static inline void pidns_proc_lock_shared(struct pid_namespace *pid_ns)
+{
+ down_read(&pid_ns->rw_proc_mounts);
+}
+
+static inline void pidns_proc_unlock_shared(struct pid_namespace *pid_ns)
+{
+ up_read(&pid_ns->rw_proc_mounts);
+}
+#else /* !CONFIG_PROC_FS */
+
+static inline void pidns_proc_lock(struct pid_namespace *pid_ns)
+{
+}
+
+static inline void pidns_proc_unlock(struct pid_namespace *pid_ns)
+{
+}
+
+static inline void pidns_proc_lock_shared(struct pid_namespace *pid_ns)
+{
+}
+
+static inline void pidns_proc_unlock_shared(struct pid_namespace *pid_ns)
+{
+}
+
+#endif /* CONFIG_PROC_FS */
+
#endif /* _LINUX_PID_NS_H */
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 5f0b1b7e4271..f307940f8311 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -20,6 +20,8 @@ enum {
};
struct proc_fs_info {
+ struct list_head pidns_entry; /* Node in procfs_mounts of a pidns */
+ struct super_block *m_super;
struct pid_namespace *pid_ns;
struct dentry *proc_self; /* For /proc/self */
struct dentry *proc_thread_self; /* For /proc/thread-self */
--
2.24.1
^ permalink raw reply related
* [PATCH v7 02/11] proc: add proc_fs_info struct to store proc information
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This is a preparation patch that adds proc_fs_info to be able to store
different procfs options and informations. Right now some mount options
are stored inside the pid namespace which makes it hard to change or
modernize procfs without affecting pid namespaces. Plus we do want to
treat proc as more of a real mount point and filesystem. procfs is part
of Linux API where it offers some features using filesystem syscalls and
in order to support some features where we are able to have multiple
instances of procfs, each one with its mount options inside the same pid
namespace, we have to separate these procfs instances.
This is the same feature that was also added to other Linux interfaces
like devpts in order to support containers, sandboxes, and to have
multiple instances of devpts filesystem [1].
[1] https://elixir.bootlin.com/linux/v3.4/source/Documentation/filesystems/devpts.txt
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/locks.c | 6 +++--
fs/proc/base.c | 8 +++++--
fs/proc/inode.c | 4 ++--
fs/proc/root.c | 49 +++++++++++++++++++++++++++--------------
include/linux/proc_fs.h | 11 ++++++++-
5 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 6970f55daf54..21200e3005e4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2795,7 +2795,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
{
struct inode *inode = NULL;
unsigned int fl_pid;
- struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(file_inode(f->file)->i_sb);
+ struct pid_namespace *proc_pidns = fs_info->pid_ns;
fl_pid = locks_translate_pid(fl, proc_pidns);
/*
@@ -2873,7 +2874,8 @@ static int locks_show(struct seq_file *f, void *v)
{
struct locks_iterator *iter = f->private;
struct file_lock *fl, *bfl;
- struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(file_inode(f->file)->i_sb);
+ struct pid_namespace *proc_pidns = fs_info->pid_ns;
fl = hlist_entry(v, struct file_lock, fl_link);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..672e71c52dbd 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3243,6 +3243,7 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
{
struct task_struct *task;
unsigned tgid;
+ struct proc_fs_info *fs_info;
struct pid_namespace *ns;
struct dentry *result = ERR_PTR(-ENOENT);
@@ -3250,7 +3251,8 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
if (tgid == ~0U)
goto out;
- ns = dentry->d_sb->s_fs_info;
+ fs_info = proc_sb_info(dentry->d_sb);
+ ns = fs_info->pid_ns;
rcu_read_lock();
task = find_task_by_pid_ns(tgid, ns);
if (task)
@@ -3538,6 +3540,7 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry
struct task_struct *task;
struct task_struct *leader = get_proc_task(dir);
unsigned tid;
+ struct proc_fs_info *fs_info;
struct pid_namespace *ns;
struct dentry *result = ERR_PTR(-ENOENT);
@@ -3548,7 +3551,8 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry
if (tid == ~0U)
goto out;
- ns = dentry->d_sb->s_fs_info;
+ fs_info = proc_sb_info(dentry->d_sb);
+ ns = fs_info->pid_ns;
rcu_read_lock();
task = find_task_by_pid_ns(tid, ns);
if (task)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index dbe43a50caf2..b631608dfbcc 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -104,8 +104,8 @@ void __init proc_init_kmemcache(void)
static int proc_show_options(struct seq_file *seq, struct dentry *root)
{
- struct super_block *sb = root->d_sb;
- struct pid_namespace *pid = sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(root->d_sb);
+ struct pid_namespace *pid = fs_info->pid_ns;
if (!gid_eq(pid->pid_gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, pid->pid_gid));
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 0b7c8dffc9ae..d449f095f0f7 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -30,7 +30,7 @@
#include "internal.h"
struct proc_fs_context {
- struct pid_namespace *pid_ns;
+ struct proc_fs_info *fs_info;
unsigned int mask;
int hidepid;
int gid;
@@ -97,7 +97,8 @@ static void proc_apply_options(struct super_block *s,
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
{
- struct pid_namespace *pid_ns = get_pid_ns(s->s_fs_info);
+ struct proc_fs_context *ctx = fc->fs_private;
+ struct pid_namespace *pid_ns = get_pid_ns(ctx->fs_info->pid_ns);
struct inode *root_inode;
int ret;
@@ -145,7 +146,8 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
static int proc_reconfigure(struct fs_context *fc)
{
struct super_block *sb = fc->root->d_sb;
- struct pid_namespace *pid = sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(sb);
+ struct pid_namespace *pid = fs_info->pid_ns;
sync_filesystem(sb);
@@ -157,14 +159,14 @@ static int proc_get_tree(struct fs_context *fc)
{
struct proc_fs_context *ctx = fc->fs_private;
- return get_tree_keyed(fc, proc_fill_super, ctx->pid_ns);
+ return get_tree_keyed(fc, proc_fill_super, ctx->fs_info);
}
static void proc_fs_context_free(struct fs_context *fc)
{
struct proc_fs_context *ctx = fc->fs_private;
- put_pid_ns(ctx->pid_ns);
+ put_pid_ns(ctx->fs_info->pid_ns);
kfree(ctx);
}
@@ -178,14 +180,27 @@ static const struct fs_context_operations proc_fs_context_ops = {
static int proc_init_fs_context(struct fs_context *fc)
{
struct proc_fs_context *ctx;
+ struct pid_namespace *pid_ns;
ctx = kzalloc(sizeof(struct proc_fs_context), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
- ctx->pid_ns = get_pid_ns(task_active_pid_ns(current));
+ pid_ns = get_pid_ns(task_active_pid_ns(current));
+
+ if (!pid_ns->proc_mnt) {
+ ctx->fs_info = kzalloc(sizeof(struct proc_fs_info), GFP_KERNEL);
+ if (!ctx->fs_info) {
+ kfree(ctx);
+ return -ENOMEM;
+ }
+ ctx->fs_info->pid_ns = pid_ns;
+ } else {
+ ctx->fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
+ }
+
put_user_ns(fc->user_ns);
- fc->user_ns = get_user_ns(ctx->pid_ns->user_ns);
+ fc->user_ns = get_user_ns(ctx->fs_info->pid_ns->user_ns);
fc->fs_private = ctx;
fc->ops = &proc_fs_context_ops;
return 0;
@@ -193,15 +208,15 @@ static int proc_init_fs_context(struct fs_context *fc)
static void proc_kill_sb(struct super_block *sb)
{
- struct pid_namespace *ns;
+ struct proc_fs_info *fs_info = proc_sb_info(sb);
- ns = (struct pid_namespace *)sb->s_fs_info;
- if (ns->proc_self)
- dput(ns->proc_self);
- if (ns->proc_thread_self)
- dput(ns->proc_thread_self);
+ if (fs_info->pid_ns->proc_self)
+ dput(fs_info->pid_ns->proc_self);
+ if (fs_info->pid_ns->proc_thread_self)
+ dput(fs_info->pid_ns->proc_thread_self);
kill_anon_super(sb);
- put_pid_ns(ns);
+ put_pid_ns(fs_info->pid_ns);
+ kfree(fs_info);
}
static struct file_system_type proc_fs_type = {
@@ -314,10 +329,10 @@ int pid_ns_prepare_proc(struct pid_namespace *ns)
}
ctx = fc->fs_private;
- if (ctx->pid_ns != ns) {
- put_pid_ns(ctx->pid_ns);
+ if (ctx->fs_info->pid_ns != ns) {
+ put_pid_ns(ctx->fs_info->pid_ns);
get_pid_ns(ns);
- ctx->pid_ns = ns;
+ ctx->fs_info->pid_ns = ns;
}
mnt = fc_mount(fc);
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index a705aa2d03f9..2d79489e55aa 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -12,6 +12,15 @@ struct proc_dir_entry;
struct seq_file;
struct seq_operations;
+struct proc_fs_info {
+ struct pid_namespace *pid_ns;
+};
+
+static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
+{
+ return sb->s_fs_info;
+}
+
#ifdef CONFIG_PROC_FS
typedef int (*proc_write_t)(struct file *, char *, size_t);
@@ -146,7 +155,7 @@ int open_related_ns(struct ns_common *ns,
/* get the associated pid namespace for a file in procfs */
static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
{
- return inode->i_sb->s_fs_info;
+ return proc_sb_info(inode->i_sb)->pid_ns;
}
#endif /* _LINUX_PROC_FS_H */
--
2.24.1
^ permalink raw reply related
* [PATCH v7 05/11] proc: add helpers to set and get proc hidepid and gid mount options
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This is a cleaning patch to add helpers to set and get proc mount
options instead of directly using them. This make it easy to track
what's happening and easy to update in future.
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 6 +++---
fs/proc/inode.c | 11 +++++++----
fs/proc/internal.h | 20 ++++++++++++++++++++
fs/proc/root.c | 8 ++++----
4 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index caca1929fee1..4ccb280a3e79 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -699,9 +699,9 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
struct task_struct *task,
int hide_pid_min)
{
- if (fs_info->hide_pid < hide_pid_min)
+ if (proc_fs_hide_pid(fs_info) < hide_pid_min)
return true;
- if (in_group_p(fs_info->pid_gid))
+ if (in_group_p(proc_fs_pid_gid(fs_info)))
return true;
return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
}
@@ -720,7 +720,7 @@ static int proc_pid_permission(struct inode *inode, int mask)
put_task_struct(task);
if (!has_perms) {
- if (fs_info->hide_pid == HIDEPID_INVISIBLE) {
+ if (proc_fs_hide_pid(fs_info) == HIDEPID_INVISIBLE) {
/*
* Let's make getdents(), stat(), and open()
* consistent with each other. If a process
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index b90c233e5968..70b722fb8811 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -105,11 +105,14 @@ void __init proc_init_kmemcache(void)
static int proc_show_options(struct seq_file *seq, struct dentry *root)
{
struct proc_fs_info *fs_info = proc_sb_info(root->d_sb);
+ int hidepid = proc_fs_hide_pid(fs_info);
+ kgid_t gid = proc_fs_pid_gid(fs_info);
- if (!gid_eq(fs_info->pid_gid, GLOBAL_ROOT_GID))
- seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, fs_info->pid_gid));
- if (fs_info->hide_pid != HIDEPID_OFF)
- seq_printf(seq, ",hidepid=%u", fs_info->hide_pid);
+ if (!gid_eq(gid, GLOBAL_ROOT_GID))
+ seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, gid));
+
+ if (hidepid != HIDEPID_OFF)
+ seq_printf(seq, ",hidepid=%u", hidepid);
return 0;
}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index cd0c8d5ce9a1..ff2f274b2e0d 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -121,6 +121,26 @@ static inline struct task_struct *get_proc_task(const struct inode *inode)
return get_pid_task(proc_pid(inode), PIDTYPE_PID);
}
+static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int hide_pid)
+{
+ fs_info->hide_pid = hide_pid;
+}
+
+static inline void proc_fs_set_pid_gid(struct proc_fs_info *fs_info, kgid_t gid)
+{
+ fs_info->pid_gid = gid;
+}
+
+static inline int proc_fs_hide_pid(struct proc_fs_info *fs_info)
+{
+ return fs_info->hide_pid;
+}
+
+static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
+{
+ return fs_info->pid_gid;
+}
+
void task_dump_owner(struct task_struct *task, umode_t mode,
kuid_t *ruid, kgid_t *rgid);
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 1ca47d446aa4..efd76c004e86 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -91,14 +91,14 @@ static void proc_apply_options(struct super_block *s,
if (pid_ns->proc_mnt) {
struct proc_fs_info *fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
- ctx->fs_info->pid_gid = fs_info->pid_gid;
- ctx->fs_info->hide_pid = fs_info->hide_pid;
+ proc_fs_set_pid_gid(ctx->fs_info, proc_fs_pid_gid(fs_info));
+ proc_fs_set_hide_pid(ctx->fs_info, proc_fs_hide_pid(fs_info));
}
if (ctx->mask & (1 << Opt_gid))
- ctx->fs_info->pid_gid = make_kgid(user_ns, ctx->gid);
+ proc_fs_set_pid_gid(ctx->fs_info, make_kgid(user_ns, ctx->gid));
if (ctx->mask & (1 << Opt_hidepid))
- ctx->fs_info->hide_pid = ctx->hidepid;
+ proc_fs_set_hide_pid(ctx->fs_info, ctx->hidepid);
}
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
--
2.24.1
^ permalink raw reply related
* [PATCH v7 03/11] proc: move /proc/{self|thread-self} dentries to proc_fs_info
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This is a preparation patch that moves /proc/{self|thread-self} dentries
to be stored inside procfs fs_info struct instead of making them per pid
namespace. Since we want to support multiple procfs instances we need to
make sure that these dentries are also per-superblock instead of
per-pidns, unmounting a private procfs won't clash with other procfs
mounts.
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 5 +++--
fs/proc/root.c | 8 ++++----
fs/proc/self.c | 4 ++--
fs/proc/thread_self.c | 6 +++---
include/linux/pid_namespace.h | 4 +---
include/linux/proc_fs.h | 2 ++
6 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 672e71c52dbd..1eb366ad8b06 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3316,6 +3316,7 @@ static struct tgid_iter next_tgid(struct pid_namespace *ns, struct tgid_iter ite
int proc_pid_readdir(struct file *file, struct dir_context *ctx)
{
struct tgid_iter iter;
+ struct proc_fs_info *fs_info = proc_sb_info(file_inode(file)->i_sb);
struct pid_namespace *ns = proc_pid_ns(file_inode(file));
loff_t pos = ctx->pos;
@@ -3323,13 +3324,13 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
return 0;
if (pos == TGID_OFFSET - 2) {
- struct inode *inode = d_inode(ns->proc_self);
+ struct inode *inode = d_inode(fs_info->proc_self);
if (!dir_emit(ctx, "self", 4, inode->i_ino, DT_LNK))
return 0;
ctx->pos = pos = pos + 1;
}
if (pos == TGID_OFFSET - 1) {
- struct inode *inode = d_inode(ns->proc_thread_self);
+ struct inode *inode = d_inode(fs_info->proc_thread_self);
if (!dir_emit(ctx, "thread-self", 11, inode->i_ino, DT_LNK))
return 0;
ctx->pos = pos = pos + 1;
diff --git a/fs/proc/root.c b/fs/proc/root.c
index d449f095f0f7..637e26cc795e 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -210,10 +210,10 @@ static void proc_kill_sb(struct super_block *sb)
{
struct proc_fs_info *fs_info = proc_sb_info(sb);
- if (fs_info->pid_ns->proc_self)
- dput(fs_info->pid_ns->proc_self);
- if (fs_info->pid_ns->proc_thread_self)
- dput(fs_info->pid_ns->proc_thread_self);
+ if (fs_info->proc_self)
+ dput(fs_info->proc_self);
+ if (fs_info->proc_thread_self)
+ dput(fs_info->proc_thread_self);
kill_anon_super(sb);
put_pid_ns(fs_info->pid_ns);
kfree(fs_info);
diff --git a/fs/proc/self.c b/fs/proc/self.c
index 57c0a1047250..846fc2b7c8a8 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -36,7 +36,7 @@ static unsigned self_inum __ro_after_init;
int proc_setup_self(struct super_block *s)
{
struct inode *root_inode = d_inode(s->s_root);
- struct pid_namespace *ns = proc_pid_ns(root_inode);
+ struct proc_fs_info *fs_info = proc_sb_info(s);
struct dentry *self;
int ret = -ENOMEM;
@@ -62,7 +62,7 @@ int proc_setup_self(struct super_block *s)
if (ret)
pr_err("proc_fill_super: can't allocate /proc/self\n");
else
- ns->proc_self = self;
+ fs_info->proc_self = self;
return ret;
}
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index f61ae53533f5..2493cbbdfa6f 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -36,7 +36,7 @@ static unsigned thread_self_inum __ro_after_init;
int proc_setup_thread_self(struct super_block *s)
{
struct inode *root_inode = d_inode(s->s_root);
- struct pid_namespace *ns = proc_pid_ns(root_inode);
+ struct proc_fs_info *fs_info = proc_sb_info(s);
struct dentry *thread_self;
int ret = -ENOMEM;
@@ -60,9 +60,9 @@ int proc_setup_thread_self(struct super_block *s)
inode_unlock(root_inode);
if (ret)
- pr_err("proc_fill_super: can't allocate /proc/thread_self\n");
+ pr_err("proc_fill_super: can't allocate /proc/thread-self\n");
else
- ns->proc_thread_self = thread_self;
+ fs_info->proc_thread_self = thread_self;
return ret;
}
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 49538b172483..f91a8bf6e09e 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -31,9 +31,7 @@ struct pid_namespace {
unsigned int level;
struct pid_namespace *parent;
#ifdef CONFIG_PROC_FS
- struct vfsmount *proc_mnt;
- struct dentry *proc_self;
- struct dentry *proc_thread_self;
+ struct vfsmount *proc_mnt; /* Internal proc mounted during each new pidns */
#endif
#ifdef CONFIG_BSD_PROCESS_ACCT
struct fs_pin *bacct;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 2d79489e55aa..59162988998e 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -14,6 +14,8 @@ struct seq_operations;
struct proc_fs_info {
struct pid_namespace *pid_ns;
+ struct dentry *proc_self; /* For /proc/self */
+ struct dentry *proc_thread_self; /* For /proc/thread-self */
};
static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
--
2.24.1
^ permalink raw reply related
* [PATCH v7 04/11] proc: move hide_pid, pid_gid from pid_namespace to proc_fs_info
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This is a preparation patch that moves hide_pid and pid_gid parameters
to be stored inside procfs fs_info struct instead of making them per pid
namespace. Since we want to support multiple procfs instances we need to
make sure that all proc-specific parameters are also per-superblock.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 18 +++++++++---------
fs/proc/inode.c | 9 ++++-----
fs/proc/root.c | 10 ++++++++--
include/linux/pid_namespace.h | 8 --------
include/linux/proc_fs.h | 9 +++++++++
5 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1eb366ad8b06..caca1929fee1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -695,13 +695,13 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
* May current process learn task's sched/cmdline info (for hide_pid_min=1)
* or euid/egid (for hide_pid_min=2)?
*/
-static bool has_pid_permissions(struct pid_namespace *pid,
+static bool has_pid_permissions(struct proc_fs_info *fs_info,
struct task_struct *task,
int hide_pid_min)
{
- if (pid->hide_pid < hide_pid_min)
+ if (fs_info->hide_pid < hide_pid_min)
return true;
- if (in_group_p(pid->pid_gid))
+ if (in_group_p(fs_info->pid_gid))
return true;
return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
}
@@ -709,18 +709,18 @@ static bool has_pid_permissions(struct pid_namespace *pid,
static int proc_pid_permission(struct inode *inode, int mask)
{
- struct pid_namespace *pid = proc_pid_ns(inode);
+ struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
struct task_struct *task;
bool has_perms;
task = get_proc_task(inode);
if (!task)
return -ESRCH;
- has_perms = has_pid_permissions(pid, task, HIDEPID_NO_ACCESS);
+ has_perms = has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS);
put_task_struct(task);
if (!has_perms) {
- if (pid->hide_pid == HIDEPID_INVISIBLE) {
+ if (fs_info->hide_pid == HIDEPID_INVISIBLE) {
/*
* Let's make getdents(), stat(), and open()
* consistent with each other. If a process
@@ -1784,7 +1784,7 @@ int pid_getattr(const struct path *path, struct kstat *stat,
u32 request_mask, unsigned int query_flags)
{
struct inode *inode = d_inode(path->dentry);
- struct pid_namespace *pid = proc_pid_ns(inode);
+ struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
struct task_struct *task;
generic_fillattr(inode, stat);
@@ -1794,7 +1794,7 @@ int pid_getattr(const struct path *path, struct kstat *stat,
rcu_read_lock();
task = pid_task(proc_pid(inode), PIDTYPE_PID);
if (task) {
- if (!has_pid_permissions(pid, task, HIDEPID_INVISIBLE)) {
+ if (!has_pid_permissions(fs_info, task, HIDEPID_INVISIBLE)) {
rcu_read_unlock();
/*
* This doesn't prevent learning whether PID exists,
@@ -3344,7 +3344,7 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
unsigned int len;
cond_resched();
- if (!has_pid_permissions(ns, iter.task, HIDEPID_INVISIBLE))
+ if (!has_pid_permissions(fs_info, iter.task, HIDEPID_INVISIBLE))
continue;
len = snprintf(name, sizeof(name), "%u", iter.tgid);
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index b631608dfbcc..b90c233e5968 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -105,12 +105,11 @@ void __init proc_init_kmemcache(void)
static int proc_show_options(struct seq_file *seq, struct dentry *root)
{
struct proc_fs_info *fs_info = proc_sb_info(root->d_sb);
- struct pid_namespace *pid = fs_info->pid_ns;
- if (!gid_eq(pid->pid_gid, GLOBAL_ROOT_GID))
- seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, pid->pid_gid));
- if (pid->hide_pid != HIDEPID_OFF)
- seq_printf(seq, ",hidepid=%u", pid->hide_pid);
+ if (!gid_eq(fs_info->pid_gid, GLOBAL_ROOT_GID))
+ seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, fs_info->pid_gid));
+ if (fs_info->hide_pid != HIDEPID_OFF)
+ seq_printf(seq, ",hidepid=%u", fs_info->hide_pid);
return 0;
}
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 637e26cc795e..1ca47d446aa4 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -89,10 +89,16 @@ static void proc_apply_options(struct super_block *s,
{
struct proc_fs_context *ctx = fc->fs_private;
+ if (pid_ns->proc_mnt) {
+ struct proc_fs_info *fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
+ ctx->fs_info->pid_gid = fs_info->pid_gid;
+ ctx->fs_info->hide_pid = fs_info->hide_pid;
+ }
+
if (ctx->mask & (1 << Opt_gid))
- pid_ns->pid_gid = make_kgid(user_ns, ctx->gid);
+ ctx->fs_info->pid_gid = make_kgid(user_ns, ctx->gid);
if (ctx->mask & (1 << Opt_hidepid))
- pid_ns->hide_pid = ctx->hidepid;
+ ctx->fs_info->hide_pid = ctx->hidepid;
}
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index f91a8bf6e09e..66f47f1afe0d 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -15,12 +15,6 @@
struct fs_pin;
-enum { /* definitions for pid_namespace's hide_pid field */
- HIDEPID_OFF = 0,
- HIDEPID_NO_ACCESS = 1,
- HIDEPID_INVISIBLE = 2,
-};
-
struct pid_namespace {
struct kref kref;
struct idr idr;
@@ -39,8 +33,6 @@ struct pid_namespace {
struct user_namespace *user_ns;
struct ucounts *ucounts;
struct work_struct proc_work;
- kgid_t pid_gid;
- int hide_pid;
int reboot; /* group exit code if this pidns was rebooted */
struct ns_common ns;
} __randomize_layout;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 59162988998e..5f0b1b7e4271 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -12,10 +12,19 @@ struct proc_dir_entry;
struct seq_file;
struct seq_operations;
+/* definitions for hide_pid field */
+enum {
+ HIDEPID_OFF = 0,
+ HIDEPID_NO_ACCESS = 1,
+ HIDEPID_INVISIBLE = 2,
+};
+
struct proc_fs_info {
struct pid_namespace *pid_ns;
struct dentry *proc_self; /* For /proc/self */
struct dentry *proc_thread_self; /* For /proc/thread-self */
+ kgid_t pid_gid;
+ int hide_pid;
};
static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
--
2.24.1
^ permalink raw reply related
* [PATCH v7 01/11] proc: Rename struct proc_fs_info to proc_fs_opts
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc_namespace.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 273ee82d8aa9..9a8b624bc3db 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -37,23 +37,23 @@ static __poll_t mounts_poll(struct file *file, poll_table *wait)
return res;
}
-struct proc_fs_info {
+struct proc_fs_opts {
int flag;
const char *str;
};
static int show_sb_opts(struct seq_file *m, struct super_block *sb)
{
- static const struct proc_fs_info fs_info[] = {
+ static const struct proc_fs_opts fs_opts[] = {
{ SB_SYNCHRONOUS, ",sync" },
{ SB_DIRSYNC, ",dirsync" },
{ SB_MANDLOCK, ",mand" },
{ SB_LAZYTIME, ",lazytime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = fs_opts; fs_infop->flag; fs_infop++) {
if (sb->s_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
@@ -63,7 +63,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb)
static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{
- static const struct proc_fs_info mnt_info[] = {
+ static const struct proc_fs_opts mnt_opts[] = {
{ MNT_NOSUID, ",nosuid" },
{ MNT_NODEV, ",nodev" },
{ MNT_NOEXEC, ",noexec" },
@@ -72,9 +72,9 @@ static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{ MNT_RELATIME, ",relatime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = mnt_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = mnt_opts; fs_infop->flag; fs_infop++) {
if (mnt->mnt_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v7 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-01-25 18:45 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton,
Jonathan Corbet, Kees Cook, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-8-gladkov.alexey@gmail.com>
On Sat, Jan 25, 2020 at 5:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> This allows to flush dcache entries of a task on multiple procfs mounts
> per pid namespace.
From a quick read-through, this is the only one I really react negatively to.
The locking looks odd. It only seems to protect the new proc_mounts
list, but then it's a whole big rwsem, and it's taken over all of
proc_flush_task_mnt(), and the locking is exported to all over as a
result of that - including the dummy functions for "there is no proc"
case.
And proc_flush_task_mnt() itself should need no locking over any of
it, so it's all just for the silly looping over the list.
So
(a) this looks fishy and feels wrong - I get a very strong feeling
that the locking is wrong to begin with, and could/should have been
done differently
(b) all the locking should have been internal to /proc, and those
wrappers shouldn't exist in a common header file (and certainly not
for the non-proc case).
Yes, (a) is just a feeling, and I don't have any great suggestions.
Maybe make it an RCU list and use a spinlock for updating it?
But (b) is pretty much a non-starter in this form. Those wrappers
shouldn't be in a globally exported core header file. No way.
Linus
^ permalink raw reply
* Re: [RFC PATCH 07/14] Add sample notification program [ver #3]
From: Guenter Roeck @ 2020-01-26 15:47 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Greg Kroah-Hartman, Casey Schaufler, Stephen Smalley,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, linux-security-module, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <157909509882.20155.1159021562184142124.stgit@warthog.procyon.org.uk>
On Wed, Jan 15, 2020 at 01:31:38PM +0000, David Howells wrote:
> The sample program is run like:
>
> ./samples/watch_queue/watch_test
>
> and watches "/" for mount changes and the current session keyring for key
> changes:
>
> # keyctl add user a a @s
> 1035096409
> # keyctl unlink 1035096409 @s
>
> producing:
>
> # ./watch_test
> read() = 16
> NOTIFY[000]: ty=000001 sy=02 i=00000110
> KEY 2ffc2e5d change=2[linked] aux=1035096409
> read() = 16
> NOTIFY[000]: ty=000001 sy=02 i=00000110
> KEY 2ffc2e5d change=3[unlinked] aux=1035096409
>
> Other events may be produced, such as with a failing disk:
>
> read() = 22
> NOTIFY[000]: ty=000003 sy=02 i=00000416
> USB 3-7.7 dev-reset e=0 r=0
> read() = 24
> NOTIFY[000]: ty=000002 sy=06 i=00000418
> BLOCK 00800050 e=6[critical medium] s=64000ef8
>
> This corresponds to:
>
> blk_update_request: critical medium error, dev sdf, sector 1677725432 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>
> in dmesg.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
mips:allmodconfig:
samples/watch_queue/watch_test.c: In function ‘keyctl_watch_key’:
samples/watch_queue/watch_test.c:34:17: error: ‘__NR_keyctl’ undeclared
Guenter
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Mimi Zohar @ 2020-01-26 17:01 UTC (permalink / raw)
To: Janne Karhunen
Cc: linux-integrity, linux-security-module, Ken Goldman,
david.safford, monty.wiseman, Serge E. Hallyn
In-Reply-To: <CAE=NcraSfo0fwxCnFQd08Ga59DsvxPTch-n_iN7fxt+3RdmP+A@mail.gmail.com>
On Thu, 2020-01-23 at 10:41 +0200, Janne Karhunen wrote:
> On Wed, Jan 22, 2020 at 5:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> > > While it can now be argued that since this is an admin-driven event,
> > > kernel does not need to write the file. However, the intention is to
> > > bring out a second patch a bit later that adds a variable to define
> > > the max number of entries to be kept in the kernel memory and
> > > workqueue based automatic flushing. In those cases the kernel has to
> > > be able to write the file without any help from the admin..
> >
> > I don't think it is common, and probably not acceptable, for the
> > kernel to open a file for writing.
>
> Ok. It just means that the kernel cannot do its own memory management
> and will depend on the user flushing the memory often enough to
> prevent something bad from happening. Is this more common in the
> kernel than writing out a file?
Ok, there are examples of both passing a file descriptor and passing a
pathname from userspace, but even in the case of passing a pathname,
userspace normally creates the file.
There's been discussion in the past of defining an integrity
capability. Are we at that point where we really do need to define an
integrity capability or is everyone comfortable with relying on
CAP_SYS_ADMIN?
When implementing this feature of exporting and truncating the
measurement list, please keep in mind how this would work in the
context of IMA namespaces.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-01-27 8:48 UTC (permalink / raw)
To: david.safford
Cc: linux-integrity, linux-security-module, Mimi Zohar, Ken Goldman,
Wiseman, Monty (GE Global Research, US)
In-Reply-To: <1a8c2fb4698eaa72be219413e373c5829a7cd652.camel@gmail.com>
On Fri, Jan 24, 2020 at 4:46 PM <david.safford@gmail.com> wrote:
> > Some systems can end up carrying lots of entries in the ima
> > measurement list. Since every entry is using a bit of kernel
> > memory, allow the sysadmin to export the measurement list to
> > the filesystem to free up some memory.
> >
> > Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
>
> I like this approach, as it will work easily for measurement lists in
> any format, and it will work for user or kernel triggering.
Yes, that was the point..
> I'm getting an OOPS, though, whenever I write a filename to the
> securityfs file (e.g. echo /var/log/ima.log > list_name).
> Here's the relevant from syslog:
>
> BUG: unable to handle page fault for address: 00005650a0e7fe30
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0001) - permissions violation
>
> Oops: 0001 [#1] SMP NOPTI
>
> RIP: 0010:ima_write_list_name+0x35/0x114
>
> I haven't had time to debug this. Any suggestions?
Interesting. I'm not a X86 man, but it looks like a SMAP trap to me.
In other words, the kernel was not allowed to read that
"/var/log/ima.log" string as SMAP protection was active for that piece
of memory. I was under the impression that the SMAP is disabled while
the copy_from_user is running..
I'll try on the X86 hardware and see if I can reproduce, maybe its off
by one read or something. Only X86 target I tested was QEMU.
> > +#define secfs_mnt "/sys/kernel/security"
> > +#define am_filename "/integrity/ima/ascii_runtime_measurements"
>
> You probably really want to export the binary data, as that's
> what's needed for attestation. (Or both, but that's trickier.)
That's why the file name was left as an argument to the export
function; you can export either one. I suppose we'd need some setting
to tell it which one to export.
--
Janne
^ permalink raw reply
* Re: [PATCH v4 7/9] parisc/perf: open access for CAP_SYS_PERFMON privileged process
From: Helge Deller @ 2020-01-27 8:52 UTC (permalink / raw)
To: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
Alexei Starovoitov, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, james.bottomley@hansenpartnership.com,
Serge Hallyn, James Morris, Will Deacon, Mark Rutland,
Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <806e4881-5c22-5914-a656-5eeb65130dbe@linux.intel.com>
On 18.12.19 10:29, Alexey Budankov wrote:
>
> Open access to monitoring for CAP_SYS_PERFMON privileged processes.
> For backward compatibility reasons access to the monitoring remains open
> for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage for secure
> monitoring is discouraged with respect to CAP_SYS_PERFMON capability.
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
Acked-by: Helge Deller <deller@gmx.de>
> ---
> arch/parisc/kernel/perf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/parisc/kernel/perf.c b/arch/parisc/kernel/perf.c
> index 676683641d00..c4208d027794 100644
> --- a/arch/parisc/kernel/perf.c
> +++ b/arch/parisc/kernel/perf.c
> @@ -300,7 +300,7 @@ static ssize_t perf_write(struct file *file, const char __user *buf,
> else
> return -EFAULT;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!perfmon_capable())
> return -EACCES;
>
> if (count != sizeof(uint32_t))
>
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-01-27 9:03 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, linux-security-module, Ken Goldman,
david.safford, monty.wiseman, Serge E. Hallyn
In-Reply-To: <1580058069.5990.36.camel@linux.ibm.com>
On Sun, Jan 26, 2020 at 7:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > I don't think it is common, and probably not acceptable, for the
> > > kernel to open a file for writing.
> >
> > Ok. It just means that the kernel cannot do its own memory management
> > and will depend on the user flushing the memory often enough to
> > prevent something bad from happening. Is this more common in the
> > kernel than writing out a file?
>
> Ok, there are examples of both passing a file descriptor and passing a
> pathname from userspace, but even in the case of passing a pathname,
> userspace normally creates the file.
Sorry, I was slow to get your proposal. I'll try to see how that would
look like.
> There's been discussion in the past of defining an integrity
> capability. Are we at that point where we really do need to define an
> integrity capability or is everyone comfortable with relying on
> CAP_SYS_ADMIN?
Every time something like this is being proposed there is a lot of
shouting from people that they want their root user (renamed as
CAP_SYS_ADMIN) back. I'd be happy with such bit and several others,
too.
> When implementing this feature of exporting and truncating the
> measurement list, please keep in mind how this would work in the
> context of IMA namespaces.
That could be rough. I'll try to think about it.
--
Janne
^ permalink raw reply
* Re: [PATCH 1/1] proc_keys_next should increase position index
From: David Howells @ 2020-01-27 11:39 UTC (permalink / raw)
To: Vasily Averin
Cc: dhowells, keyrings, linux-security-module, Jarkko Sakkinen,
James Morris, Serge E. Hallyn
In-Reply-To: <af9dcaa7-6e4f-281a-2bae-fb605cc55d2d@virtuozzo.com>
I don't see the effect you're talking about with /proc/keys. I see the
following:
[root@andromeda ~]# dd if=/proc/keys bs=40 skip=1
dd: /proc/keys: cannot skip to specified offset
and then it follows up with the normal content with no obvious duplicates (the
lines are numbered ascendingly in the first column).
I think I may be being confused by what you mean by "the last line".
David
^ permalink raw reply
* Re: [PATCH v14 00/23] LSM: Module stacking for AppArmor
From: Stephen Smalley @ 2020-01-27 16:14 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <6b717a13-3586-5854-0eee-617798f92d34@schaufler-ca.com>
On 1/24/20 4:49 PM, Casey Schaufler wrote:
> On 1/24/2020 1:04 PM, Stephen Smalley wrote:
>> On 1/23/20 7:22 PM, Casey Schaufler wrote:
>>> This patchset provides the changes required for
>>> the AppArmor security module to stack safely with any other.
>>>
>>> v14: Rebase to 5.5-rc5
>>> Incorporate feedback from v13
>>> - Use an array of audit rules (patch 0002)
>>> - Significant change, removed Acks (patch 0002)
>>> - Remove unneeded include (patch 0013)
>>> - Use context.len correctly (patch 0015)
>>> - Reorder code to be more sensible (patch 0016)
>>> - Drop SO_PEERCONTEXT as it's not needed yet (patch 0023)
>>
>> I don't know for sure if this is your bug, but it happens every time I boot with your patches applied and not at all on stock v5.5-rc5 so here it is. Will try to bisect as time permits but not until next week. Trigger seems to be loading the tun driver.
>
> Thanks. I will have a look as well.
Bisection led to the first patch in the series, "LSM: Infrastructure
management of the sock security". Still not sure if the bug is in the
patch itself or just being surfaced by it.
>>
>> [ 67.726834] tun: Universal TUN/TAP device driver, 1.6
>> [ 67.736657] ==================================================================
>> [ 67.741335] BUG: KASAN: slab-out-of-bounds in sock_init_data+0x14a/0x5a0
>> [ 67.745037] Read of size 4 at addr ffff88870afe8928 by task libvirtd/1238
>>
>> [ 67.751861] CPU: 4 PID: 1238 Comm: libvirtd Tainted: G T 5.5.0-rc5+ #54
>> [ 67.756250] Call Trace:
>> [ 67.759510] dump_stack+0xb8/0x110
>> [ 67.761604] print_address_description.constprop.0+0x1f/0x280
>> [ 67.763768] __kasan_report.cold+0x75/0x8f
>> [ 67.765895] ? sock_init_data+0x14a/0x5a0
>> [ 67.768282] kasan_report+0xe/0x20
>> [ 67.770397] sock_init_data+0x14a/0x5a0
>> [ 67.772511] tun_chr_open+0x1de/0x280 [tun]
>> [ 67.774644] misc_open+0x1cb/0x210
>> [ 67.776820] chrdev_open+0x15b/0x350
>> [ 67.778917] ? cdev_put.part.0+0x30/0x30
>> [ 67.781030] do_dentry_open+0x2cb/0x800
>> [ 67.783135] ? cdev_put.part.0+0x30/0x30
>> [ 67.785225] ? devcgroup_check_permission+0x11a/0x260
>> [ 67.787321] ? __x64_sys_fchdir+0xf0/0xf0
>> [ 67.789418] ? security_inode_permission+0x5b/0x70
>> [ 67.791513] path_openat+0x858/0x14a0
>> [ 67.793589] ? path_mountpoint+0x5e0/0x5e0
>> [ 67.795719] ? mark_lock+0xb8/0xb00
>> [ 67.797786] do_filp_open+0x11e/0x1b0
>> [ 67.799840] ? may_open_dev+0x60/0x60
>> [ 67.801871] ? match_held_lock+0x1b/0x240
>> [ 67.803968] ? lock_downgrade+0x360/0x360
>> [ 67.805997] ? do_raw_spin_lock+0x119/0x1d0
>> [ 67.808041] ? rwlock_bug.part.0+0x60/0x60
>> [ 67.810099] ? do_raw_spin_unlock+0xa3/0x130
>> [ 67.812244] ? _raw_spin_unlock+0x1f/0x30
>> [ 67.814287] ? __alloc_fd+0x143/0x2f0
>> [ 67.816324] do_sys_open+0x1f0/0x2d0
>> [ 67.818358] ? filp_open+0x50/0x50
>> [ 67.820404] ? trace_hardirqs_on_thunk+0x1a/0x1c
>> [ 67.822447] ? lockdep_hardirqs_off+0xbe/0x100
>> [ 67.824473] ? mark_held_locks+0x24/0x90
>> [ 67.826484] do_syscall_64+0x74/0xd0
>> [ 67.828480] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [ 67.830478] RIP: 0033:0x7f1a2cce6074
>> [ 67.832495] Code: 24 20 eb 8f 66 90 44 89 54 24 0c e8 86 f4 ff ff 44 8b 54 24 0c 44 89 e2 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 32 44 89 c7 89 44 24 0c e8 b8 f4 ff ff 8b 44
>> [ 67.834760] RSP: 002b:00007f19e4af46d0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
>> [ 67.837032] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f1a2cce6074
>> [ 67.839318] RDX: 0000000000000002 RSI: 00007f1a2d0bfb67 RDI: 00000000ffffff9c
>> [ 67.841598] RBP: 00007f1a2d0bfb67 R08: 0000000000000000 R09: 00007f19e4af4914
>> [ 67.843941] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000002
>> [ 67.846283] R13: 000000000000000d R14: 00007f19e4af4920 R15: 00007f1a2d0bfb67
>>
>> [ 67.850936] Allocated by task 1238:
>> [ 67.853241] save_stack+0x1b/0x80
>> [ 67.855533] __kasan_kmalloc.constprop.0+0xc2/0xd0
>> [ 67.857935] sk_prot_alloc+0x115/0x170
>> [ 67.860235] sk_alloc+0x2f/0xa10
>> [ 67.862541] tun_chr_open+0x4d/0x280 [tun]
>> [ 67.864894] misc_open+0x1cb/0x210
>> [ 67.867164] chrdev_open+0x15b/0x350
>> [ 67.869448] do_dentry_open+0x2cb/0x800
>> [ 67.871768] path_openat+0x858/0x14a0
>> [ 67.874041] do_filp_open+0x11e/0x1b0
>> [ 67.876328] do_sys_open+0x1f0/0x2d0
>> [ 67.878592] do_syscall_64+0x74/0xd0
>> [ 67.880899] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> [ 67.885431] Freed by task 726:
>> [ 67.887689] save_stack+0x1b/0x80
>> [ 67.889967] __kasan_slab_free+0x12c/0x170
>> [ 67.892197] kfree+0xff/0x430
>> [ 67.894444] uevent_show+0x176/0x1b0
>> [ 67.896709] dev_attr_show+0x37/0x70
>> [ 67.898940] sysfs_kf_seq_show+0x119/0x210
>> [ 67.901159] seq_read+0x29d/0x720
>> [ 67.903367] vfs_read+0xf9/0x1f0
>> [ 67.905538] ksys_read+0xc9/0x160
>> [ 67.907736] do_syscall_64+0x74/0xd0
>> [ 67.909889] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> [ 67.914100] The buggy address belongs to the object at ffff88870afe8000
>> which belongs to the cache kmalloc-4k of size 4096
>> [ 67.918357] The buggy address is located 2344 bytes inside of
>> 4096-byte region [ffff88870afe8000, ffff88870afe9000)
>> [ 67.922562] The buggy address belongs to the page:
>> [ 67.924725] page:ffffea001c2bfa00 refcount:1 mapcount:0 mapping:ffff88881f00de00 index:0x0 compound_mapcount: 0
>> [ 67.926926] raw: 0017ffe000010200 ffffea001c167a00 0000000200000002 ffff88881f00de00
>> [ 67.929144] raw: 0000000000000000 0000000080040004 00000001ffffffff 0000000000000000
>> [ 67.931362] page dumped because: kasan: bad access detected
>>
>> [ 67.936192] Memory state around the buggy address:
>> [ 67.938438] ffff88870afe8800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 67.941078] ffff88870afe8880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 67.943393] >ffff88870afe8900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 67.945709] ^
>> [ 67.948000] ffff88870afe8980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 67.950311] ffff88870afe8a00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 67.952629] ==================================================================
^ permalink raw reply
* KASAN slab-out-of-bounds in tun_chr_open/sock_init_data (Was: Re: [PATCH v14 00/23] LSM: Module stacking for AppArmor)
From: Stephen Smalley @ 2020-01-27 16:56 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, lorenzo,
David S. Miller, amade, Linux Kernel Network Developers, maxk
In-Reply-To: <de97dc66-7f5b-21f0-cf3d-a1485acbc1c9@tycho.nsa.gov>
On 1/27/20 11:14 AM, Stephen Smalley wrote:
> On 1/24/20 4:49 PM, Casey Schaufler wrote:
>> On 1/24/2020 1:04 PM, Stephen Smalley wrote:
>>> On 1/23/20 7:22 PM, Casey Schaufler wrote:
>>>> This patchset provides the changes required for
>>>> the AppArmor security module to stack safely with any other.
>>>>
>>>> v14: Rebase to 5.5-rc5
>>>> Incorporate feedback from v13
>>>> - Use an array of audit rules (patch 0002)
>>>> - Significant change, removed Acks (patch 0002)
>>>> - Remove unneeded include (patch 0013)
>>>> - Use context.len correctly (patch 0015)
>>>> - Reorder code to be more sensible (patch 0016)
>>>> - Drop SO_PEERCONTEXT as it's not needed yet (patch 0023)
>>>
>>> I don't know for sure if this is your bug, but it happens every time
>>> I boot with your patches applied and not at all on stock v5.5-rc5 so
>>> here it is. Will try to bisect as time permits but not until next
>>> week. Trigger seems to be loading the tun driver.
>>
>> Thanks. I will have a look as well.
>
> Bisection led to the first patch in the series, "LSM: Infrastructure
> management of the sock security". Still not sure if the bug is in the
> patch itself or just being surfaced by it.
Looks like the bug is pre-existing to me and just exposed by your patch.
tun_chr_open() is creating a struct tun_file via sk_alloc() with its own
tun_proto with a custom .obj_size. It then passes the tun_file->socket
and ->sk fields to sock_init_data(). sock_init_data() assumes it can
safely use SOCK_INODE(sock) if sock is non-NULL, which means that it
presumes all such sockets were wrapped in a struct socket_alloc. But
this one wasn't. I don't know if that's a bug in the tun driver for not
wrapping its socket in a socket_alloc or in sock_init_data() for
assuming that all sockets it is passed have been so wrapped. KASAN is
tripping on this assignment in sock_init_data():
net/core/sock.c:
2871 sk->sk_uid = SOCK_INODE(sock)->i_uid;
This appears to have been broken since commit
86741ec25462e4c8cdce6df2f41ead05568c7d5e ("net: core: Add a UID field to
struct sock.").
Previously reported here by someone else with RFC patches:
https://lore.kernel.org/netdev/20190929110502.2284-1-amade@asmblr.net/
>
>>>
>>> [ 67.726834] tun: Universal TUN/TAP device driver, 1.6
>>> [ 67.736657]
>>> ==================================================================
>>> [ 67.741335] BUG: KASAN: slab-out-of-bounds in
>>> sock_init_data+0x14a/0x5a0
>>> [ 67.745037] Read of size 4 at addr ffff88870afe8928 by task
>>> libvirtd/1238
>>>
>>> [ 67.751861] CPU: 4 PID: 1238 Comm: libvirtd Tainted: G T
>>> 5.5.0-rc5+ #54
>>> [ 67.756250] Call Trace:
>>> [ 67.759510] dump_stack+0xb8/0x110
>>> [ 67.761604] print_address_description.constprop.0+0x1f/0x280
>>> [ 67.763768] __kasan_report.cold+0x75/0x8f
>>> [ 67.765895] ? sock_init_data+0x14a/0x5a0
>>> [ 67.768282] kasan_report+0xe/0x20
>>> [ 67.770397] sock_init_data+0x14a/0x5a0
>>> [ 67.772511] tun_chr_open+0x1de/0x280 [tun]
>>> [ 67.774644] misc_open+0x1cb/0x210
>>> [ 67.776820] chrdev_open+0x15b/0x350
>>> [ 67.778917] ? cdev_put.part.0+0x30/0x30
>>> [ 67.781030] do_dentry_open+0x2cb/0x800
>>> [ 67.783135] ? cdev_put.part.0+0x30/0x30
>>> [ 67.785225] ? devcgroup_check_permission+0x11a/0x260
>>> [ 67.787321] ? __x64_sys_fchdir+0xf0/0xf0
>>> [ 67.789418] ? security_inode_permission+0x5b/0x70
>>> [ 67.791513] path_openat+0x858/0x14a0
>>> [ 67.793589] ? path_mountpoint+0x5e0/0x5e0
>>> [ 67.795719] ? mark_lock+0xb8/0xb00
>>> [ 67.797786] do_filp_open+0x11e/0x1b0
>>> [ 67.799840] ? may_open_dev+0x60/0x60
>>> [ 67.801871] ? match_held_lock+0x1b/0x240
>>> [ 67.803968] ? lock_downgrade+0x360/0x360
>>> [ 67.805997] ? do_raw_spin_lock+0x119/0x1d0
>>> [ 67.808041] ? rwlock_bug.part.0+0x60/0x60
>>> [ 67.810099] ? do_raw_spin_unlock+0xa3/0x130
>>> [ 67.812244] ? _raw_spin_unlock+0x1f/0x30
>>> [ 67.814287] ? __alloc_fd+0x143/0x2f0
>>> [ 67.816324] do_sys_open+0x1f0/0x2d0
>>> [ 67.818358] ? filp_open+0x50/0x50
>>> [ 67.820404] ? trace_hardirqs_on_thunk+0x1a/0x1c
>>> [ 67.822447] ? lockdep_hardirqs_off+0xbe/0x100
>>> [ 67.824473] ? mark_held_locks+0x24/0x90
>>> [ 67.826484] do_syscall_64+0x74/0xd0
>>> [ 67.828480] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>> [ 67.830478] RIP: 0033:0x7f1a2cce6074
>>> [ 67.832495] Code: 24 20 eb 8f 66 90 44 89 54 24 0c e8 86 f4 ff ff
>>> 44 8b 54 24 0c 44 89 e2 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00
>>> 00 0f 05 <48> 3d 00 f0 ff ff 77 32 44 89 c7 89 44 24 0c e8 b8 f4 ff
>>> ff 8b 44
>>> [ 67.834760] RSP: 002b:00007f19e4af46d0 EFLAGS: 00000293 ORIG_RAX:
>>> 0000000000000101
>>> [ 67.837032] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
>>> 00007f1a2cce6074
>>> [ 67.839318] RDX: 0000000000000002 RSI: 00007f1a2d0bfb67 RDI:
>>> 00000000ffffff9c
>>> [ 67.841598] RBP: 00007f1a2d0bfb67 R08: 0000000000000000 R09:
>>> 00007f19e4af4914
>>> [ 67.843941] R10: 0000000000000000 R11: 0000000000000293 R12:
>>> 0000000000000002
>>> [ 67.846283] R13: 000000000000000d R14: 00007f19e4af4920 R15:
>>> 00007f1a2d0bfb67
>>>
>>> [ 67.850936] Allocated by task 1238:
>>> [ 67.853241] save_stack+0x1b/0x80
>>> [ 67.855533] __kasan_kmalloc.constprop.0+0xc2/0xd0
>>> [ 67.857935] sk_prot_alloc+0x115/0x170
>>> [ 67.860235] sk_alloc+0x2f/0xa10
>>> [ 67.862541] tun_chr_open+0x4d/0x280 [tun]
>>> [ 67.864894] misc_open+0x1cb/0x210
>>> [ 67.867164] chrdev_open+0x15b/0x350
>>> [ 67.869448] do_dentry_open+0x2cb/0x800
>>> [ 67.871768] path_openat+0x858/0x14a0
>>> [ 67.874041] do_filp_open+0x11e/0x1b0
>>> [ 67.876328] do_sys_open+0x1f0/0x2d0
>>> [ 67.878592] do_syscall_64+0x74/0xd0
>>> [ 67.880899] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>>
>>> [ 67.885431] Freed by task 726:
>>> [ 67.887689] save_stack+0x1b/0x80
>>> [ 67.889967] __kasan_slab_free+0x12c/0x170
>>> [ 67.892197] kfree+0xff/0x430
>>> [ 67.894444] uevent_show+0x176/0x1b0
>>> [ 67.896709] dev_attr_show+0x37/0x70
>>> [ 67.898940] sysfs_kf_seq_show+0x119/0x210
>>> [ 67.901159] seq_read+0x29d/0x720
>>> [ 67.903367] vfs_read+0xf9/0x1f0
>>> [ 67.905538] ksys_read+0xc9/0x160
>>> [ 67.907736] do_syscall_64+0x74/0xd0
>>> [ 67.909889] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>>
>>> [ 67.914100] The buggy address belongs to the object at
>>> ffff88870afe8000
>>> which belongs to the cache kmalloc-4k of size 4096
>>> [ 67.918357] The buggy address is located 2344 bytes inside of
>>> 4096-byte region [ffff88870afe8000, ffff88870afe9000)
>>> [ 67.922562] The buggy address belongs to the page:
>>> [ 67.924725] page:ffffea001c2bfa00 refcount:1 mapcount:0
>>> mapping:ffff88881f00de00 index:0x0 compound_mapcount: 0
>>> [ 67.926926] raw: 0017ffe000010200 ffffea001c167a00
>>> 0000000200000002 ffff88881f00de00
>>> [ 67.929144] raw: 0000000000000000 0000000080040004
>>> 00000001ffffffff 0000000000000000
>>> [ 67.931362] page dumped because: kasan: bad access detected
>>>
>>> [ 67.936192] Memory state around the buggy address:
>>> [ 67.938438] ffff88870afe8800: 00 00 00 00 00 00 00 00 00 00 00 00
>>> 00 00 00 00
>>> [ 67.941078] ffff88870afe8880: fc fc fc fc fc fc fc fc fc fc fc fc
>>> fc fc fc fc
>>> [ 67.943393] >ffff88870afe8900: fc fc fc fc fc fc fc fc fc fc fc fc
>>> fc fc fc fc
>>> [ 67.945709] ^
>>> [ 67.948000] ffff88870afe8980: fc fc fc fc fc fc fc fc fc fc fc fc
>>> fc fc fc fc
>>> [ 67.950311] ffff88870afe8a00: fc fc fc fc fc fc fc fc fc fc fc fc
>>> fc fc fc fc
>>> [ 67.952629]
>>> ==================================================================
>
^ permalink raw reply
* [PATCH 0/8] ima: support stronger algorithms for attestation
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
proof to a remote verifier that the measurement list contains all
measurements done by the kernel and that the list was not maliciously
modified by an attacker.
IMA was originally designed to extend PCRs with a SHA1 digest, provided
with the measurement list, and was subsequently updated to extend all PCR
banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
used for remote attestation, as they are extended with a SHA1 digest padded
with zeros, which does not increase the strength.
This patch set addresses this issue by extending PCRs with the digest of
the measurement entry calculated with the crypto subsystem. The list of
algorithms used to calculate the digest are taken from
ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
is always calculated, as SHA1 still remains the default algorithm for the
template digest in the measurement list.
This patch set also makes two additional modifications related to the usage
of hash algorithms. First, since now the template digest for the default
IMA algorithm is always calculated, this is used for hash collision
detection, to check if there are duplicate measurement entries.
Second, it uses the default IMA hash algorithm to calculate the boot
aggregate, assuming that the corresponding PCR bank is currently allocated.
Otherwise, it finds the first PCR bank for which the crypto ID is known.
IMA initialization fails only if no algorithm known to the crypto subsystem
is found.
This patch set does not yet modify the format of the measurement list to
provide the digests passed to the TPM. However, reconstructing the value of
the quoted PCR is still possible for the verifier by calculating the digest
on measurement data found in binary_runtime_measurements.
The attest-tools library [1] has been updated to verify non-SHA1 PCR
banks [2] and to handle non-SHA1 boot aggregate [3].
[1] https://github.com/euleros/attest-tools/tree/0.2-devel
[2] https://github.com/euleros/attest-tools/commit/282a0b1a5e6d9c87adf21561018528d7bbdc7f38
[3] https://github.com/euleros/attest-tools/commit/3a4c8e250fde7661257aba022d677bf0af5399da
Roberto Sassu (8):
tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
ima: evaluate error in init_ima()
ima: store template digest directly in ima_template_entry
ima: switch to dynamically allocated buffer for template digests
ima: allocate and initialize tfm for each PCR bank
ima: calculate and extend PCR with digests in ima_template_entry
ima: use ima_hash_algo for collision detection in the measurement list
ima: switch to ima_hash_algo for boot aggregate
drivers/char/tpm/tpm2-cmd.c | 2 +
security/integrity/ima/ima.h | 7 +-
security/integrity/ima/ima_api.c | 20 ++-
security/integrity/ima/ima_crypto.c | 219 ++++++++++++++++++++------
security/integrity/ima/ima_fs.c | 4 +-
security/integrity/ima/ima_init.c | 6 +-
security/integrity/ima/ima_main.c | 6 +
security/integrity/ima/ima_queue.c | 36 +++--
security/integrity/ima/ima_template.c | 22 ++-
9 files changed, 241 insertions(+), 81 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
chip->allocated_banks contains the list of TPM algorithm IDs of allocated
PCR banks. It also contains the corresponding ID of the crypto subsystem,
so that users of the TPM driver can calculate a digest for a PCR extend
operation.
However, if there is no mapping between TPM algorithm ID and crypto ID, the
crypto_id field in chip->allocated_banks remains set to zero (the array is
allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
Zero should not be used as value for unknown mappings, as it is a valid
crypto ID (HASH_ALGO_MD4).
This patch initializes crypto_id to HASH_ALGO__LAST.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
drivers/char/tpm/tpm2-cmd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 13696deceae8..760329598b99 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -525,6 +525,8 @@ static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
return 0;
}
+ bank->crypto_id = HASH_ALGO__LAST;
+
return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
}
--
2.17.1
^ permalink raw reply related
* [PATCH 2/8] ima: evaluate error in init_ima()
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
Evaluate error in init_ima() before register_blocking_lsm_notifier() and
return if not zero.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9fe949c6a530..2f95b133f246 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -791,6 +791,9 @@ static int __init init_ima(void)
error = ima_init();
}
+ if (error)
+ return error;
+
error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
if (error)
pr_warn("Couldn't register LSM notifier, error %d\n", error);
--
2.17.1
^ permalink raw reply related
* [PATCH 3/8] ima: store template digest directly in ima_template_entry
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
In preparation for the patch that calculates a digest for each allocated
PCR bank, this patch passes to ima_calc_field_array_hash() the
ima_template_entry structure, so that digests can be directly stored in
that structure instead of ima_digest_data.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima.h | 3 +--
security/integrity/ima/ima_api.c | 12 +-----------
security/integrity/ima/ima_crypto.c | 18 +++++++-----------
3 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6bb3152b3e24..6238c50ae44e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -138,8 +138,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
int ima_calc_buffer_hash(const void *buf, loff_t len,
struct ima_digest_data *hash);
int ima_calc_field_array_hash(struct ima_field_data *field_data,
- struct ima_template_desc *desc, int num_fields,
- struct ima_digest_data *hash);
+ struct ima_template_entry *entry);
int __init ima_calc_boot_aggregate(struct ima_digest_data *hash);
void ima_add_violation(struct file *file, const unsigned char *filename,
struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index f6bc00914aa5..2ef5a40c7ca5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -96,26 +96,16 @@ int ima_store_template(struct ima_template_entry *entry,
static const char audit_cause[] = "hashing_error";
char *template_name = entry->template_desc->name;
int result;
- struct {
- struct ima_digest_data hdr;
- char digest[TPM_DIGEST_SIZE];
- } hash;
if (!violation) {
- int num_fields = entry->template_desc->num_fields;
-
- /* this function uses default algo */
- hash.hdr.algo = HASH_ALGO_SHA1;
result = ima_calc_field_array_hash(&entry->template_data[0],
- entry->template_desc,
- num_fields, &hash.hdr);
+ entry);
if (result < 0) {
integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
template_name, op,
audit_cause, result, 0);
return result;
}
- memcpy(entry->digest, hash.hdr.digest, hash.hdr.length);
}
entry->pcr = pcr;
result = ima_add_template_entry(entry, violation, op, inode, filename);
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7967a6904851..f2d9d4b18499 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -466,18 +466,16 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
* Calculate the hash of template data
*/
static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
- struct ima_template_desc *td,
- int num_fields,
- struct ima_digest_data *hash,
+ struct ima_template_entry *entry,
struct crypto_shash *tfm)
{
SHASH_DESC_ON_STACK(shash, tfm);
+ struct ima_template_desc *td = entry->template_desc;
+ int num_fields = entry->template_desc->num_fields;
int rc, i;
shash->tfm = tfm;
- hash->length = crypto_shash_digestsize(tfm);
-
rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
@@ -506,24 +504,22 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
}
if (!rc)
- rc = crypto_shash_final(shash, hash->digest);
+ rc = crypto_shash_final(shash, entry->digest);
return rc;
}
int ima_calc_field_array_hash(struct ima_field_data *field_data,
- struct ima_template_desc *desc, int num_fields,
- struct ima_digest_data *hash)
+ struct ima_template_entry *entry)
{
struct crypto_shash *tfm;
int rc;
- tfm = ima_alloc_tfm(hash->algo);
+ tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
if (IS_ERR(tfm))
return PTR_ERR(tfm);
- rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields,
- hash, tfm);
+ rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
ima_free_tfm(tfm);
--
2.17.1
^ permalink raw reply related
* [PATCH 4/8] ima: switch to dynamically allocated buffer for template digests
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
This patch dynamically allocates the array of tpm_digest structures in
ima_alloc_init_template() and ima_restore_template_data(). It also
allocates an item in addition to the number of allocated PCR banks, to
store a SHA1 digest when the SHA1 PCR bank is not allocated.
Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
hash algorithm for the measurement list. When IMA will support the Crypto
Agile format, remaining digests will be also provided.
The position in the array of the SHA1 digest is stored in the ima_sha1_idx
global variable and it is determined at IMA initialization time.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima.h | 3 ++-
security/integrity/ima/ima_api.c | 8 ++++++++
security/integrity/ima/ima_crypto.c | 3 ++-
security/integrity/ima/ima_fs.c | 4 ++--
security/integrity/ima/ima_main.c | 2 ++
security/integrity/ima/ima_queue.c | 10 ++++++----
security/integrity/ima/ima_template.c | 12 ++++++++++--
7 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6238c50ae44e..d959dab5bcce 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -50,6 +50,7 @@ extern int ima_policy_flag;
/* set during initialization */
extern int ima_hash_algo;
+extern int ima_sha1_idx;
extern int ima_appraise;
extern struct tpm_chip *ima_tpm_chip;
@@ -92,7 +93,7 @@ struct ima_template_desc {
struct ima_template_entry {
int pcr;
- u8 digest[TPM_DIGEST_SIZE]; /* sha1 or md5 measurement hash */
+ struct tpm_digest *digests;
struct ima_template_desc *template_desc; /* template descriptor */
u32 template_data_len;
struct ima_field_data template_data[0]; /* template related data */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 2ef5a40c7ca5..ebaf0056735c 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -27,6 +27,7 @@ void ima_free_template_entry(struct ima_template_entry *entry)
for (i = 0; i < entry->template_desc->num_fields; i++)
kfree(entry->template_data[i].data);
+ kfree(entry->digests);
kfree(entry);
}
@@ -50,6 +51,13 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
if (!*entry)
return -ENOMEM;
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ sizeof(*(*entry)->digests), GFP_NOFS);
+ if (!(*entry)->digests) {
+ result = -ENOMEM;
+ goto out;
+ }
+
(*entry)->template_desc = template_desc;
for (i = 0; i < template_desc->num_fields; i++) {
const struct ima_template_field *field =
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f2d9d4b18499..57b06321d5c8 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -504,7 +504,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
}
if (!rc)
- rc = crypto_shash_final(shash, entry->digest);
+ rc = crypto_shash_final(shash,
+ entry->digests[ima_sha1_idx].digest);
return rc;
}
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2000e8df0301..2b79581d0e25 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -152,7 +152,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
ima_putc(m, &pcr, sizeof(e->pcr));
/* 2nd: template digest */
- ima_putc(m, e->digest, TPM_DIGEST_SIZE);
+ ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
/* 3rd: template name size */
namelen = !ima_canonical_fmt ? strlen(template_name) :
@@ -235,7 +235,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
seq_printf(m, "%2d ", e->pcr);
/* 2nd: SHA1 template hash */
- ima_print_digest(m, e->digest, TPM_DIGEST_SIZE);
+ ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
/* 3th: template name */
seq_printf(m, " %s", template_name);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2f95b133f246..c068067a0c47 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -30,6 +30,8 @@
#include "ima.h"
+int ima_sha1_idx;
+
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise = IMA_APPRAISE_ENFORCE;
#else
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..bcd99db9722c 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,7 +57,8 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
key = ima_hash_key(digest_value);
rcu_read_lock();
hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
- rc = memcmp(qe->entry->digest, digest_value, TPM_DIGEST_SIZE);
+ rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
+ digest_value, TPM_DIGEST_SIZE);
if ((rc == 0) && (qe->entry->pcr == pcr)) {
ret = qe;
break;
@@ -77,7 +78,7 @@ static int get_binary_runtime_size(struct ima_template_entry *entry)
int size = 0;
size += sizeof(u32); /* pcr */
- size += sizeof(entry->digest);
+ size += TPM_DIGEST_SIZE;
size += sizeof(int); /* template name size field */
size += strlen(entry->template_desc->name);
size += sizeof(entry->template_data_len);
@@ -109,7 +110,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
atomic_long_inc(&ima_htable.len);
if (update_htable) {
- key = ima_hash_key(entry->digest);
+ key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
}
@@ -173,7 +174,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
mutex_lock(&ima_extend_list_mutex);
if (!violation) {
- memcpy(digest, entry->digest, sizeof(digest));
+ memcpy(digest, entry->digests[ima_sha1_idx].digest,
+ sizeof(digest));
if (ima_lookup_digest_entry(digest, entry->pcr)) {
audit_cause = "hash_exists";
result = -EEXIST;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 6aa6408603e3..2e9c5d99e70e 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,11 +311,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
if (!*entry)
return -ENOMEM;
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ sizeof(*(*entry)->digests), GFP_NOFS);
+ if (!(*entry)->digests) {
+ kfree(*entry);
+ return -ENOMEM;
+ }
+
ret = ima_parse_buf(template_data, template_data + template_data_size,
NULL, template_desc->num_fields,
(*entry)->template_data, NULL, NULL,
ENFORCE_FIELDS | ENFORCE_BUFEND, "template data");
if (ret < 0) {
+ kfree((*entry)->digests);
kfree(*entry);
return ret;
}
@@ -447,8 +455,8 @@ int ima_restore_measurement_list(loff_t size, void *buf)
if (ret < 0)
break;
- memcpy(entry->digest, hdr[HDR_DIGEST].data,
- hdr[HDR_DIGEST].len);
+ memcpy(entry->digests[ima_sha1_idx].digest,
+ hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
le32_to_cpu(*(hdr[HDR_PCR].data));
ret = ima_restore_measurement_entry(entry);
--
2.17.1
^ permalink raw reply related
* [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
This patch creates a crypto_shash structure for each allocated PCR bank and
for SHA1 if a bank with that algorithm is not currently allocated.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_crypto.c | 137 +++++++++++++++++++++++-----
1 file changed, 113 insertions(+), 24 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 57b06321d5c8..63fb4bdf80b0 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,7 +59,14 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
static struct crypto_shash *ima_shash_tfm;
static struct crypto_ahash *ima_ahash_tfm;
-int __init ima_init_crypto(void)
+struct ima_algo_desc {
+ struct crypto_shash *tfm;
+ enum hash_algo algo;
+};
+
+static struct ima_algo_desc *ima_algo_array;
+
+int __init ima_init_ima_crypto(void)
{
long rc;
@@ -78,26 +85,116 @@ int __init ima_init_crypto(void)
static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
{
struct crypto_shash *tfm = ima_shash_tfm;
- int rc;
+ int rc, i;
if (algo < 0 || algo >= HASH_ALGO__LAST)
algo = ima_hash_algo;
- if (algo != ima_hash_algo) {
- tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
- if (IS_ERR(tfm)) {
- rc = PTR_ERR(tfm);
- pr_err("Can not allocate %s (reason: %d)\n",
- hash_algo_name[algo], rc);
- }
+ if (algo == ima_hash_algo)
+ return tfm;
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
+ return ima_algo_array[i].tfm;
+
+ tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
+ if (IS_ERR(tfm)) {
+ rc = PTR_ERR(tfm);
+ pr_err("Can not allocate %s (reason: %d)\n",
+ hash_algo_name[algo], rc);
}
return tfm;
}
+int __init ima_init_crypto(void)
+{
+ enum hash_algo algo;
+ long rc;
+ int i;
+
+ rc = ima_init_ima_crypto();
+ if (rc)
+ return rc;
+
+ ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+ sizeof(*ima_algo_array), GFP_KERNEL);
+ if (!ima_algo_array) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ ima_algo_array[i].tfm = NULL;
+ ima_algo_array[i].algo = HASH_ALGO__LAST;
+ }
+
+ ima_sha1_idx = -1;
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
+ algo = ima_tpm_chip->allocated_banks[i].crypto_id;
+ ima_algo_array[i].algo = algo;
+
+ /* unknown TPM algorithm */
+ if (algo == HASH_ALGO__LAST)
+ continue;
+
+ if (algo == HASH_ALGO_SHA1)
+ ima_sha1_idx = i;
+
+ if (algo == ima_hash_algo) {
+ ima_algo_array[i].tfm = ima_shash_tfm;
+ continue;
+ }
+
+ ima_algo_array[i].tfm = ima_alloc_tfm(algo);
+ if (IS_ERR(ima_algo_array[i].tfm)) {
+ if (algo == HASH_ALGO_SHA1) {
+ rc = PTR_ERR(ima_algo_array[i].tfm);
+ ima_algo_array[i].tfm = NULL;
+ goto out_array;
+ }
+
+ ima_algo_array[i].tfm = NULL;
+ }
+ }
+
+ if (ima_sha1_idx < 0) {
+ ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
+ if (IS_ERR(ima_algo_array[i].tfm)) {
+ rc = PTR_ERR(ima_algo_array[i].tfm);
+ goto out_array;
+ }
+
+ ima_sha1_idx = i;
+ ima_algo_array[i].algo = HASH_ALGO_SHA1;
+ }
+
+ return 0;
+out_array:
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ if (!ima_algo_array[i].tfm ||
+ ima_algo_array[i].tfm == ima_shash_tfm)
+ continue;
+
+ crypto_free_shash(ima_algo_array[i].tfm);
+ }
+out:
+ crypto_free_shash(ima_shash_tfm);
+ return rc;
+}
+
static void ima_free_tfm(struct crypto_shash *tfm)
{
- if (tfm != ima_shash_tfm)
- crypto_free_shash(tfm);
+ int i;
+
+ if (tfm == ima_shash_tfm)
+ return;
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ if (ima_algo_array[i].tfm == tfm)
+ return;
+
+ crypto_free_shash(tfm);
}
/**
@@ -467,14 +564,14 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
*/
static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
struct ima_template_entry *entry,
- struct crypto_shash *tfm)
+ int tfm_idx)
{
- SHASH_DESC_ON_STACK(shash, tfm);
+ SHASH_DESC_ON_STACK(shash, ima_algo_array[tfm_idx].tfm);
struct ima_template_desc *td = entry->template_desc;
int num_fields = entry->template_desc->num_fields;
int rc, i;
- shash->tfm = tfm;
+ shash->tfm = ima_algo_array[tfm_idx].tfm;
rc = crypto_shash_init(shash);
if (rc != 0)
@@ -505,7 +602,7 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
if (!rc)
rc = crypto_shash_final(shash,
- entry->digests[ima_sha1_idx].digest);
+ entry->digests[tfm_idx].digest);
return rc;
}
@@ -513,17 +610,9 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
int ima_calc_field_array_hash(struct ima_field_data *field_data,
struct ima_template_entry *entry)
{
- struct crypto_shash *tfm;
int rc;
- tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
- if (IS_ERR(tfm))
- return PTR_ERR(tfm);
-
- rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
-
- ima_free_tfm(tfm);
-
+ rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
return rc;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 6/8] ima: calculate and extend PCR with digests in ima_template_entry
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
This patch modifies ima_calc_field_array_hash() to calculate a template
digest for each allocated PCR bank and SHA1. It also passes the tpm_digest
array of the template entry to ima_pcr_extend() or in case of a violation,
the pre-initialized digests array filled with 0xff.
Padding with zeros is still done if the mapping between TPM algorithm ID
and crypto ID is unknown.
This patch calculates again the template digest when a measurement list is
restored. Copying only the SHA1 digest (due to the limitation of the
current measurement list format) is not sufficient, as hash collision
detection will be done on the digest calculated with the default IMA hash
algorithm.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_crypto.c | 26 ++++++++++++++++++++++-
security/integrity/ima/ima_queue.c | 30 ++++++++++++++++-----------
security/integrity/ima/ima_template.c | 14 +++++++++++--
3 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 63fb4bdf80b0..786340feebbb 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -610,9 +610,33 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
int ima_calc_field_array_hash(struct ima_field_data *field_data,
struct ima_template_entry *entry)
{
- int rc;
+ u16 alg_id;
+ int rc, i;
rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
+ if (rc)
+ return rc;
+
+ entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ if (i == ima_sha1_idx)
+ continue;
+
+ alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+ entry->digests[i].alg_id = alg_id;
+
+ if (!ima_algo_array[i].tfm) {
+ memcpy(entry->digests[i].digest,
+ entry->digests[ima_sha1_idx].digest,
+ TPM_DIGEST_SIZE);
+ continue;
+ }
+
+ rc = ima_calc_field_array_hash_tfm(field_data, entry, i);
+ if (rc)
+ return rc;
+ }
return rc;
}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index bcd99db9722c..7f7509774b85 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -137,18 +137,14 @@ unsigned long ima_get_binary_runtime_size(void)
return binary_runtime_size + sizeof(struct ima_kexec_hdr);
};
-static int ima_pcr_extend(const u8 *hash, int pcr)
+static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
{
int result = 0;
- int i;
if (!ima_tpm_chip)
return result;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
- memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE);
-
- result = tpm_pcr_extend(ima_tpm_chip, pcr, digests);
+ result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
if (result != 0)
pr_err("Error Communicating to TPM chip, result: %d\n", result);
return result;
@@ -166,7 +162,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
const unsigned char *filename)
{
- u8 digest[TPM_DIGEST_SIZE];
+ u8 *digest = entry->digests[ima_sha1_idx].digest;
+ struct tpm_digest *digests_arg = entry->digests;
const char *audit_cause = "hash_added";
char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];
int audit_info = 1;
@@ -174,8 +171,6 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
mutex_lock(&ima_extend_list_mutex);
if (!violation) {
- memcpy(digest, entry->digests[ima_sha1_idx].digest,
- sizeof(digest));
if (ima_lookup_digest_entry(digest, entry->pcr)) {
audit_cause = "hash_exists";
result = -EEXIST;
@@ -191,9 +186,9 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
}
if (violation) /* invalidate pcr */
- memset(digest, 0xff, sizeof(digest));
+ digests_arg = digests;
- tpmresult = ima_pcr_extend(digest, entry->pcr);
+ tpmresult = ima_pcr_extend(digests_arg, entry->pcr);
if (tpmresult != 0) {
snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)",
tpmresult);
@@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
int __init ima_init_digests(void)
{
+ u16 digest_size;
+ u16 crypto_id;
int i;
if (!ima_tpm_chip)
@@ -229,8 +226,17 @@ int __init ima_init_digests(void)
if (!digests)
return -ENOMEM;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+ digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
+ crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
+
+ /* for unmapped TPM algorithms digest is still a padded SHA1 */
+ if (crypto_id == HASH_ALGO__LAST)
+ digest_size = SHA1_DIGEST_SIZE;
+
+ memset(digests[i].digest, 0xff, digest_size);
+ }
return 0;
}
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 2e9c5d99e70e..a9e1390c8e12 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -356,6 +356,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
int ima_restore_measurement_list(loff_t size, void *buf)
{
char template_name[MAX_TEMPLATE_NAME_LEN];
+ unsigned char zero[TPM_DIGEST_SIZE] = { 0 };
struct ima_kexec_hdr *khdr = buf;
struct ima_field_data hdr[HDR__LAST] = {
@@ -455,8 +456,17 @@ int ima_restore_measurement_list(loff_t size, void *buf)
if (ret < 0)
break;
- memcpy(entry->digests[ima_sha1_idx].digest,
- hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
+ if (memcmp(hdr[HDR_DIGEST].data, zero, sizeof(zero))) {
+ ret = ima_calc_field_array_hash(
+ &entry->template_data[0],
+ entry);
+ if (ret < 0) {
+ pr_err("cannot calculate template digest\n");
+ ret = -EINVAL;
+ break;
+ }
+ }
+
entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
le32_to_cpu(*(hdr[HDR_PCR].data));
ret = ima_restore_measurement_entry(entry);
--
2.17.1
^ permalink raw reply related
* [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Roberto Sassu @ 2020-01-27 17:04 UTC (permalink / raw)
To: zohar, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu,
Roberto Sassu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
Before calculating a digest for each PCR bank, collisions were detected
with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
used to calculate the template digest and checks collisions on that digest.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_crypto.c | 25 ++++++++++++++++++-------
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_queue.c | 8 ++++----
security/integrity/ima/ima_template.c | 2 +-
6 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d959dab5bcce..3e1afa5150bc 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -51,6 +51,7 @@ extern int ima_policy_flag;
/* set during initialization */
extern int ima_hash_algo;
extern int ima_sha1_idx;
+extern int ima_hash_algo_idx;
extern int ima_appraise;
extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index ebaf0056735c..a9bb45de6db9 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
if (!*entry)
return -ENOMEM;
- (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*(*entry)->digests), GFP_NOFS);
if (!(*entry)->digests) {
result = -ENOMEM;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 786340feebbb..f84dfd8fc5ca 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -93,7 +93,7 @@ static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
if (algo == ima_hash_algo)
return tfm;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
return ima_algo_array[i].tfm;
@@ -116,19 +116,20 @@ int __init ima_init_crypto(void)
if (rc)
return rc;
- ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+ ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*ima_algo_array), GFP_KERNEL);
if (!ima_algo_array) {
rc = -ENOMEM;
goto out;
}
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
ima_algo_array[i].tfm = NULL;
ima_algo_array[i].algo = HASH_ALGO__LAST;
}
ima_sha1_idx = -1;
+ ima_hash_algo_idx = -1;
for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
algo = ima_tpm_chip->allocated_banks[i].crypto_id;
@@ -143,6 +144,7 @@ int __init ima_init_crypto(void)
if (algo == ima_hash_algo) {
ima_algo_array[i].tfm = ima_shash_tfm;
+ ima_hash_algo_idx = i;
continue;
}
@@ -166,12 +168,21 @@ int __init ima_init_crypto(void)
}
ima_sha1_idx = i;
- ima_algo_array[i].algo = HASH_ALGO_SHA1;
+ if (ima_hash_algo == HASH_ALGO_SHA1)
+ ima_hash_algo_idx = i;
+
+ ima_algo_array[i++].algo = HASH_ALGO_SHA1;
+ }
+
+ if (ima_hash_algo_idx < 0) {
+ ima_algo_array[i].tfm = ima_shash_tfm;
+ ima_algo_array[i].algo = ima_hash_algo;
+ ima_hash_algo_idx = i;
}
return 0;
out_array:
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
if (!ima_algo_array[i].tfm ||
ima_algo_array[i].tfm == ima_shash_tfm)
continue;
@@ -190,7 +201,7 @@ static void ima_free_tfm(struct crypto_shash *tfm)
if (tfm == ima_shash_tfm)
return;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
if (ima_algo_array[i].tfm == tfm)
return;
@@ -619,7 +630,7 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
if (i == ima_sha1_idx)
continue;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c068067a0c47..6963d6c31bf1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -31,6 +31,7 @@
#include "ima.h"
int ima_sha1_idx;
+int ima_hash_algo_idx;
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise = IMA_APPRAISE_ENFORCE;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 7f7509774b85..58983d0f0214 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,8 +57,8 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
key = ima_hash_key(digest_value);
rcu_read_lock();
hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
- rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
- digest_value, TPM_DIGEST_SIZE);
+ rc = memcmp(qe->entry->digests[ima_hash_algo_idx].digest,
+ digest_value, hash_digest_size[ima_hash_algo]);
if ((rc == 0) && (qe->entry->pcr == pcr)) {
ret = qe;
break;
@@ -110,7 +110,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
atomic_long_inc(&ima_htable.len);
if (update_htable) {
- key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
+ key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
}
@@ -162,7 +162,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
const unsigned char *filename)
{
- u8 *digest = entry->digests[ima_sha1_idx].digest;
+ u8 *digest = entry->digests[ima_hash_algo_idx].digest;
struct tpm_digest *digests_arg = entry->digests;
const char *audit_cause = "hash_added";
char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index a9e1390c8e12..f71b5ee44179 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,7 +311,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
if (!*entry)
return -ENOMEM;
- (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*(*entry)->digests), GFP_NOFS);
if (!(*entry)->digests) {
kfree(*entry);
--
2.17.1
^ permalink raw reply related
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