* Re: Please revert SELinux/keys patches from the keys linux-next branch
From: David Howells @ 2020-02-28 15:37 UTC (permalink / raw)
To: Paul Moore; +Cc: dhowells, selinux, linux-security-module
In-Reply-To: <CAHC9VhQ=W4R2LGCxaKzVEx4J31m4-F7mDo2BOMTqso2JdScHzw@mail.gmail.com>
Paul Moore <paul@paul-moore.com> wrote:
> For some reason we haven't been able to get your attention on the
> related SELinux mailing list threads, but we need you to revert commit
> f981a85690dc ("security/selinux: Add support for new key permissions")
> from your linux-next branch. Can you please do that?
Sorry, I had to squeeze out a new version of notifications and fsinfo before
disappearing off to Vault - and then I disappeared off to Vault. However, I
can do that now.
I can drop keyring ACL patches for the moment and pick them back up after the
next merge window.
David
^ permalink raw reply
* Re: Please revert SELinux/keys patches from the keys linux-next branch
From: Paul Moore @ 2020-02-28 15:39 UTC (permalink / raw)
To: David Howells; +Cc: selinux, linux-security-module
In-Reply-To: <101039.1582904234@warthog.procyon.org.uk>
On Fri, Feb 28, 2020 at 10:37 AM David Howells <dhowells@redhat.com> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>
> > For some reason we haven't been able to get your attention on the
> > related SELinux mailing list threads, but we need you to revert commit
> > f981a85690dc ("security/selinux: Add support for new key permissions")
> > from your linux-next branch. Can you please do that?
>
> Sorry, I had to squeeze out a new version of notifications and fsinfo before
> disappearing off to Vault - and then I disappeared off to Vault. However, I
> can do that now.
>
> I can drop keyring ACL patches for the moment and pick them back up after the
> next merge window.
Great, thanks. For future reference, I would *really* prefer if
patches like this one went up to Linus via the SELinux tree as it
changes the SELinux kernel ABI (the keys and selinux next trees
currently conflict because of this).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* protecting overlayfs's lower consistently.
From: Alexander Ivanov @ 2020-02-28 18:31 UTC (permalink / raw)
To: linux-security-module
We have LSM that implements file_open call back to protect fs object based on path. Now, supposed are to protect /mnt/l in the following setup
mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w none /mnt/m
However, when one attempts to change the upper (for the objects that originates on lower) the dentry passed into vfs_open() and then to do_dentry_open() points to the lower and there seems no easy way to calculate its upper. It
This seems was different in older kernels, eg. 3.10 (rhel7). The changes the seemingly broke that behaviour happened around kernel 4.4. What was a reason for that change and is there anyway to figure out the upper given the lower passed into vfs_open()?
Thanks,
--Alex
^ permalink raw reply
* [PATCH 0/3] proc: Actually honor the mount options
From: Eric W. Biederman @ 2020-02-28 20:17 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <871rqk2brn.fsf_-_@x220.int.ebiederm.org>
Proc mount option handling is broken, and it has been since I
accidentally broke it in the middle 2006.
The problem is that because we perform an internal mount of proc
before user space mounts proc all of the mount options that user
specifies when mounting proc are ignored.
You can set those mount options with a remount but that is rather
surprising.
This most directly affects android which is using hidpid=2 by default.
Now that the sysctl system call support has been removed, and we have
settled on way of flushing proc dentries when a process exits without
using proc_mnt, there is an simple and easy fix.
a) Give UML mconsole it's own private mount of proc to use.
b) Stop creating the internal mount of proc
We still need Alexey Gladkov's full patch to get proc mount options to
work inside of UML, and to be generally useful. This set of changes
is just enough to get them working as well as they have in the past.
If anyone sees any problem with this code please let me know.
Otherwise I plan to merge these set of fixes through my tree.
Link: https://lore.kernel.org/lkml/87r21tuulj.fsf@x220.int.ebiederm.org/
Link: https://lore.kernel.org/lkml/871rqk2brn.fsf_-_@x220.int.ebiederm.org/
Link: https://lore.kernel.org/lkml/20200210150519.538333-1-gladkov.alexey@gmail.com/
Link: https://lore.kernel.org/lkml/20180611195744.154962-1-astrachan@google.com/
Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns.")
Eric W. Biederman (3):
uml: Don't consult current to find the proc_mnt in mconsole_proc
uml: Create a private mount of proc for mconsole
proc: Remove the now unnecessary internal mount of proc
arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
fs/proc/root.c | 36 ------------------------------------
include/linux/pid_namespace.h | 2 --
include/linux/proc_ns.h | 5 -----
kernel/pid.c | 8 --------
kernel/pid_namespace.c | 7 -------
6 files changed, 27 insertions(+), 59 deletions(-)
Eric
^ permalink raw reply
* [PATCH 1/3] uml: Don't consult current to find the proc_mnt in mconsole_proc
From: Eric W. Biederman @ 2020-02-28 20:18 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <878skmsbyy.fsf_-_@x220.int.ebiederm.org>
Inspection of the control flow reveals that mconsole_proc is either
called from mconsole_stop called from mc_work_proc or from
mc_work_proc directly. The function mc_work_proc is dispatched to a
kernel thread with schedule_work.
All of the threads that run dispatched by schedule_work are in the
init pid namespace.
So make the code clearer and by using init_pid_ns instead of
task_active_pid_ns(current).
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
arch/um/drivers/mconsole_kern.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index b80a1d616e4e..e8f5c81c2c6c 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -123,7 +123,7 @@ void mconsole_log(struct mc_request *req)
void mconsole_proc(struct mc_request *req)
{
- struct vfsmount *mnt = task_active_pid_ns(current)->proc_mnt;
+ struct vfsmount *mnt = init_pid_ns.proc_mnt;
char *buf;
int len;
struct file *file;
--
2.25.0
^ permalink raw reply related
* [PATCH 2/3] uml: Create a private mount of proc for mconsole
From: Eric W. Biederman @ 2020-02-28 20:18 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <878skmsbyy.fsf_-_@x220.int.ebiederm.org>
The mconsole code only ever accesses proc for the initial pid
namespace. Instead of depending upon the proc_mnt which is
for proc_flush_task have uml create it's own mount of proc
instead.
This allows proc_flush_task to evolve and remove the
need for having a proc_mnt to do it's job.
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index e8f5c81c2c6c..30575bd92975 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -36,6 +36,8 @@
#include "mconsole_kern.h"
#include <os.h>
+static struct vfsmount *proc_mnt = NULL;
+
static int do_unlink_socket(struct notifier_block *notifier,
unsigned long what, void *data)
{
@@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
void mconsole_proc(struct mc_request *req)
{
- struct vfsmount *mnt = init_pid_ns.proc_mnt;
+ struct vfsmount *mnt = proc_mnt;
char *buf;
int len;
struct file *file;
@@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
ptr += strlen("proc");
ptr = skip_spaces(ptr);
+ if (!mnt) {
+ mconsole_reply(req, "Proc not available", 1, 0);
+ goto out;
+ }
file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
if (IS_ERR(file)) {
mconsole_reply(req, "Failed to open file", 1, 0);
@@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
with_console(req, stack_proc, to);
}
+static int __init mount_proc(void)
+{
+ struct file_system_type *proc_fs_type;
+ struct vfsmount *mnt;
+
+ proc_fs_type = get_fs_type("proc");
+ if (!proc_fs_type)
+ return -ENODEV;
+
+ mnt = kern_mount(proc_fs_type);
+ put_filesystem(proc_fs_type);
+ if (IS_ERR(mnt))
+ return PTR_ERR(mnt);
+
+ proc_mnt = mnt;
+ return 0;
+}
+
/*
* Changed by mconsole_setup, which is __setup, and called before SMP is
* active.
@@ -696,6 +720,8 @@ static int __init mconsole_init(void)
int err;
char file[UNIX_PATH_MAX];
+ mount_proc();
+
if (umid_file_name("mconsole", file, sizeof(file)))
return -1;
snprintf(mconsole_socket_name, sizeof(file), "%s", file);
--
2.25.0
^ permalink raw reply related
* [PATCH 3/3] proc: Remove the now unnecessary internal mount of proc
From: Eric W. Biederman @ 2020-02-28 20:19 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <878skmsbyy.fsf_-_@x220.int.ebiederm.org>
There remains no more code in the kernel using pids_ns->proc_mnt,
therefore remove it from the kernel.
The big benefit of this change is that one of the most error prone and
tricky parts of the pid namespace implementation, maintaining kernel
mounts of proc is removed.
In addition removing the unnecessary complexity of the kernel mount
fixes a regression that caused the proc mount options to be ignored.
Now that the initial mount of proc comes from userspace, those mount
options are again honored. This fixes Android's usage of the proc
hidepid option.
Reported-by: Alistair Strachan <astrachan@google.com>
Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns.")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/proc/root.c | 36 -----------------------------------
include/linux/pid_namespace.h | 2 --
include/linux/proc_ns.h | 5 -----
kernel/pid.c | 8 --------
kernel/pid_namespace.c | 7 -------
5 files changed, 58 deletions(-)
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 608233dfd29c..2633f10446c3 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -292,39 +292,3 @@ struct proc_dir_entry proc_root = {
.subdir = RB_ROOT,
.name = "/proc",
};
-
-int pid_ns_prepare_proc(struct pid_namespace *ns)
-{
- struct proc_fs_context *ctx;
- struct fs_context *fc;
- struct vfsmount *mnt;
-
- fc = fs_context_for_mount(&proc_fs_type, SB_KERNMOUNT);
- if (IS_ERR(fc))
- return PTR_ERR(fc);
-
- if (fc->user_ns != ns->user_ns) {
- put_user_ns(fc->user_ns);
- fc->user_ns = get_user_ns(ns->user_ns);
- }
-
- ctx = fc->fs_private;
- if (ctx->pid_ns != ns) {
- put_pid_ns(ctx->pid_ns);
- get_pid_ns(ns);
- ctx->pid_ns = ns;
- }
-
- mnt = fc_mount(fc);
- put_fs_context(fc);
- if (IS_ERR(mnt))
- return PTR_ERR(mnt);
-
- ns->proc_mnt = mnt;
- return 0;
-}
-
-void pid_ns_release_proc(struct pid_namespace *ns)
-{
- kern_unmount(ns->proc_mnt);
-}
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 2ed6af88794b..4956e362e55e 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -33,7 +33,6 @@ 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;
#endif
@@ -42,7 +41,6 @@ struct pid_namespace {
#endif
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 */
diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index 4626b1ac3b6c..e1106a077c1a 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -50,16 +50,11 @@ enum {
#ifdef CONFIG_PROC_FS
-extern int pid_ns_prepare_proc(struct pid_namespace *ns);
-extern void pid_ns_release_proc(struct pid_namespace *ns);
extern int proc_alloc_inum(unsigned int *pino);
extern void proc_free_inum(unsigned int inum);
#else /* CONFIG_PROC_FS */
-static inline int pid_ns_prepare_proc(struct pid_namespace *ns) { return 0; }
-static inline void pid_ns_release_proc(struct pid_namespace *ns) {}
-
static inline int proc_alloc_inum(unsigned int *inum)
{
*inum = 1;
diff --git a/kernel/pid.c b/kernel/pid.c
index ca08d6a3aa77..60820e72634c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -144,9 +144,6 @@ void free_pid(struct pid *pid)
/* Handle a fork failure of the first process */
WARN_ON(ns->child_reaper);
ns->pid_allocated = 0;
- /* fall through */
- case 0:
- schedule_work(&ns->proc_work);
break;
}
@@ -247,11 +244,6 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
tmp = tmp->parent;
}
- if (unlikely(is_child_reaper(pid))) {
- if (pid_ns_prepare_proc(ns))
- goto out_free;
- }
-
get_pid_ns(ns);
refcount_set(&pid->count, 1);
for (type = 0; type < PIDTYPE_MAX; ++type)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index d40017e79ebe..318fcc6ba301 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -57,12 +57,6 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
return READ_ONCE(*pkc);
}
-static void proc_cleanup_work(struct work_struct *work)
-{
- struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_work);
- pid_ns_release_proc(ns);
-}
-
static struct ucounts *inc_pid_namespaces(struct user_namespace *ns)
{
return inc_ucount(ns, current_euid(), UCOUNT_PID_NAMESPACES);
@@ -114,7 +108,6 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
ns->user_ns = get_user_ns(user_ns);
ns->ucounts = ucounts;
ns->pid_allocated = PIDNS_ADDING;
- INIT_WORK(&ns->proc_work, proc_cleanup_work);
return ns;
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 2/3] uml: Create a private mount of proc for mconsole
From: Christian Brauner @ 2020-02-28 20:30 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <87wo86qxcs.fsf_-_@x220.int.ebiederm.org>
On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
>
> The mconsole code only ever accesses proc for the initial pid
> namespace. Instead of depending upon the proc_mnt which is
> for proc_flush_task have uml create it's own mount of proc
> instead.
>
> This allows proc_flush_task to evolve and remove the
> need for having a proc_mnt to do it's job.
>
> Cc: Jeff Dike <jdike@addtoit.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
> index e8f5c81c2c6c..30575bd92975 100644
> --- a/arch/um/drivers/mconsole_kern.c
> +++ b/arch/um/drivers/mconsole_kern.c
> @@ -36,6 +36,8 @@
> #include "mconsole_kern.h"
> #include <os.h>
>
> +static struct vfsmount *proc_mnt = NULL;
> +
> static int do_unlink_socket(struct notifier_block *notifier,
> unsigned long what, void *data)
> {
> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
>
> void mconsole_proc(struct mc_request *req)
> {
> - struct vfsmount *mnt = init_pid_ns.proc_mnt;
> + struct vfsmount *mnt = proc_mnt;
> char *buf;
> int len;
> struct file *file;
> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
> ptr += strlen("proc");
> ptr = skip_spaces(ptr);
>
> + if (!mnt) {
> + mconsole_reply(req, "Proc not available", 1, 0);
> + goto out;
> + }
> file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
> if (IS_ERR(file)) {
> mconsole_reply(req, "Failed to open file", 1, 0);
> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
> with_console(req, stack_proc, to);
> }
>
> +static int __init mount_proc(void)
> +{
> + struct file_system_type *proc_fs_type;
> + struct vfsmount *mnt;
> +
> + proc_fs_type = get_fs_type("proc");
> + if (!proc_fs_type)
> + return -ENODEV;
> +
> + mnt = kern_mount(proc_fs_type);
> + put_filesystem(proc_fs_type);
> + if (IS_ERR(mnt))
> + return PTR_ERR(mnt);
> +
> + proc_mnt = mnt;
> + return 0;
> +}
> +
> /*
> * Changed by mconsole_setup, which is __setup, and called before SMP is
> * active.
> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
> int err;
> char file[UNIX_PATH_MAX];
>
> + mount_proc();
Hm, either check the return value or make the mount_proc() void?
Probably worth logging something but moving on without proc.
I guess this is user visible in some scenarios but the patch series
seems worth it!
Christian
^ permalink raw reply
* Re: [PATCH 3/3] proc: Remove the now unnecessary internal mount of proc
From: Christian Brauner @ 2020-02-28 20:39 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <87r1yeqxbp.fsf_-_@x220.int.ebiederm.org>
On Fri, Feb 28, 2020 at 02:19:22PM -0600, Eric W. Biederman wrote:
>
> There remains no more code in the kernel using pids_ns->proc_mnt,
> therefore remove it from the kernel.
>
> The big benefit of this change is that one of the most error prone and
> tricky parts of the pid namespace implementation, maintaining kernel
> mounts of proc is removed.
>
> In addition removing the unnecessary complexity of the kernel mount
> fixes a regression that caused the proc mount options to be ignored.
> Now that the initial mount of proc comes from userspace, those mount
> options are again honored. This fixes Android's usage of the proc
> hidepid option.
>
> Reported-by: Alistair Strachan <astrachan@google.com>
> Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns.")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> fs/proc/root.c | 36 -----------------------------------
What about proc_flush_task()? Or is this on top of your other series?
> include/linux/pid_namespace.h | 2 --
> include/linux/proc_ns.h | 5 -----
> kernel/pid.c | 8 --------
> kernel/pid_namespace.c | 7 -------
> 5 files changed, 58 deletions(-)
>
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 608233dfd29c..2633f10446c3 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -292,39 +292,3 @@ struct proc_dir_entry proc_root = {
> .subdir = RB_ROOT,
> .name = "/proc",
> };
> -
> -int pid_ns_prepare_proc(struct pid_namespace *ns)
> -{
> - struct proc_fs_context *ctx;
> - struct fs_context *fc;
> - struct vfsmount *mnt;
> -
> - fc = fs_context_for_mount(&proc_fs_type, SB_KERNMOUNT);
> - if (IS_ERR(fc))
> - return PTR_ERR(fc);
> -
> - if (fc->user_ns != ns->user_ns) {
> - put_user_ns(fc->user_ns);
> - fc->user_ns = get_user_ns(ns->user_ns);
> - }
> -
> - ctx = fc->fs_private;
> - if (ctx->pid_ns != ns) {
> - put_pid_ns(ctx->pid_ns);
> - get_pid_ns(ns);
> - ctx->pid_ns = ns;
> - }
> -
> - mnt = fc_mount(fc);
> - put_fs_context(fc);
> - if (IS_ERR(mnt))
> - return PTR_ERR(mnt);
> -
> - ns->proc_mnt = mnt;
> - return 0;
> -}
> -
> -void pid_ns_release_proc(struct pid_namespace *ns)
> -{
> - kern_unmount(ns->proc_mnt);
> -}
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 2ed6af88794b..4956e362e55e 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -33,7 +33,6 @@ 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;
> #endif
> @@ -42,7 +41,6 @@ struct pid_namespace {
> #endif
> 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 */
> diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
> index 4626b1ac3b6c..e1106a077c1a 100644
> --- a/include/linux/proc_ns.h
> +++ b/include/linux/proc_ns.h
> @@ -50,16 +50,11 @@ enum {
>
> #ifdef CONFIG_PROC_FS
>
> -extern int pid_ns_prepare_proc(struct pid_namespace *ns);
> -extern void pid_ns_release_proc(struct pid_namespace *ns);
> extern int proc_alloc_inum(unsigned int *pino);
> extern void proc_free_inum(unsigned int inum);
>
> #else /* CONFIG_PROC_FS */
>
> -static inline int pid_ns_prepare_proc(struct pid_namespace *ns) { return 0; }
> -static inline void pid_ns_release_proc(struct pid_namespace *ns) {}
> -
> static inline int proc_alloc_inum(unsigned int *inum)
> {
> *inum = 1;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index ca08d6a3aa77..60820e72634c 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -144,9 +144,6 @@ void free_pid(struct pid *pid)
> /* Handle a fork failure of the first process */
> WARN_ON(ns->child_reaper);
> ns->pid_allocated = 0;
> - /* fall through */
> - case 0:
> - schedule_work(&ns->proc_work);
> break;
> }
>
> @@ -247,11 +244,6 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
> tmp = tmp->parent;
> }
>
> - if (unlikely(is_child_reaper(pid))) {
> - if (pid_ns_prepare_proc(ns))
> - goto out_free;
> - }
> -
> get_pid_ns(ns);
> refcount_set(&pid->count, 1);
> for (type = 0; type < PIDTYPE_MAX; ++type)
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index d40017e79ebe..318fcc6ba301 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -57,12 +57,6 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
> return READ_ONCE(*pkc);
> }
>
> -static void proc_cleanup_work(struct work_struct *work)
There's a comment in kernel/pid_namespace.c that references
proc_cleanup_work(). Can you please remove that as well?
> -{
> - struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_work);
> - pid_ns_release_proc(ns);
> -}
> -
> static struct ucounts *inc_pid_namespaces(struct user_namespace *ns)
> {
> return inc_ucount(ns, current_euid(), UCOUNT_PID_NAMESPACES);
> @@ -114,7 +108,6 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
> ns->user_ns = get_user_ns(user_ns);
> ns->ucounts = ucounts;
> ns->pid_allocated = PIDNS_ADDING;
> - INIT_WORK(&ns->proc_work, proc_cleanup_work);
>
> return ns;
>
> --
> 2.25.0
>
^ permalink raw reply
* Re: [PATCH 2/3] uml: Create a private mount of proc for mconsole
From: Eric W. Biederman @ 2020-02-28 21:28 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <20200228203058.jcnqeyvmqhfslcym@wittgenstein>
Christian Brauner <christian.brauner@ubuntu.com> writes:
> On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
>>
>> The mconsole code only ever accesses proc for the initial pid
>> namespace. Instead of depending upon the proc_mnt which is
>> for proc_flush_task have uml create it's own mount of proc
>> instead.
>>
>> This allows proc_flush_task to evolve and remove the
>> need for having a proc_mnt to do it's job.
>>
>> Cc: Jeff Dike <jdike@addtoit.com>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>> ---
>> arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
>> index e8f5c81c2c6c..30575bd92975 100644
>> --- a/arch/um/drivers/mconsole_kern.c
>> +++ b/arch/um/drivers/mconsole_kern.c
>> @@ -36,6 +36,8 @@
>> #include "mconsole_kern.h"
>> #include <os.h>
>>
>> +static struct vfsmount *proc_mnt = NULL;
>> +
>> static int do_unlink_socket(struct notifier_block *notifier,
>> unsigned long what, void *data)
>> {
>> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
>>
>> void mconsole_proc(struct mc_request *req)
>> {
>> - struct vfsmount *mnt = init_pid_ns.proc_mnt;
>> + struct vfsmount *mnt = proc_mnt;
>> char *buf;
>> int len;
>> struct file *file;
>> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
>> ptr += strlen("proc");
>> ptr = skip_spaces(ptr);
>>
>> + if (!mnt) {
>> + mconsole_reply(req, "Proc not available", 1, 0);
>> + goto out;
>> + }
>> file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
>> if (IS_ERR(file)) {
>> mconsole_reply(req, "Failed to open file", 1, 0);
>> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
>> with_console(req, stack_proc, to);
>> }
>>
>> +static int __init mount_proc(void)
>> +{
>> + struct file_system_type *proc_fs_type;
>> + struct vfsmount *mnt;
>> +
>> + proc_fs_type = get_fs_type("proc");
>> + if (!proc_fs_type)
>> + return -ENODEV;
>> +
>> + mnt = kern_mount(proc_fs_type);
>> + put_filesystem(proc_fs_type);
>> + if (IS_ERR(mnt))
>> + return PTR_ERR(mnt);
>> +
>> + proc_mnt = mnt;
>> + return 0;
>> +}
>> +
>> /*
>> * Changed by mconsole_setup, which is __setup, and called before SMP is
>> * active.
>> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
>> int err;
>> char file[UNIX_PATH_MAX];
>>
>> + mount_proc();
>
> Hm, either check the return value or make the mount_proc() void?
> Probably worth logging something but moving on without proc.
I modified mconsole_proc (the only place that cares to see if
it has a valid proc_mnt).
So the code already does the moving on without mounting proc
and continues to work.
Further the code logs something when it tries to use the mount
of proc and proc is not available.
I think this can happen if someone is strange enough to compile
the kernel without proc. So at least in some scenarios I believe
it is expected that it will fail.
So while I think it is good form to generate good error codes in
the incredibly unlikely case that proc_mount() fails during boot
I don't see the point of doing anything with them.
> I guess this is user visible in some scenarios but the patch series
> seems worth it!
What scenarios do you think this would be user visible?
The set of calls to mount proc are slightly different, but the options
to proc when mounting (none) remain the same.
For the series as a whole the only place where it should be user visible
is when the proc mount options start getting honored. AKA when
hidepid=N starts working as designed again.
Eric
^ permalink raw reply
* Re: [PATCH 3/3] proc: Remove the now unnecessary internal mount of proc
From: Eric W. Biederman @ 2020-02-28 21:40 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <20200228203915.jelui3l5xue5utpx@wittgenstein>
Christian Brauner <christian.brauner@ubuntu.com> writes:
> On Fri, Feb 28, 2020 at 02:19:22PM -0600, Eric W. Biederman wrote:
>>
>> There remains no more code in the kernel using pids_ns->proc_mnt,
>> therefore remove it from the kernel.
>>
>> The big benefit of this change is that one of the most error prone and
>> tricky parts of the pid namespace implementation, maintaining kernel
>> mounts of proc is removed.
>>
>> In addition removing the unnecessary complexity of the kernel mount
>> fixes a regression that caused the proc mount options to be ignored.
>> Now that the initial mount of proc comes from userspace, those mount
>> options are again honored. This fixes Android's usage of the proc
>> hidepid option.
>>
>> Reported-by: Alistair Strachan <astrachan@google.com>
>> Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns.")
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> fs/proc/root.c | 36 -----------------------------------
>
> What about proc_flush_task()? Or is this on top of your other series?
On top of the other series. Which is why it is a reply to it.
That I have pushed into linux-next earlier today.
No one seems worried enough about that patchset to comment on v2 so I am
just going with it.
>> include/linux/pid_namespace.h | 2 --
>> include/linux/proc_ns.h | 5 -----
>> kernel/pid.c | 8 --------
>> kernel/pid_namespace.c | 7 -------
>> 5 files changed, 58 deletions(-)
>>
>> diff --git a/fs/proc/root.c b/fs/proc/root.c
>> index 608233dfd29c..2633f10446c3 100644
>> --- a/fs/proc/root.c
>> +++ b/fs/proc/root.c
>> @@ -292,39 +292,3 @@ struct proc_dir_entry proc_root = {
>> .subdir = RB_ROOT,
>> .name = "/proc",
>> };
>> -
>> -int pid_ns_prepare_proc(struct pid_namespace *ns)
>> -{
>> - struct proc_fs_context *ctx;
>> - struct fs_context *fc;
>> - struct vfsmount *mnt;
>> -
>> - fc = fs_context_for_mount(&proc_fs_type, SB_KERNMOUNT);
>> - if (IS_ERR(fc))
>> - return PTR_ERR(fc);
>> -
>> - if (fc->user_ns != ns->user_ns) {
>> - put_user_ns(fc->user_ns);
>> - fc->user_ns = get_user_ns(ns->user_ns);
>> - }
>> -
>> - ctx = fc->fs_private;
>> - if (ctx->pid_ns != ns) {
>> - put_pid_ns(ctx->pid_ns);
>> - get_pid_ns(ns);
>> - ctx->pid_ns = ns;
>> - }
>> -
>> - mnt = fc_mount(fc);
>> - put_fs_context(fc);
>> - if (IS_ERR(mnt))
>> - return PTR_ERR(mnt);
>> -
>> - ns->proc_mnt = mnt;
>> - return 0;
>> -}
>> -
>> -void pid_ns_release_proc(struct pid_namespace *ns)
>> -{
>> - kern_unmount(ns->proc_mnt);
>> -}
>> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
>> index 2ed6af88794b..4956e362e55e 100644
>> --- a/include/linux/pid_namespace.h
>> +++ b/include/linux/pid_namespace.h
>> @@ -33,7 +33,6 @@ 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;
>> #endif
>> @@ -42,7 +41,6 @@ struct pid_namespace {
>> #endif
>> 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 */
>> diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
>> index 4626b1ac3b6c..e1106a077c1a 100644
>> --- a/include/linux/proc_ns.h
>> +++ b/include/linux/proc_ns.h
>> @@ -50,16 +50,11 @@ enum {
>>
>> #ifdef CONFIG_PROC_FS
>>
>> -extern int pid_ns_prepare_proc(struct pid_namespace *ns);
>> -extern void pid_ns_release_proc(struct pid_namespace *ns);
>> extern int proc_alloc_inum(unsigned int *pino);
>> extern void proc_free_inum(unsigned int inum);
>>
>> #else /* CONFIG_PROC_FS */
>>
>> -static inline int pid_ns_prepare_proc(struct pid_namespace *ns) { return 0; }
>> -static inline void pid_ns_release_proc(struct pid_namespace *ns) {}
>> -
>> static inline int proc_alloc_inum(unsigned int *inum)
>> {
>> *inum = 1;
>> diff --git a/kernel/pid.c b/kernel/pid.c
>> index ca08d6a3aa77..60820e72634c 100644
>> --- a/kernel/pid.c
>> +++ b/kernel/pid.c
>> @@ -144,9 +144,6 @@ void free_pid(struct pid *pid)
>> /* Handle a fork failure of the first process */
>> WARN_ON(ns->child_reaper);
>> ns->pid_allocated = 0;
>> - /* fall through */
>> - case 0:
>> - schedule_work(&ns->proc_work);
>> break;
>> }
>>
>> @@ -247,11 +244,6 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
>> tmp = tmp->parent;
>> }
>>
>> - if (unlikely(is_child_reaper(pid))) {
>> - if (pid_ns_prepare_proc(ns))
>> - goto out_free;
>> - }
>> -
>> get_pid_ns(ns);
>> refcount_set(&pid->count, 1);
>> for (type = 0; type < PIDTYPE_MAX; ++type)
>> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
>> index d40017e79ebe..318fcc6ba301 100644
>> --- a/kernel/pid_namespace.c
>> +++ b/kernel/pid_namespace.c
>> @@ -57,12 +57,6 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
>> return READ_ONCE(*pkc);
>> }
>>
>> -static void proc_cleanup_work(struct work_struct *work)
>
> There's a comment in kernel/pid_namespace.c that references
> proc_cleanup_work(). Can you please remove that as well?
Good catch. It isn't immediately obvious to me how that sentence
needs to be updated but it should be done.
Eric
>> -{
>> - struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_work);
>> - pid_ns_release_proc(ns);
>> -}
>> -
>> static struct ucounts *inc_pid_namespaces(struct user_namespace *ns)
>> {
>> return inc_ucount(ns, current_euid(), UCOUNT_PID_NAMESPACES);
>> @@ -114,7 +108,6 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>> ns->user_ns = get_user_ns(user_ns);
>> ns->ucounts = ucounts;
>> ns->pid_allocated = PIDNS_ADDING;
>> - INIT_WORK(&ns->proc_work, proc_cleanup_work);
>>
>> return ns;
>>
>> --
>> 2.25.0
>>
^ permalink raw reply
* Re: [PATCH 2/3] uml: Create a private mount of proc for mconsole
From: Christian Brauner @ 2020-02-28 21:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <87zhd2pfjd.fsf@x220.int.ebiederm.org>
On Fri, Feb 28, 2020 at 03:28:54PM -0600, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
>
> > On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
> >>
> >> The mconsole code only ever accesses proc for the initial pid
> >> namespace. Instead of depending upon the proc_mnt which is
> >> for proc_flush_task have uml create it's own mount of proc
> >> instead.
> >>
> >> This allows proc_flush_task to evolve and remove the
> >> need for having a proc_mnt to do it's job.
> >>
> >> Cc: Jeff Dike <jdike@addtoit.com>
> >> Cc: Richard Weinberger <richard@nod.at>
> >> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> >> ---
> >> arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
> >> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
> >> index e8f5c81c2c6c..30575bd92975 100644
> >> --- a/arch/um/drivers/mconsole_kern.c
> >> +++ b/arch/um/drivers/mconsole_kern.c
> >> @@ -36,6 +36,8 @@
> >> #include "mconsole_kern.h"
> >> #include <os.h>
> >>
> >> +static struct vfsmount *proc_mnt = NULL;
> >> +
> >> static int do_unlink_socket(struct notifier_block *notifier,
> >> unsigned long what, void *data)
> >> {
> >> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
> >>
> >> void mconsole_proc(struct mc_request *req)
> >> {
> >> - struct vfsmount *mnt = init_pid_ns.proc_mnt;
> >> + struct vfsmount *mnt = proc_mnt;
> >> char *buf;
> >> int len;
> >> struct file *file;
> >> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
> >> ptr += strlen("proc");
> >> ptr = skip_spaces(ptr);
> >>
> >> + if (!mnt) {
> >> + mconsole_reply(req, "Proc not available", 1, 0);
> >> + goto out;
> >> + }
> >> file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
> >> if (IS_ERR(file)) {
> >> mconsole_reply(req, "Failed to open file", 1, 0);
> >> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
> >> with_console(req, stack_proc, to);
> >> }
> >>
> >> +static int __init mount_proc(void)
> >> +{
> >> + struct file_system_type *proc_fs_type;
> >> + struct vfsmount *mnt;
> >> +
> >> + proc_fs_type = get_fs_type("proc");
> >> + if (!proc_fs_type)
> >> + return -ENODEV;
> >> +
> >> + mnt = kern_mount(proc_fs_type);
> >> + put_filesystem(proc_fs_type);
> >> + if (IS_ERR(mnt))
> >> + return PTR_ERR(mnt);
> >> +
> >> + proc_mnt = mnt;
> >> + return 0;
> >> +}
> >> +
> >> /*
> >> * Changed by mconsole_setup, which is __setup, and called before SMP is
> >> * active.
> >> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
> >> int err;
> >> char file[UNIX_PATH_MAX];
> >>
> >> + mount_proc();
> >
> > Hm, either check the return value or make the mount_proc() void?
> > Probably worth logging something but moving on without proc.
>
> I modified mconsole_proc (the only place that cares to see if
> it has a valid proc_mnt).
>
> So the code already does the moving on without mounting proc
> and continues to work.
Ok, but then make mount_proc()
static void __init mount_proc(void)
and not
static int __init mount_proc(void)
like you have now. That was what I was getting it. Unless there's
another reason for this.
Christian
^ permalink raw reply
* [PATCH 4/3] pid: Improve the comment about waiting in zap_pid_ns_processes
From: Eric W. Biederman @ 2020-02-28 22:34 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <878skmsbyy.fsf_-_@x220.int.ebiederm.org>
Oleg wrote a very informative comment, but with the removal of
proc_cleanup_work it is no longer accurate.
Rewrite the comment so that it only talks about the details
that are still relevant, and hopefully is a little clearer.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
kernel/pid_namespace.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 318fcc6ba301..01f8ba32cc0c 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -224,20 +224,27 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
} while (rc != -ECHILD);
/*
- * kernel_wait4() above can't reap the EXIT_DEAD children but we do not
- * really care, we could reparent them to the global init. We could
- * exit and reap ->child_reaper even if it is not the last thread in
- * this pid_ns, free_pid(pid_allocated == 0) calls proc_cleanup_work(),
- * pid_ns can not go away until proc_kill_sb() drops the reference.
+ * kernel_wait4() misses EXIT_DEAD children, and EXIT_ZOMBIE
+ * process whose parents processes are outside of the pid
+ * namespace. Such processes are created with setns()+fork().
*
- * But this ns can also have other tasks injected by setns()+fork().
- * Again, ignoring the user visible semantics we do not really need
- * to wait until they are all reaped, but they can be reparented to
- * us and thus we need to ensure that pid->child_reaper stays valid
- * until they all go away. See free_pid()->wake_up_process().
+ * If those EXIT_ZOMBIE processes are not reaped by their
+ * parents before their parents exit, they will be reparented
+ * to pid_ns->child_reaper. Thus pidns->child_reaper needs to
+ * stay valid until they all go away.
*
- * We rely on ignored SIGCHLD, an injected zombie must be autoreaped
- * if reparented.
+ * The code relies on the the pid_ns->child_reaper ignoring
+ * SIGCHILD to cause those EXIT_ZOMBIE processes to be
+ * autoreaped if reparented.
+ *
+ * Semantically it is also desirable to wait for EXIT_ZOMBIE
+ * processes before allowing the child_reaper to be reaped, as
+ * that gives the invariant that when the init process of a
+ * pid namespace is reaped all of the processes in the pid
+ * namespace are gone.
+ *
+ * Once all of the other tasks are gone from the pid_namespace
+ * free_pid() will awaken this task.
*/
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 4/3] pid: Improve the comment about waiting in zap_pid_ns_processes
From: Christian Brauner @ 2020-02-29 2:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, Al Viro, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Alexey Gladkov, Linus Torvalds, Jeff Dike,
Richard Weinberger, Anton Ivanov
In-Reply-To: <878skmpcib.fsf_-_@x220.int.ebiederm.org>
On Fri, Feb 28, 2020 at 04:34:20PM -0600, Eric W. Biederman wrote:
>
> Oleg wrote a very informative comment, but with the removal of
> proc_cleanup_work it is no longer accurate.
>
> Rewrite the comment so that it only talks about the details
> that are still relevant, and hopefully is a little clearer.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> kernel/pid_namespace.c | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 318fcc6ba301..01f8ba32cc0c 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -224,20 +224,27 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> } while (rc != -ECHILD);
>
> /*
> - * kernel_wait4() above can't reap the EXIT_DEAD children but we do not
> - * really care, we could reparent them to the global init. We could
> - * exit and reap ->child_reaper even if it is not the last thread in
> - * this pid_ns, free_pid(pid_allocated == 0) calls proc_cleanup_work(),
> - * pid_ns can not go away until proc_kill_sb() drops the reference.
> + * kernel_wait4() misses EXIT_DEAD children, and EXIT_ZOMBIE
> + * process whose parents processes are outside of the pid
> + * namespace. Such processes are created with setns()+fork().
> *
> - * But this ns can also have other tasks injected by setns()+fork().
> - * Again, ignoring the user visible semantics we do not really need
> - * to wait until they are all reaped, but they can be reparented to
> - * us and thus we need to ensure that pid->child_reaper stays valid
> - * until they all go away. See free_pid()->wake_up_process().
> + * If those EXIT_ZOMBIE processes are not reaped by their
> + * parents before their parents exit, they will be reparented
> + * to pid_ns->child_reaper. Thus pidns->child_reaper needs to
> + * stay valid until they all go away.
> *
> - * We rely on ignored SIGCHLD, an injected zombie must be autoreaped
> - * if reparented.
> + * The code relies on the the pid_ns->child_reaper ignoring
s/the the/the/
Hm, can we maybe reformulate this to:
"The code relies on having made pid_ns->child_reaper ignore SIGCHLD above
causing EXIT_ZOMBIE processes to be autoreaped if reparented."
Which imho makes it clearer that it was us ensuring that SIGCHLD is
ignored. Someone not too familiar with the exit codepaths might be
looking at zap_pid_ns_processes() not knowing that it is only called
when namespace init is exiting.
Otherwise
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [RFC PATCH v14 10/10] landlock: Add user and kernel documentation
From: Randy Dunlap @ 2020-02-29 17:23 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
Cc: Al Viro, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <20200224160215.4136-11-mic@digikod.net>
Hi,
Here are a few corrections for you to consider.
On 2/24/20 8:02 AM, Mickaël Salaün wrote:
> This documentation can be built with the Sphinx framework.
>
> Another location might be more appropriate, though.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Vincent Dagonneau <vincent.dagonneau@ssi.gouv.fr>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: James Morris <jmorris@namei.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> ---
>
> Changes since v13:
> * Rewrote the documentation according to the major revamp.
>
> Previous version:
> https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
> ---
> Documentation/security/index.rst | 1 +
> Documentation/security/landlock/index.rst | 18 ++
> Documentation/security/landlock/kernel.rst | 44 ++++
> Documentation/security/landlock/user.rst | 233 +++++++++++++++++++++
> 4 files changed, 296 insertions(+)
> create mode 100644 Documentation/security/landlock/index.rst
> create mode 100644 Documentation/security/landlock/kernel.rst
> create mode 100644 Documentation/security/landlock/user.rst
>
> diff --git a/Documentation/security/landlock/index.rst b/Documentation/security/landlock/index.rst
> new file mode 100644
> index 000000000000..dbd33b96ce60
> --- /dev/null
> +++ b/Documentation/security/landlock/index.rst
> @@ -0,0 +1,18 @@
> +=========================================
> +Landlock LSM: unprivileged access control
> +=========================================
> +
> +:Author: Mickaël Salaün
> +
> +The goal of Landlock is to enable to restrict ambient rights (e.g. global
> +filesystem access) for a set of processes. Because Landlock is a stackable
> +LSM, it makes possible to create safe security sandboxes as new security layers
> +in addition to the existing system-wide access-controls. This kind of sandbox
> +is expected to help mitigate the security impact of bugs or
> +unexpected/malicious behaviors in user-space applications. Landlock empower any
empowers
> +process, including unprivileged ones, to securely restrict themselves.
> +
> +.. toctree::
> +
> + user
> + kernel
> diff --git a/Documentation/security/landlock/kernel.rst b/Documentation/security/landlock/kernel.rst
> new file mode 100644
> index 000000000000..b87769909029
> --- /dev/null
> +++ b/Documentation/security/landlock/kernel.rst
> @@ -0,0 +1,44 @@
> +==============================
> +Landlock: kernel documentation
> +==============================
> +
> +Landlock's goal is to create scoped access-control (i.e. sandboxing). To
> +harden a whole system, this feature should be available to any process,
> +including unprivileged ones. Because such process may be compromised or
> +backdoored (i.e. untrusted), Landlock's features must be safe to use from the
> +kernel and other processes point of view. Landlock's interface must therefore
> +expose a minimal attack surface.
> +
> +Landlock is designed to be usable by unprivileged processes while following the
> +system security policy enforced by other access control mechanisms (e.g. DAC,
> +LSM). Indeed, a Landlock rule shall not interfere with other access-controls
> +enforced on the system, only add more restrictions.
> +
> +Any user can enforce Landlock rulesets on their processes. They are merged and
> +evaluated according to the inherited ones in a way that ensure that only more
ensures
> +constraints can be added.
> +
> +
> +Guiding principles for safe access controls
> +===========================================
> +
> +* A Landlock rule shall be focused on access control on kernel objects instead
> + of syscall filtering (i.e. syscall arguments), which is the purpose of
> + seccomp-bpf.
> +* To avoid multiple kind of side-channel attacks (e.g. leak of security
kinds
> + policies, CPU-based attacks), Landlock rules shall not be able to
> + programmatically communicate with user space.
> +* Kernel access check shall not slow down access request from unsandboxed
> + processes.
> +* Computation related to Landlock operations (e.g. enforce a ruleset) shall
> + only impact the processes requesting them.
> +
> +
> +Landlock rulesets and domains
> +=============================
> +
> +A domain is a read-only ruleset tied to a set of subjects (i.e. tasks). A
> +domain can transition to a new one which is the intersection of the constraints
> +from the current and a new ruleset. The definition of a subject is implicit
> +for a task sandboxing itself, which makes the reasoning much easier and helps
> +avoid pitfalls.
> diff --git a/Documentation/security/landlock/user.rst b/Documentation/security/landlock/user.rst
> new file mode 100644
> index 000000000000..cbd7f61fca8c
> --- /dev/null
> +++ b/Documentation/security/landlock/user.rst
> @@ -0,0 +1,233 @@
> +=================================
> +Landlock: userspace documentation
> +=================================
> +
> +Landlock rules
> +==============
> +
> +A Landlock rule enables to describe an action on an object. An object is
> +currently a file hierarchy, and the related filesystem actions are defined in
> +`Access rights`_. A set of rules are aggregated in a ruleset, which can then
is
> +restricts the thread enforcing it, and its future children.
restrict
> +
> +
> +Defining and enforcing a security policy
> +----------------------------------------
> +
> +Before defining a security policy, an application should first probe for the
> +features supported by the running kernel, which is important to be compatible
> +with older kernels. This can be done thanks to the `landlock` syscall (cf.
> +:ref:`syscall`).
> +
> +.. code-block:: c
> +
> + struct landlock_attr_features attr_features;
> +
> + if (landlock(LANDLOCK_CMD_GET_FEATURES, LANDLOCK_OPT_GET_FEATURES,
> + sizeof(attr_features), &attr_features)) {
> + perror("Failed to probe the Landlock supported features");
> + return 1;
> + }
> +
> +Then, we need to create the ruleset that will contains our rules. For this
contain
> +example, the ruleset will contains rules which only allow read actions, but
contain
> +write actions will be denied. The ruleset then needs to handle both of these
> +kind of actions. To have a backward compatibility, these actions should be
> +ANDed with the supported ones.
> +
> +.. code-block:: c
> +
> + int ruleset_fd;
> + struct landlock_attr_ruleset ruleset = {
> + .handled_access_fs =
> + LANDLOCK_ACCESS_FS_READ |
> + LANDLOCK_ACCESS_FS_READDIR |
> + LANDLOCK_ACCESS_FS_EXECUTE |
> + LANDLOCK_ACCESS_FS_WRITE |
> + LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_CHMOD |
> + LANDLOCK_ACCESS_FS_CHOWN |
> + LANDLOCK_ACCESS_FS_CHGRP |
> + LANDLOCK_ACCESS_FS_LINK_TO |
> + LANDLOCK_ACCESS_FS_RENAME_FROM |
> + LANDLOCK_ACCESS_FS_RENAME_TO |
> + LANDLOCK_ACCESS_FS_RMDIR |
> + LANDLOCK_ACCESS_FS_UNLINK |
> + LANDLOCK_ACCESS_FS_MAKE_CHAR |
> + LANDLOCK_ACCESS_FS_MAKE_DIR |
> + LANDLOCK_ACCESS_FS_MAKE_REG |
> + LANDLOCK_ACCESS_FS_MAKE_SOCK |
> + LANDLOCK_ACCESS_FS_MAKE_FIFO |
> + LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> + LANDLOCK_ACCESS_FS_MAKE_SYM,
> + };
> +
> + ruleset.handled_access_fs &= attr_features.access_fs;
> + ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> + LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> + if (ruleset_fd < 0) {
> + perror("Failed to create a ruleset");
> + return 1;
> + }
> +
> +We can now add a new rule to this ruleset thanks to the returned file
> +descriptor referring to this ruleset. The rule will only enable to read the
> +file hierarchy ``/usr``. Without other rule, write actions would then be
Without other rules,
or
Without another rule,
> +denied by the ruleset. To add ``/usr`` to the ruleset, we open it with the
> +``O_PATH`` flag and fill the &struct landlock_attr_path_beneath with this file
> +descriptor.
> +
> +.. code-block:: c
> +
> + int err;
> + struct landlock_attr_path_beneath path_beneath = {
> + .ruleset_fd = ruleset_fd,
> + .allowed_access =
> + LANDLOCK_ACCESS_FS_READ |
> + LANDLOCK_ACCESS_FS_READDIR |
> + LANDLOCK_ACCESS_FS_EXECUTE,
> + };
> +
> + path_beneath.allowed_access &= attr_features.access_fs;
> + path_beneath.parent_fd = open("/usr", O_PATH | O_CLOEXEC);
> + if (path_beneath.parent_fd < 0) {
> + perror("Failed to open file");
> + close(ruleset_fd);
> + return 1;
> + }
> + err = landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> + sizeof(path_beneath), &path_beneath);
> + close(path_beneath.parent_fd);
> + if (err) {
> + perror("Failed to update ruleset");
> + close(ruleset_fd);
> + return 1;
> + }
> +
> +We now have a ruleset with one rule allowing read access to ``/usr`` while
> +denying all accesses featured in ``attr_features.access_fs`` to everything else
> +on the filesystem. The next step is to restrict the current thread from
> +gaining more privileges (e.g. thanks to a SUID binary).
> +
> +.. code-block:: c
> +
> + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> + perror("Failed to restrict privileges");
> + close(ruleset_fd);
> + return 1;
> + }
> +
> +The current thread is now ready to sandbox itself with the ruleset.
> +
> +.. code-block:: c
> +
> + struct landlock_attr_enforce attr_enforce = {
> + .ruleset_fd = ruleset_fd,
> + };
> +
> + if (landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> + sizeof(attr_enforce), &attr_enforce)) {
> + perror("Failed to enforce ruleset");
> + close(ruleset_fd);
> + return 1;
> + }
> + close(ruleset_fd);
> +
> +If this last system call succeeds, the current thread is now restricted and
If this last landlock system call succeeds,
[because close() is the last system call]
> +this policy will be enforced on all its subsequently created children as well.
> +Once a thread is landlocked, there is no way to remove its security policy,
preferably: policy;
> +only adding more restrictions is allowed. These threads are now in a new
> +Landlock domain, merge of their parent one (if any) with the new ruleset.
> +
> +A full working code can be found in `samples/landlock/sandboxer.c`_.
Full working code
> +
> +
> +Inheritance
> +-----------
> +
> +Every new thread resulting from a :manpage:`clone(2)` inherits Landlock program
> +restrictions from its parent. This is similar to the seccomp inheritance (cf.
> +:doc:`/userspace-api/seccomp_filter`) or any other LSM dealing with task's
> +:manpage:`credentials(7)`. For instance, one process' thread may apply
process's
> +Landlock rules to itself, but they will not be automatically applied to other
> +sibling threads (unlike POSIX thread credential changes, cf.
> +:manpage:`nptl(7)`).
[snip]
thanks for the documentation.
--
~Randy
^ permalink raw reply
* Re: [Intel-gfx] [PATCH v7 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability
From: Serge Hallyn @ 2020-03-02 0:19 UTC (permalink / raw)
To: Alexey Budankov
Cc: James Morris, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner, linux-man,
Andi Kleen, linux-parisc@vger.kernel.org,
linux-doc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list, Jiri Olsa,
linux-arm-kernel
In-Reply-To: <3ae0bed5-204e-de81-7647-5f0d8106cd67@linux.intel.com>
Thanks, this looks good to me, in keeping with the CAP_SYSLOG break.
Acked-by: Serge E. Hallyn <serge@hallyn.com>
for the set.
James/Ingo/Peter, if noone has remaining objections, whose branch
should these go in through?
thanks,
-serge
On Tue, Feb 25, 2020 at 12:55:54PM +0300, Alexey Budankov wrote:
>
> Hi,
>
> Is there anything else I could do in order to move the changes forward
> or is something still missing from this patch set?
> Could you please share you mind?
>
> Thanks,
> Alexey
>
> On 17.02.2020 11:02, Alexey Budankov wrote:
> >
> > Currently access to perf_events, i915_perf and other performance
> > monitoring and observability subsystems of the kernel is open only for
> > a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
> > process effective set [2].
> >
> > This patch set introduces CAP_PERFMON capability designed to secure
> > system performance monitoring and observability operations so that
> > CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
> > for performance monitoring and observability subsystems of the kernel.
> >
> > CAP_PERFMON intends to harden system security and integrity during
> > performance monitoring and observability operations by decreasing attack
> > surface that is available to a CAP_SYS_ADMIN privileged process [2].
> > Providing the access to performance monitoring and observability
> > operations under CAP_PERFMON capability singly, without the rest of
> > CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials
> > and makes the operation more secure. Thus, CAP_PERFMON implements the
> > principal of least privilege for performance monitoring and
> > observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle of
> > least privilege: A security design principle that states that a process
> > or program be granted only those privileges (e.g., capabilities)
> > necessary to accomplish its legitimate function, and only for the time
> > that such privileges are actually required)
> >
> > CAP_PERFMON intends to meet the demand to secure system performance
> > monitoring and observability operations for adoption in security
> > sensitive, restricted, multiuser production environments (e.g. HPC
> > clusters, cloud and virtual compute environments), where root or
> > CAP_SYS_ADMIN credentials are not available to mass users of a system,
> > and securely unblock accessibility of system performance monitoring and
> > observability operations beyond root and CAP_SYS_ADMIN use cases.
> >
> > CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
> > system performance monitoring and observability operations and balance
> > amount of CAP_SYS_ADMIN credentials following the recommendations in
> > the capabilities man page [2] for CAP_SYS_ADMIN: "Note: this capability
> > is overloaded; see Notes to kernel developers, below." For backward
> > compatibility reasons access to system performance monitoring and
> > observability subsystems of the kernel remains open for CAP_SYS_ADMIN
> > privileged processes but CAP_SYS_ADMIN capability usage for secure
> > system performance monitoring and observability operations is
> > discouraged with respect to the designed CAP_PERFMON capability.
> >
> > Possible alternative solution to this system security hardening,
> > capabilities balancing task of making performance monitoring and
> > observability operations more secure and accessible could be to use
> > the existing CAP_SYS_PTRACE capability to govern system performance
> > monitoring and observability subsystems. However CAP_SYS_PTRACE
> > capability still provides users with more credentials than are
> > required for secure performance monitoring and observability
> > operations and this excess is avoided by the designed CAP_PERFMON.
> >
> > Although software running under CAP_PERFMON can not ensure avoidance of
> > related hardware issues, the software can still mitigate those issues
> > following the official hardware issues mitigation procedure [3]. The
> > bugs in the software itself can be fixed following the standard kernel
> > development process [4] to maintain and harden security of system
> > performance monitoring and observability operations. Finally, the patch
> > set is shaped in the way that simplifies backtracking procedure of
> > possible induced issues [5] as much as possible.
> >
> > The patch set is for tip perf/core repository:
> > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
> > sha1: fdb64822443ec9fb8c3a74b598a74790ae8d2e22
> >
> > ---
> > Changes in v7:
> > - updated and extended kernel.rst and perf-security.rst documentation
> > files with the information about CAP_PERFMON capability and its use cases
> > - documented the case of double audit logging of CAP_PERFMON and CAP_SYS_ADMIN
> > capabilities on a SELinux enabled system
> > Changes in v6:
> > - avoided noaudit checks in perfmon_capable() to explicitly advertise
> > CAP_PERFMON usage thru audit logs to secure system performance
> > monitoring and observability
> > Changes in v5:
> > - renamed CAP_SYS_PERFMON to CAP_PERFMON
> > - extended perfmon_capable() with noaudit checks
> > Changes in v4:
> > - converted perfmon_capable() into an inline function
> > - made perf_events kprobes, uprobes, hw breakpoints and namespaces data
> > available to CAP_SYS_PERFMON privileged processes
> > - applied perfmon_capable() to drivers/perf and drivers/oprofile
> > - extended __cmd_ftrace() with support of CAP_SYS_PERFMON
> > Changes in v3:
> > - implemented perfmon_capable() macros aggregating required capabilities
> > checks
> > Changes in v2:
> > - made perf_events trace points available to CAP_SYS_PERFMON privileged
> > processes
> > - made perf_event_paranoid_check() treat CAP_SYS_PERFMON equally to
> > CAP_SYS_ADMIN
> > - applied CAP_SYS_PERFMON to i915_perf, bpf_trace, powerpc and parisc
> > system performance monitoring and observability related subsystems
> >
> > ---
> > Alexey Budankov (12):
> > capabilities: introduce CAP_PERFMON to kernel and user space
> > perf/core: open access to the core for CAP_PERFMON privileged process
> > perf/core: open access to probes for CAP_PERFMON privileged process
> > perf tool: extend Perf tool with CAP_PERFMON capability support
> > drm/i915/perf: open access for CAP_PERFMON privileged process
> > trace/bpf_trace: open access for CAP_PERFMON privileged process
> > powerpc/perf: open access for CAP_PERFMON privileged process
> > parisc/perf: open access for CAP_PERFMON privileged process
> > drivers/perf: open access for CAP_PERFMON privileged process
> > drivers/oprofile: open access for CAP_PERFMON privileged process
> > doc/admin-guide: update perf-security.rst with CAP_PERFMON information
> > doc/admin-guide: update kernel.rst with CAP_PERFMON information
> >
> > Documentation/admin-guide/perf-security.rst | 65 +++++++++++++--------
> > Documentation/admin-guide/sysctl/kernel.rst | 16 +++--
> > arch/parisc/kernel/perf.c | 2 +-
> > arch/powerpc/perf/imc-pmu.c | 4 +-
> > drivers/gpu/drm/i915/i915_perf.c | 13 ++---
> > drivers/oprofile/event_buffer.c | 2 +-
> > drivers/perf/arm_spe_pmu.c | 4 +-
> > include/linux/capability.h | 4 ++
> > include/linux/perf_event.h | 6 +-
> > include/uapi/linux/capability.h | 8 ++-
> > kernel/events/core.c | 6 +-
> > kernel/trace/bpf_trace.c | 2 +-
> > security/selinux/include/classmap.h | 4 +-
> > tools/perf/builtin-ftrace.c | 5 +-
> > tools/perf/design.txt | 3 +-
> > tools/perf/util/cap.h | 4 ++
> > tools/perf/util/evsel.c | 10 ++--
> > tools/perf/util/util.c | 1 +
> > 18 files changed, 98 insertions(+), 61 deletions(-)
> >
> > ---
> > Validation (Intel Skylake, 8 cores, Fedora 29, 5.5.0-rc3+, x86_64):
> >
> > libcap library [6], [7], [8] and Perf tool can be used to apply
> > CAP_PERFMON capability for secure system performance monitoring and
> > observability beyond the scope permitted by the system wide
> > perf_event_paranoid kernel setting [9] and below are the steps for
> > evaluation:
> >
> > - patch, build and boot the kernel
> > - patch, build Perf tool e.g. to /home/user/perf
> > ...
> > # git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
> > # pushd libcap
> > # patch libcap/include/uapi/linux/capabilities.h with [PATCH 1]
> > # make
> > # pushd progs
> > # ./setcap "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > # ./setcap -v "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > /home/user/perf: OK
> > # ./getcap /home/user/perf
> > /home/user/perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
> > # echo 2 > /proc/sys/kernel/perf_event_paranoid
> > # cat /proc/sys/kernel/perf_event_paranoid
> > 2
> > ...
> > $ /home/user/perf top
> > ... works as expected ...
> > $ cat /proc/`pidof perf`/status
> > Name: perf
> > Umask: 0002
> > State: S (sleeping)
> > Tgid: 2958
> > Ngid: 0
> > Pid: 2958
> > PPid: 9847
> > TracerPid: 0
> > Uid: 500 500 500 500
> > Gid: 500 500 500 500
> > FDSize: 256
> > ...
> > CapInh: 0000000000000000
> > CapPrm: 0000004400080000
> > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > cap_perfmon,cap_sys_ptrace,cap_syslog
> > CapBnd: 0000007fffffffff
> > CapAmb: 0000000000000000
> > NoNewPrivs: 0
> > Seccomp: 0
> > Speculation_Store_Bypass: thread vulnerable
> > Cpus_allowed: ff
> > Cpus_allowed_list: 0-7
> > ...
> >
> > Usage of cap_perfmon effectively avoids unused credentials excess:
> >
> > - with cap_sys_admin:
> > CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
> >
> > - with cap_perfmon:
> > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > 38 34 19
> > perfmon syslog sys_ptrace
> >
> > ---
> > [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> > [2] http://man7.org/linux/man-pages/man7/capabilities.7.html
> > [3] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
> > [4] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
> > [5] https://www.kernel.org/doc/html/latest/process/management-style.html#decisions
> > [6] http://man7.org/linux/man-pages/man8/setcap.8.html
> > [7] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
> > [8] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
> > [9] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: kernel BUG at lib/assoc_array.c:LINE!
From: David Howells @ 2020-03-02 8:28 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: dhowells, jmorris, keyrings, linux-security-module, serge
In-Reply-To: <20200218211259.GA19673@linux.intel.com>
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> The arguments for request_key_and_link() are fairly constrained:
>
> type == &key_type_dns_resolver
> description == "afsdb:<cell name>"
> domain_tag == net->key_domain
> callout_info == "srv=1"
> callout_len == 5
> aux == NULL
> dest_keyring == NULL
> flags == KEY_ALLOC_IN_QUOTA
>
> (manually resolved)
>
> The only obvious moving part I see is the key type implementatio i.e.
> net/dns_resolver/dns_key.c.
It shouldn't matter what the payload of the key is going to be, but it might
matter what the cell name is. Looking in the log, I see this:
mount(&(0x7f0000000000)=ANY=[@ANYBLOB="25dc545df1e34ab2e26e2f5034c85b3a"], &(0x7f0000000140)='./file0\x00', &(0x7f0000000180)='afs\x00', 0x0, 0x0)
The:
@ANYBLOB="25dc545df1e34ab2e26e2f5034c85b3a"
is the source, i.e.:
%\xdcT]\xf1\xe3J\xb2\xe2n/P4\xc8[':
This is in the form "[%#][<cellname>:]<volumename>", which comports with the
log:
[ 621.627412][ T6728] kAFS: unable to lookup cell '?T]??J??n/P4?['
This is a bit odd, since the version allegedly being tested includes a patch
to prohibit AFS cell names that contain unprintable chars. It should error
out in afs_alloc_cell(), way before it tries to do a DNS lookup.
David
^ permalink raw reply
* Re: kernel BUG at lib/assoc_array.c:LINE!
From: David Howells @ 2020-03-02 9:27 UTC (permalink / raw)
Cc: dhowells, Jarkko Sakkinen, jmorris, keyrings,
linux-security-module, serge
In-Reply-To: <538780.1583137732@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> wrote:
> [ 621.627412][ T6728] kAFS: unable to lookup cell '?T]??J??n/P4?['
>
> This is a bit odd, since the version allegedly being tested includes a patch
> to prohibit AFS cell names that contain unprintable chars. It should error
> out in afs_alloc_cell(), way before it tries to do a DNS lookup.
Ah, no, that's okay. This error message is printed in afs_parse_source() on
the way out after afs_lookup_cell() fails (presumably due to afs_alloc_cell()
failing).
It seems unlikely that the dead process is actually anything to do with the
mount that more or less immediately preceded it in the log. It mount should
have failed without creating a cell record - which then precludes affecting
afs_manage_cell(). However, a previously created cell with a valid name can
feasibly be driving afs_manage_cell() and causing the problem.
David
^ permalink raw reply
* RE: [ima] 9165b814d2: BUG:kernel_NULL_pointer_dereference,address
From: Roberto Sassu @ 2020-03-02 9:46 UTC (permalink / raw)
To: kernel test robot
Cc: zohar@linux.ibm.com, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu, lkp@lists.01.org
In-Reply-To: <20200302012218.GU6548@shao2-debian>
> -----Original Message-----
> From: kernel test robot [mailto:rong.a.chen@intel.com]
> Sent: Monday, March 2, 2020 2:22 AM
> To: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: zohar@linux.ibm.com; James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; linux-integrity@vger.kernel.org; linux-
> security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu
> Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu
> <roberto.sassu@huawei.com>; lkp@lists.01.org
> Subject: [ima] 9165b814d2:
> BUG:kernel_NULL_pointer_dereference,address
>
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: 9165b814d2bea8cfeb557505bb206396331e8192 ("[PATCH v2 8/8]
> ima: Use ima_hash_algo for collision detection in the measurement list")
> url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-support-
> stronger-algorithms-for-attestation/20200205-233901
> base: https://git.kernel.org/cgit/linux/kernel/git/zohar/linux-integrity.git
> next-integrity
Hi
thanks for the report.
Yes, version 2 had a bug:
---
ima_algo_array[i++].algo = HASH_ALGO_SHA1;
}
if (ima_hash_algo_idx >= nr_allocated_banks) {
ima_algo_array[i].tfm = ima_shash_tfm;
ima_algo_array[i].algo = ima_hash_algo;
}
---
The code allocated ima_algo_array with size 1 (TPM was not found and
the default algorithm is SHA1).
However, later it initializes ima_algo_array for SHA1 and increments the
i variable. Since the code does not check if the default algorithm is SHA1,
the last part is also executed and causes corruption, because ima_algo_array
has only one element.
I fixed already this bug in version 3 of the patch set.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [RFC PATCH v14 10/10] landlock: Add user and kernel documentation
From: Mickaël Salaün @ 2020-03-02 10:03 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: Al Viro, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <cc8da381-d3dc-3c0a-5afd-96824362b636@infradead.org>
On 29/02/2020 18:23, Randy Dunlap wrote:
> Hi,
> Here are a few corrections for you to consider.
>
>
> On 2/24/20 8:02 AM, Mickaël Salaün wrote:
>> This documentation can be built with the Sphinx framework.
>>
>> Another location might be more appropriate, though.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Reviewed-by: Vincent Dagonneau <vincent.dagonneau@ssi.gouv.fr>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> ---
>>
>> Changes since v13:
>> * Rewrote the documentation according to the major revamp.
>>
>> Previous version:
>> https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
>> ---
>> Documentation/security/index.rst | 1 +
>> Documentation/security/landlock/index.rst | 18 ++
>> Documentation/security/landlock/kernel.rst | 44 ++++
>> Documentation/security/landlock/user.rst | 233 +++++++++++++++++++++
>> 4 files changed, 296 insertions(+)
>> create mode 100644 Documentation/security/landlock/index.rst
>> create mode 100644 Documentation/security/landlock/kernel.rst
>> create mode 100644 Documentation/security/landlock/user.rst
>>
>> diff --git a/Documentation/security/landlock/index.rst b/Documentation/security/landlock/index.rst
>> new file mode 100644
>> index 000000000000..dbd33b96ce60
>> --- /dev/null
>> +++ b/Documentation/security/landlock/index.rst
>> @@ -0,0 +1,18 @@
>> +=========================================
>> +Landlock LSM: unprivileged access control
>> +=========================================
>> +
>> +:Author: Mickaël Salaün
>> +
>> +The goal of Landlock is to enable to restrict ambient rights (e.g. global
>> +filesystem access) for a set of processes. Because Landlock is a stackable
>> +LSM, it makes possible to create safe security sandboxes as new security layers
>> +in addition to the existing system-wide access-controls. This kind of sandbox
>> +is expected to help mitigate the security impact of bugs or
>> +unexpected/malicious behaviors in user-space applications. Landlock empower any
>
> empowers
>
>> +process, including unprivileged ones, to securely restrict themselves.
>> +
>> +.. toctree::
>> +
>> + user
>> + kernel
>> diff --git a/Documentation/security/landlock/kernel.rst b/Documentation/security/landlock/kernel.rst
>> new file mode 100644
>> index 000000000000..b87769909029
>> --- /dev/null
>> +++ b/Documentation/security/landlock/kernel.rst
>> @@ -0,0 +1,44 @@
>> +==============================
>> +Landlock: kernel documentation
>> +==============================
>> +
>> +Landlock's goal is to create scoped access-control (i.e. sandboxing). To
>> +harden a whole system, this feature should be available to any process,
>> +including unprivileged ones. Because such process may be compromised or
>> +backdoored (i.e. untrusted), Landlock's features must be safe to use from the
>> +kernel and other processes point of view. Landlock's interface must therefore
>> +expose a minimal attack surface.
>> +
>> +Landlock is designed to be usable by unprivileged processes while following the
>> +system security policy enforced by other access control mechanisms (e.g. DAC,
>> +LSM). Indeed, a Landlock rule shall not interfere with other access-controls
>> +enforced on the system, only add more restrictions.
>> +
>> +Any user can enforce Landlock rulesets on their processes. They are merged and
>> +evaluated according to the inherited ones in a way that ensure that only more
>
> ensures
>
>> +constraints can be added.
>> +
>> +
>> +Guiding principles for safe access controls
>> +===========================================
>> +
>> +* A Landlock rule shall be focused on access control on kernel objects instead
>> + of syscall filtering (i.e. syscall arguments), which is the purpose of
>> + seccomp-bpf.
>> +* To avoid multiple kind of side-channel attacks (e.g. leak of security
>
> kinds
>
>> + policies, CPU-based attacks), Landlock rules shall not be able to
>> + programmatically communicate with user space.
>> +* Kernel access check shall not slow down access request from unsandboxed
>> + processes.
>> +* Computation related to Landlock operations (e.g. enforce a ruleset) shall
>> + only impact the processes requesting them.
>> +
>> +
>> +Landlock rulesets and domains
>> +=============================
>> +
>> +A domain is a read-only ruleset tied to a set of subjects (i.e. tasks). A
>> +domain can transition to a new one which is the intersection of the constraints
>> +from the current and a new ruleset. The definition of a subject is implicit
>> +for a task sandboxing itself, which makes the reasoning much easier and helps
>> +avoid pitfalls.
>> diff --git a/Documentation/security/landlock/user.rst b/Documentation/security/landlock/user.rst
>> new file mode 100644
>> index 000000000000..cbd7f61fca8c
>> --- /dev/null
>> +++ b/Documentation/security/landlock/user.rst
>> @@ -0,0 +1,233 @@
>> +=================================
>> +Landlock: userspace documentation
>> +=================================
>> +
>> +Landlock rules
>> +==============
>> +
>> +A Landlock rule enables to describe an action on an object. An object is
>> +currently a file hierarchy, and the related filesystem actions are defined in
>> +`Access rights`_. A set of rules are aggregated in a ruleset, which can then
>
> is
>
>> +restricts the thread enforcing it, and its future children.
>
> restrict
>
>> +
>> +
>> +Defining and enforcing a security policy
>> +----------------------------------------
>> +
>> +Before defining a security policy, an application should first probe for the
>> +features supported by the running kernel, which is important to be compatible
>> +with older kernels. This can be done thanks to the `landlock` syscall (cf.
>> +:ref:`syscall`).
>> +
>> +.. code-block:: c
>> +
>> + struct landlock_attr_features attr_features;
>> +
>> + if (landlock(LANDLOCK_CMD_GET_FEATURES, LANDLOCK_OPT_GET_FEATURES,
>> + sizeof(attr_features), &attr_features)) {
>> + perror("Failed to probe the Landlock supported features");
>> + return 1;
>> + }
>> +
>> +Then, we need to create the ruleset that will contains our rules. For this
>
> contain
>
>> +example, the ruleset will contains rules which only allow read actions, but
>
> contain
>
>> +write actions will be denied. The ruleset then needs to handle both of these
>> +kind of actions. To have a backward compatibility, these actions should be
>> +ANDed with the supported ones.
>> +
>> +.. code-block:: c
>> +
>> + int ruleset_fd;
>> + struct landlock_attr_ruleset ruleset = {
>> + .handled_access_fs =
>> + LANDLOCK_ACCESS_FS_READ |
>> + LANDLOCK_ACCESS_FS_READDIR |
>> + LANDLOCK_ACCESS_FS_EXECUTE |
>> + LANDLOCK_ACCESS_FS_WRITE |
>> + LANDLOCK_ACCESS_FS_TRUNCATE |
>> + LANDLOCK_ACCESS_FS_CHMOD |
>> + LANDLOCK_ACCESS_FS_CHOWN |
>> + LANDLOCK_ACCESS_FS_CHGRP |
>> + LANDLOCK_ACCESS_FS_LINK_TO |
>> + LANDLOCK_ACCESS_FS_RENAME_FROM |
>> + LANDLOCK_ACCESS_FS_RENAME_TO |
>> + LANDLOCK_ACCESS_FS_RMDIR |
>> + LANDLOCK_ACCESS_FS_UNLINK |
>> + LANDLOCK_ACCESS_FS_MAKE_CHAR |
>> + LANDLOCK_ACCESS_FS_MAKE_DIR |
>> + LANDLOCK_ACCESS_FS_MAKE_REG |
>> + LANDLOCK_ACCESS_FS_MAKE_SOCK |
>> + LANDLOCK_ACCESS_FS_MAKE_FIFO |
>> + LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>> + LANDLOCK_ACCESS_FS_MAKE_SYM,
>> + };
>> +
>> + ruleset.handled_access_fs &= attr_features.access_fs;
>> + ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
>> + LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
>> + if (ruleset_fd < 0) {
>> + perror("Failed to create a ruleset");
>> + return 1;
>> + }
>> +
>> +We can now add a new rule to this ruleset thanks to the returned file
>> +descriptor referring to this ruleset. The rule will only enable to read the
>> +file hierarchy ``/usr``. Without other rule, write actions would then be
>
> Without other rules,
> or
> Without another rule,
>
>> +denied by the ruleset. To add ``/usr`` to the ruleset, we open it with the
>> +``O_PATH`` flag and fill the &struct landlock_attr_path_beneath with this file
>> +descriptor.
>> +
>> +.. code-block:: c
>> +
>> + int err;
>> + struct landlock_attr_path_beneath path_beneath = {
>> + .ruleset_fd = ruleset_fd,
>> + .allowed_access =
>> + LANDLOCK_ACCESS_FS_READ |
>> + LANDLOCK_ACCESS_FS_READDIR |
>> + LANDLOCK_ACCESS_FS_EXECUTE,
>> + };
>> +
>> + path_beneath.allowed_access &= attr_features.access_fs;
>> + path_beneath.parent_fd = open("/usr", O_PATH | O_CLOEXEC);
>> + if (path_beneath.parent_fd < 0) {
>> + perror("Failed to open file");
>> + close(ruleset_fd);
>> + return 1;
>> + }
>> + err = landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
>> + sizeof(path_beneath), &path_beneath);
>> + close(path_beneath.parent_fd);
>> + if (err) {
>> + perror("Failed to update ruleset");
>> + close(ruleset_fd);
>> + return 1;
>> + }
>> +
>> +We now have a ruleset with one rule allowing read access to ``/usr`` while
>> +denying all accesses featured in ``attr_features.access_fs`` to everything else
>> +on the filesystem. The next step is to restrict the current thread from
>> +gaining more privileges (e.g. thanks to a SUID binary).
>> +
>> +.. code-block:: c
>> +
>> + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
>> + perror("Failed to restrict privileges");
>> + close(ruleset_fd);
>> + return 1;
>> + }
>> +
>> +The current thread is now ready to sandbox itself with the ruleset.
>> +
>> +.. code-block:: c
>> +
>> + struct landlock_attr_enforce attr_enforce = {
>> + .ruleset_fd = ruleset_fd,
>> + };
>> +
>> + if (landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
>> + sizeof(attr_enforce), &attr_enforce)) {
>> + perror("Failed to enforce ruleset");
>> + close(ruleset_fd);
>> + return 1;
>> + }
>> + close(ruleset_fd);
>> +
>> +If this last system call succeeds, the current thread is now restricted and
>
> If this last landlock system call succeeds,
>
> [because close() is the last system call]
>
>> +this policy will be enforced on all its subsequently created children as well.
>> +Once a thread is landlocked, there is no way to remove its security policy,
>
> preferably: policy;
>
>> +only adding more restrictions is allowed. These threads are now in a new
>> +Landlock domain, merge of their parent one (if any) with the new ruleset.
>> +
>> +A full working code can be found in `samples/landlock/sandboxer.c`_.
>
> Full working code
>
>> +
>> +
>> +Inheritance
>> +-----------
>> +
>> +Every new thread resulting from a :manpage:`clone(2)` inherits Landlock program
>> +restrictions from its parent. This is similar to the seccomp inheritance (cf.
>> +:doc:`/userspace-api/seccomp_filter`) or any other LSM dealing with task's
>> +:manpage:`credentials(7)`. For instance, one process' thread may apply
>
> process's
>
>> +Landlock rules to itself, but they will not be automatically applied to other
>> +sibling threads (unlike POSIX thread credential changes, cf.
>> +:manpage:`nptl(7)`).
>
> [snip]
>
> thanks for the documentation.
>
Done. Thanks for this attentive review!
^ permalink raw reply
* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-03-02 13:41 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <6955307747034265bd282bf68c368f34@huawei.com>
On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote:
> > -----Original Message-----
Please find/use a mailer that doesn't include this junk.
> > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > boot_aggregate is the first entry of IMA measurement list. Its purpose is
> > > to link pre-boot measurements to IMA measurements. As IMA was
> > designed to
> > > work with a TPM 1.2, the SHA1 PCR bank was always selected.
> > >
> > > Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> > > However, the assumption that the SHA1 PCR bank is always available is not
> > > correct, as PCR banks can be selected with the PCR_Allocate() TPM
> > command.
> > >
> > > This patch tries to use ima_hash_algo as hash algorithm for
> > boot_aggregate.
> > > If no PCR bank uses that algorithm, the patch tries to find the SHA256 PCR
> > > bank (which is mandatory in the TCG PC Client specification).
> >
> > Up to here, the patch description matches the code.
> > > If also this
> > > bank is not found, the patch selects the first one. If the TPM algorithm
> > > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> >
> > This comment and the one inline are left over from previous version.
>
> Hi Mimi
>
> actually the code does what is described above. bank_idx is initially
> set to zero and remains as it is if there is no PCR bank for the default
> IMA algorithm or SHA256.
Sorry for the delay in continuing to review this patch set. It took a
while to write ima-evm-utils regression tests for it.
Dmitry and you were the ones that initiated ima-evm-utils, saying
there should a single package for signing files and integrity testing.
The features in ima-evm-utils should reflect what is actually
upstreamed in the kernel. (Currently there are a few experimental
features which were never upstreamed. I'd like to remove them, but am
a bit concerned that they are being used.) I'd appreciate your help
in keeping ima-evm-utils up to date. It will help simplify
upstreaming new kernel features.
My initial patch attempted to use any common TPM and kernel hash
algorithm to calculate the boot_aggregate. The discussion with James
was pretty clear, which you even stated in the Changelog. Either we
use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
1.2 for the boot-aggregate.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-03-02 14:28 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1583156506.8544.60.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 2:42 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
> On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
>
> Please find/use a mailer that doesn't include this junk.
I will do. I didn't have the time yet.
> > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > > boot_aggregate is the first entry of IMA measurement list. Its purpose
> is
> > > > to link pre-boot measurements to IMA measurements. As IMA was
> > > designed to
> > > > work with a TPM 1.2, the SHA1 PCR bank was always selected.
> > > >
> > > > Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> > > > However, the assumption that the SHA1 PCR bank is always available is
> not
> > > > correct, as PCR banks can be selected with the PCR_Allocate() TPM
> > > command.
> > > >
> > > > This patch tries to use ima_hash_algo as hash algorithm for
> > > boot_aggregate.
> > > > If no PCR bank uses that algorithm, the patch tries to find the SHA256
> PCR
> > > > bank (which is mandatory in the TCG PC Client specification).
> > >
> > > Up to here, the patch description matches the code.
> > > > If also this
> > > > bank is not found, the patch selects the first one. If the TPM algorithm
> > > > of that bank is not mapped to a crypto ID, boot_aggregate is set to
> zero.
> > >
> > > This comment and the one inline are left over from previous version.
> >
> > Hi Mimi
> >
> > actually the code does what is described above. bank_idx is initially
> > set to zero and remains as it is if there is no PCR bank for the default
> > IMA algorithm or SHA256.
>
> Sorry for the delay in continuing to review this patch set. It took a
> while to write ima-evm-utils regression tests for it.
>
> Dmitry and you were the ones that initiated ima-evm-utils, saying
> there should a single package for signing files and integrity testing.
> The features in ima-evm-utils should reflect what is actually
> upstreamed in the kernel. (Currently there are a few experimental
> features which were never upstreamed. I'd like to remove them, but am
> a bit concerned that they are being used.) I'd appreciate your help
> in keeping ima-evm-utils up to date. It will help simplify
> upstreaming new kernel features.
>
> My initial patch attempted to use any common TPM and kernel hash
> algorithm to calculate the boot_aggregate. The discussion with James
> was pretty clear, which you even stated in the Changelog. Either we
> use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
> 1.2 for the boot-aggregate.
Ok, I didn't understand fully. I thought we should use the default IMA
algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
PCR bank for default algorithm. I additionally implemented the logic to
select the first PCR bank if the SHA256 PCR bank is not available but I can
remove it.
SHA256 should be the minimum requirement for boot aggregate. The
advantage of using the default IMA algorithm is that it will be possible to
select stronger algorithms when they are supported by the TPM. We might
introduce a new option to specify only the algorithm for boot aggregate,
like James suggested to support embedded systems. Let me know which
option you prefer.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Serge E. Hallyn @ 2020-03-02 14:34 UTC (permalink / raw)
To: Josef Bacik
Cc: Christian Brauner, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn, smbarber, Seth Forshee, Alexander Viro,
Alexey Dobriyan, Serge Hallyn, James Morris, Kees Cook,
Jonathan Corbet, Phil Estes, linux-kernel, linux-fsdevel,
containers, linux-security-module, linux-api, mpawlowski
In-Reply-To: <2b0fe94b-036a-919e-219b-cc1ba0641781@toxicpanda.com>
On Thu, Feb 27, 2020 at 02:33:04PM -0500, Josef Bacik wrote:
> On 2/18/20 9:33 AM, Christian Brauner wrote:
> > Hey everyone,
> >
> > This is v3 after (off- and online) discussions with Jann the following
> > changes were made:
> > - To handle nested user namespaces cleanly, efficiently, and with full
> > backwards compatibility for non fsid-mapping aware workloads we only
> > allow writing fsid mappings as long as the corresponding id mapping
> > type has not been written.
> > - Split the patch which adds the internal ability in
> > kernel/user_namespace to verify and write fsid mappings into tree
> > patches:
> > 1. [PATCH v3 04/25] fsuidgid: add fsid mapping helpers
> > patch to implement core helpers for fsid translations (i.e.
> > make_kfs*id(), from_kfs*id{_munged}(), kfs*id_to_k*id(),
> > k*id_to_kfs*id()
> > 2. [PATCH v3 05/25] user_namespace: refactor map_write()
> > patch to refactor map_write() in order to prepare for actual fsid
> > mappings changes in the following patch. (This should make it
> > easier to review.)
> > 3. [PATCH v3 06/25] user_namespace: make map_write() support fsid mappings
> > patch to implement actual fsid mappings support in mape_write()
> > - Let the keyctl infrastructure only operate on kfsid which are always
> > mapped/looked up in the id mappings similar to what we do for
> > filesystems that have the same superblock visible in multiple user
> > namespaces.
> >
> > This version also comes with minimal tests which I intend to expand in
> > the future.
> >
> > From pings and off-list questions and discussions at Google Container
> > Security Summit there seems to be quite a lot of interest in this
> > patchset with use-cases ranging from layer sharing for app containers
> > and k8s, as well as data sharing between containers with different id
> > mappings. I haven't Cced all people because I don't have all the email
> > adresses at hand but I've at least added Phil now. :)
> >
> I put this into a kernel for our container guys to mess with in order to
> validate it would actually be useful for real world uses. I've cc'ed the
> guy who did all of the work in case you have specific questions.
>
> Good news is the interface is acceptable, albeit apparently the whole user
> ns interface sucks in general. But you haven't made it worse, so success!
Well I very much disagree here :) With the first part! But I do
understand the shortcomings. Anyway,
I still hope we get to talk about this in person, but IMO this is the
right approach (this being - thinking about how to make the uid mappings
more flexible without making them too complicated to be safe to use),
but a bit too static in terms of target. There are at least two ways
that I could see usefully generalizing it
From a user space pov, the following goal is indespensible (for my use
cases): that the fsuid be selectable based on fs, mountpoint, or file
context (as in selinux).
From a userns pov, one way to look at it is this: when task t1 signals
task t2, it's not only t1's namespace that's considered when filling in
the sender uid, but also t2's. Likewise, when writing a file, we should
consider both t1's fsuid+userns, and the file's, mount's, or filesystem's
userns.
From that POV, your patch is a step in the right direction and could be
taken as is (modulo any tmpfs fix Josef needs :) From there I would
propose adding a 'userns=<uidnsfd>' bind mount option, so we could create
an empty userns with the desired mapping (subject to permissions granted
by subuids), get an fd to the uidns, and say
mount --bind -o uidns=5 /shared /containers/c1/mnt/shared
So now when I write a file /etc/hosts as container fsuid 0, it'll be
subject to the container rootfs mount's uid mapping, presumably
100000. When I write /mnt/shared/hello, it'll be subject to the mount's
uid mapping, which might be 1000.
-serge
^ permalink raw reply
* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-03-02 14:46 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <8a6fb34e18b147fa811e82c78fb30d66@huawei.com>
> > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > My initial patch attempted to use any common TPM and kernel hash
> > algorithm to calculate the boot_aggregate. The discussion with James
> > was pretty clear, which you even stated in the Changelog. Either we
> > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
> > 1.2 for the boot-aggregate.
>
> Ok, I didn't understand fully. I thought we should use the default IMA
> algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
> PCR bank for default algorithm.
Yes, preference is given to the IMA default algorithm, but it should
fall back to using SHA256 or SHA1, based on the TPM.
> I additionally implemented the logic to
> select the first PCR bank if the SHA256 PCR bank is not available but I can
> remove it.
>
> SHA256 should be the minimum requirement for boot aggregate. The
> advantage of using the default IMA algorithm is that it will be possible to
> select stronger algorithms when they are supported by the TPM. We might
> introduce a new option to specify only the algorithm for boot aggregate,
> like James suggested to support embedded systems. Let me know which
> option you prefer.
I don't remember James saying that, but if the community really wants
that support, then it should be upstreamed independently, as a
separate patch. Let's first get the basics working.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-03-02 15:11 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1583160394.8544.89.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 3:47 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
>
> > > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > My initial patch attempted to use any common TPM and kernel hash
> > > algorithm to calculate the boot_aggregate. The discussion with James
> > > was pretty clear, which you even stated in the Changelog. Either we
> > > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for
> TPM
> > > 1.2 for the boot-aggregate.
> >
> > Ok, I didn't understand fully. I thought we should use the default IMA
> > algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
> > PCR bank for default algorithm.
>
> Yes, preference is given to the IMA default algorithm, but it should
> fall back to using SHA256 or SHA1, based on the TPM.
Ok. The patch already does it even if the TPM version is not checked.
For TPM 1.2, if the default algorithm is not SHA1 the patch will select
the first PCR bank (SHA1).
Should I send a new patch which explicitly checks the TPM version?
> > I additionally implemented the logic to
> > select the first PCR bank if the SHA256 PCR bank is not available but I can
> > remove it.
> >
> > SHA256 should be the minimum requirement for boot aggregate. The
> > advantage of using the default IMA algorithm is that it will be possible to
> > select stronger algorithms when they are supported by the TPM. We
> might
> > introduce a new option to specify only the algorithm for boot aggregate,
> > like James suggested to support embedded systems. Let me know which
> > option you prefer.
>
> I don't remember James saying that, but if the community really wants
> that support, then it should be upstreamed independently, as a
> separate patch. Let's first get the basics working.
Ok.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox