* [PATCH v2 2/2] capabilities: Add a securebit to disable PR_CAP_AMBIENT_RAISE
From: Andy Lutomirski @ 2015-05-15 6:39 UTC (permalink / raw)
To: Serge Hallyn, Andrew Morton
Cc: Jarkko Sakkinen, Andrew Lutomirski, Ted Ts'o,
Andrew G. Morgan, Linux API, Mimi Zohar, Michael Kerrisk,
Austin S Hemmelgarn, linux-security-module, Aaron Jones,
Serge Hallyn, LKML, Markku Savela, Kees Cook, Jonathan Corbet,
Christoph Lameter, Andy Lutomirski
In-Reply-To: <cover.1431671529.git.luto@kernel.org>
Per Andrew Morgan's request, add a securebit to allow admins to
disable PR_CAP_AMBIENT_RAISE. This securebit will prevent processes
from adding capabilities to their ambient set.
For simplicity, this disables PR_CAP_AMBIENT_RAISE entirely rather
than just disabling setting previously cleared bits.
Requested-by: Andrew G. Morgan <morgan@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Aaron Jones <aaronmdjones@gmail.com>
CC: Ted Ts'o <tytso@mit.edu>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: akpm@linuxfoundation.org
Cc: Andrew G. Morgan <morgan@kernel.org>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Austin S Hemmelgarn <ahferroin7@gmail.com>
Cc: Markku Savela <msa@moth.iki.fi>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
include/uapi/linux/securebits.h | 11 ++++++++++-
security/commoncap.c | 3 ++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
index 985aac9e6bf8..35ac35cef217 100644
--- a/include/uapi/linux/securebits.h
+++ b/include/uapi/linux/securebits.h
@@ -43,9 +43,18 @@
#define SECBIT_KEEP_CAPS (issecure_mask(SECURE_KEEP_CAPS))
#define SECBIT_KEEP_CAPS_LOCKED (issecure_mask(SECURE_KEEP_CAPS_LOCKED))
+/* When set, a process cannot add new capabilities to its ambient set. */
+#define SECURE_NO_CAP_AMBIENT_RAISE 6
+#define SECURE_NO_CAP_AMBIENT_RAISE_LOCKED 7 /* make bit-6 immutable */
+
+#define SECBIT_NO_CAP_AMBIENT_RAISE (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
+ (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
+
#define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
issecure_mask(SECURE_NO_SETUID_FIXUP) | \
- issecure_mask(SECURE_KEEP_CAPS))
+ issecure_mask(SECURE_KEEP_CAPS) | \
+ issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
#define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
#endif /* _UAPI_LINUX_SECUREBITS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index 09541a6a85a0..98aa1b129c12 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -983,7 +983,8 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
if (arg2 == PR_CAP_AMBIENT_RAISE &&
(!cap_raised(current_cred()->cap_permitted, arg3) ||
!cap_raised(current_cred()->cap_inheritable,
- arg3)))
+ arg3) ||
+ issecure(SECURE_NO_CAP_AMBIENT_RAISE)))
return -EPERM;
new = prepare_creds();
--
2.1.0
^ permalink raw reply related
* [PATCH v2 1/2] capabilities: Ambient capabilities
From: Andy Lutomirski @ 2015-05-15 6:39 UTC (permalink / raw)
To: Serge Hallyn, Andrew Morton
Cc: Jarkko Sakkinen, Andrew Lutomirski, Ted Ts'o,
Andrew G. Morgan, Linux API, Mimi Zohar, Michael Kerrisk,
Austin S Hemmelgarn, linux-security-module, Aaron Jones,
Serge Hallyn, LKML, Markku Savela, Kees Cook, Jonathan Corbet,
Christoph Lameter, Andy Lutomirski
In-Reply-To: <cover.1431671529.git.luto@kernel.org>
Credit where credit is due: this idea comes from Christoph Lameter
with a lot of valuable input from Serge Hallyn. This patch is
heavily based on Christoph's patch.
===== The status quo =====
On Linux, there are a number of capabilities defined by the kernel.
To perform various privileged tasks, processes can wield
capabilities that they hold.
Each task has four capability masks: effective (pE), permitted (pP),
inheritable (pI), and a bounding set (X). When the kernel checks
for a capability, it checks pE. The other capability masks serve to
modify what capabilities can be in pE.
Any task can remove capabilities from pE, pP, or pI at any time. If
a task has a capability in pP, it can add that capability to pE
and/or pI. If a task has CAP_SETPCAP, then it can add any
capability to pI, and it can remove capabilities from X.
Tasks are not the only things that can have capabilities; files can
also have capabilities. A file can have no capabilty information at
all [1]. If a file has capability information, then it has a
permitted mask (fP) and an inheritable mask (fI) as well as a single
effective bit (fE) [2]. File capabilities modify the capabilities
of tasks that execve(2) them.
A task that successfully calls execve has its capabilities modified
for the file ultimately being excecuted (i.e. the binary itself if
that binary is ELF or for the interpreter if the binary is a
script.) [3] In the capability evolution rules, for each mask Z, pZ
represents the old value and pZ' represents the new value. The
rules are:
pP' = (X & fP) | (pI & fI)
pI' = pI
pE' = (fE ? pP' : 0)
X is unchanged
For setuid binaries, fP, fI, and fE are modified by a moderately
complicated set of rules that emulate POSIX behavior. Similarly, if
euid == 0 or ruid == 0, then fP, fI, and fE are modified differently
(primary, fP and fI usually end up being the full set). For nonroot
users executing binaries with neither setuid nor file caps, fI and
fP are empty and fE is false.
As an extra complication, if you execute a process as nonroot and fE
is set, then the "secure exec" rules are in effect: AT_SECURE gets
set, LD_PRELOAD doesn't work, etc.
This is rather messy. We've learned that making any changes is
dangerous, though: if a new kernel version allows an unprivileged
program to change its security state in a way that persists cross
execution of a setuid program or a program with file caps, this
persistent state is surprisingly likely to allow setuid or
file-capped programs to be exploited for privilege escalation.
===== The problem =====
Capability inheritance is basically useless.
If you aren't root and you execute an ordinary binary, fI is zero,
so your capabilities have no effect whatsoever on pP'. This means
that you can't usefully execute a helper process or a shell command
with elevated capabilities if you aren't root.
On current kernels, you can sort of work around this by setting fI
to the full set for most or all non-setuid executable files. This
causes pP' = pI for nonroot, and inheritance works. No one does
this because it's a PITA and it isn't even supported on most
filesystems.
If you try this, you'll discover that every nonroot program ends up
with secure exec rules, breaking many things.
This is a problem that has bitten many people who have tried to use
capabilities for anything useful.
===== The proposed change =====
This patch adds a fifth capability mask called the ambient mask
(pA). pA does what pI should have done.
pA obeys the invariant that no bit can ever be set in pA if it is
not set in both pP and pI. Dropping a bit from pP or pI drops that
bit from pA. This ensures that existing programs that try to drop
capabilities still do so, with a complication. Because capability
inheritance is so broken, setting KEEPCAPS, using setresuid to
switch to nonroot uids, or calling execve effectively drops
capabilities. Therefore, setresuid from root to nonroot
conditionally clears pA unless SECBIT_NO_SETUID_FIXUP is set.
Processes that don't like this can re-add bits to pA afterwards.
The capability evolution rules are changed:
pA' = (file caps or setuid or setgid ? 0 : pA)
pP' = (X & fP) | (pI & fI) | pA'
pI' = pI
pE' = (fE ? pP' : pA')
X is unchanged
If you are nonroot but you have a capability, you can add it to pA.
If you do so, your children get that capability in pA, pP, and pE.
For example, you can set pA = CAP_NET_BIND_SERVICE, and your
children can automatically bind low-numbered ports. Hallelujah!
Unprivileged users can create user namespaces, map themselves to a
nonzero uid, and create both privileged (relative to their
namespace) and unprivileged process trees. This is currently more
or less impossible. Hallelujah!
You cannot use pA to try to subvert a setuid, setgid, or file-capped
program: if you execute any such program, pA gets cleared and the
resulting evolution rules are unchanged by this patch.
Users with nonzero pA are unlikely to unintentionally leak that
capability. If they run programs that try to drop privileges,
dropping privileges will still work.
It's worth noting that the degree of paranoia in this patch could
possibly be reduced without causing serious problems. Specifically,
if we allowed pA to persist across executing non-pA-aware setuid
binaries and across setresuid, then, naively, the only capabilities
that could leak as a result would be the capabilities in pA, and any
attacker *already* has those capabilities. This would make me
nervous, though -- setuid binaries that tried to privilege-separate
might fail to do so, and putting CAP_DAC_READ_SEARCH or
CAP_DAC_OVERRIDE into pA could have unexpected side effects.
(Whether these unexpected side effects would be exploitable is an
open question.) I've therefore taken the more paranoid route.
An alternative would be to require PR_SET_NO_NEW_PRIVS before
setting ambient capabilities. I think that this would be annoying
and would make granting otherwise unprivileged users minor ambient
capabilities (CAP_NET_BIND_SERVICE or CAP_NET_RAW for example) much
less useful than it is with this patch.
===== Footnotes =====
[1] Files that are missing the "security.capability" xattr or that
have unrecognized values for that xattr end up with has_cap set to
false. The code that does that appears to be complicated for no
good reason.
[2] The libcap capability mask parsers and formatters are
dangerously misleading and the documentation is flat-out wrong. fE
is *not* a mask; it's a single bit. This has probably confused
every single person who has tried to use file capabilities.
[3] Linux very confusingly processes both the script and the
interpreter if applicable, for reasons that elude me. The results
from thinking about a script's file capabilities and/or setuid bits
are mostly discarded.
Cc: Kees Cook <keescook@chromium.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Aaron Jones <aaronmdjones@gmail.com>
CC: Ted Ts'o <tytso@mit.edu>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: akpm@linuxfoundation.org
Cc: Andrew G. Morgan <morgan@kernel.org>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Austin S Hemmelgarn <ahferroin7@gmail.com>
Cc: Markku Savela <msa@moth.iki.fi>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
fs/proc/array.c | 5 ++-
include/linux/cred.h | 8 ++++
include/uapi/linux/prctl.h | 6 +++
kernel/user_namespace.c | 1 +
security/commoncap.c | 87 +++++++++++++++++++++++++++++++++++++++-----
security/keys/process_keys.c | 1 +
6 files changed, 97 insertions(+), 11 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 1295a00ca316..bc15356d6551 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -282,7 +282,8 @@ static void render_cap_t(struct seq_file *m, const char *header,
static inline void task_cap(struct seq_file *m, struct task_struct *p)
{
const struct cred *cred;
- kernel_cap_t cap_inheritable, cap_permitted, cap_effective, cap_bset;
+ kernel_cap_t cap_inheritable, cap_permitted, cap_effective,
+ cap_bset, cap_ambient;
rcu_read_lock();
cred = __task_cred(p);
@@ -290,12 +291,14 @@ static inline void task_cap(struct seq_file *m, struct task_struct *p)
cap_permitted = cred->cap_permitted;
cap_effective = cred->cap_effective;
cap_bset = cred->cap_bset;
+ cap_ambient = cred->cap_ambient;
rcu_read_unlock();
render_cap_t(m, "CapInh:\t", &cap_inheritable);
render_cap_t(m, "CapPrm:\t", &cap_permitted);
render_cap_t(m, "CapEff:\t", &cap_effective);
render_cap_t(m, "CapBnd:\t", &cap_bset);
+ render_cap_t(m, "CapAmb:\t", &cap_ambient);
}
static inline void task_seccomp(struct seq_file *m, struct task_struct *p)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 2fb2ca2127ed..05178874e771 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -122,6 +122,7 @@ struct cred {
kernel_cap_t cap_permitted; /* caps we're permitted */
kernel_cap_t cap_effective; /* caps we can actually use */
kernel_cap_t cap_bset; /* capability bounding set */
+ kernel_cap_t cap_ambient; /* Ambient capability set */
#ifdef CONFIG_KEYS
unsigned char jit_keyring; /* default keyring to attach requested
* keys to */
@@ -197,6 +198,13 @@ static inline void validate_process_creds(void)
}
#endif
+static inline bool cap_ambient_invariant_ok(const struct cred *cred)
+{
+ return cap_issubset(cred->cap_ambient,
+ cap_intersect(cred->cap_permitted,
+ cred->cap_inheritable));
+}
+
/**
* get_new_cred - Get a reference on a new set of credentials
* @cred: The new credentials to reference
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 31891d9535e2..65407f867e82 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -190,4 +190,10 @@ struct prctl_mm_map {
# define PR_FP_MODE_FR (1 << 0) /* 64b FP registers */
# define PR_FP_MODE_FRE (1 << 1) /* 32b compatibility */
+/* Control the ambient capability set */
+#define PR_CAP_AMBIENT 47
+# define PR_CAP_AMBIENT_GET 1
+# define PR_CAP_AMBIENT_RAISE 2
+# define PR_CAP_AMBIENT_LOWER 3
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 4109f8320684..dab0f808235a 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -39,6 +39,7 @@ static void set_cred_user_ns(struct cred *cred, struct user_namespace *user_ns)
cred->cap_inheritable = CAP_EMPTY_SET;
cred->cap_permitted = CAP_FULL_SET;
cred->cap_effective = CAP_FULL_SET;
+ cred->cap_ambient = CAP_EMPTY_SET;
cred->cap_bset = CAP_FULL_SET;
#ifdef CONFIG_KEYS
key_put(cred->request_key_auth);
diff --git a/security/commoncap.c b/security/commoncap.c
index f66713bd7450..09541a6a85a0 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -272,6 +272,16 @@ int cap_capset(struct cred *new,
new->cap_effective = *effective;
new->cap_inheritable = *inheritable;
new->cap_permitted = *permitted;
+
+ /*
+ * Mask off ambient bits that are no longer both permitted and
+ * inheritable.
+ */
+ new->cap_ambient = cap_intersect(new->cap_ambient,
+ cap_intersect(*permitted,
+ *inheritable));
+ if (WARN_ON(!cap_ambient_invariant_ok(new)))
+ return -EINVAL;
return 0;
}
@@ -352,6 +362,7 @@ static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
/*
* pP' = (X & fP) | (pI & fI)
+ * The addition of pA' is handled later.
*/
new->cap_permitted.cap[i] =
(new->cap_bset.cap[i] & permitted) |
@@ -479,10 +490,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
{
const struct cred *old = current_cred();
struct cred *new = bprm->cred;
- bool effective, has_cap = false;
+ bool effective, has_cap = false, is_setid;
int ret;
kuid_t root_uid;
+ if (WARN_ON(!cap_ambient_invariant_ok(old)))
+ return -EPERM;
+
effective = false;
ret = get_file_caps(bprm, &effective, &has_cap);
if (ret < 0)
@@ -527,8 +541,9 @@ skip:
*
* In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
*/
- if ((!uid_eq(new->euid, old->uid) ||
- !gid_eq(new->egid, old->gid) ||
+ is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
+
+ if ((is_setid ||
!cap_issubset(new->cap_permitted, old->cap_permitted)) &&
bprm->unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
/* downgrade; they get no more than they had, and maybe less */
@@ -544,10 +559,24 @@ skip:
new->suid = new->fsuid = new->euid;
new->sgid = new->fsgid = new->egid;
+ /* File caps or setid cancels ambient. */
+ if (has_cap || is_setid)
+ cap_clear(new->cap_ambient);
+
+ /*
+ * Now that we've computed pA', update pP' to give:
+ * pP' = (X & fP) | (pI & fI) | pA'
+ */
+ new->cap_permitted = cap_combine(new->cap_permitted, new->cap_ambient);
+
if (effective)
new->cap_effective = new->cap_permitted;
else
- cap_clear(new->cap_effective);
+ new->cap_effective = new->cap_ambient;
+
+ if (WARN_ON(!cap_ambient_invariant_ok(new)))
+ return -EPERM;
+
bprm->cap_effective = effective;
/*
@@ -562,7 +591,7 @@ skip:
* Number 1 above might fail if you don't have a full bset, but I think
* that is interesting information to audit.
*/
- if (!cap_isclear(new->cap_effective)) {
+ if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
issecure(SECURE_NOROOT)) {
@@ -573,6 +602,10 @@ skip:
}
new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
+
+ if (WARN_ON(!cap_ambient_invariant_ok(new)))
+ return -EPERM;
+
return 0;
}
@@ -594,7 +627,7 @@ int cap_bprm_secureexec(struct linux_binprm *bprm)
if (!uid_eq(cred->uid, root_uid)) {
if (bprm->cap_effective)
return 1;
- if (!cap_isclear(cred->cap_permitted))
+ if (!cap_issubset(cred->cap_permitted, cred->cap_ambient))
return 1;
}
@@ -696,10 +729,18 @@ static inline void cap_emulate_setxuid(struct cred *new, const struct cred *old)
uid_eq(old->suid, root_uid)) &&
(!uid_eq(new->uid, root_uid) &&
!uid_eq(new->euid, root_uid) &&
- !uid_eq(new->suid, root_uid)) &&
- !issecure(SECURE_KEEP_CAPS)) {
- cap_clear(new->cap_permitted);
- cap_clear(new->cap_effective);
+ !uid_eq(new->suid, root_uid))) {
+ if (!issecure(SECURE_KEEP_CAPS)) {
+ cap_clear(new->cap_permitted);
+ cap_clear(new->cap_effective);
+ }
+
+ /*
+ * Pre-ambient programs except setresuid to nonroot followed
+ * by exec to drop capabilities. We should make sure that
+ * this remains the case.
+ */
+ cap_clear(new->cap_ambient);
}
if (uid_eq(old->euid, root_uid) && !uid_eq(new->euid, root_uid))
cap_clear(new->cap_effective);
@@ -929,6 +970,32 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
return commit_creds(new);
+ case PR_CAP_AMBIENT:
+ if (((!cap_valid(arg3)) | arg4 | arg5))
+ return -EINVAL;
+
+ if (arg2 == PR_CAP_AMBIENT_GET) {
+ return !!cap_raised(current_cred()->cap_ambient, arg3);
+ } else if (arg2 != PR_CAP_AMBIENT_RAISE &&
+ arg2 != PR_CAP_AMBIENT_LOWER) {
+ return -EINVAL;
+ } else {
+ if (arg2 == PR_CAP_AMBIENT_RAISE &&
+ (!cap_raised(current_cred()->cap_permitted, arg3) ||
+ !cap_raised(current_cred()->cap_inheritable,
+ arg3)))
+ return -EPERM;
+
+ new = prepare_creds();
+ if (!new)
+ return -ENOMEM;
+ if (arg2 == PR_CAP_AMBIENT_RAISE)
+ cap_raise(new->cap_ambient, arg3);
+ else
+ cap_lower(new->cap_ambient, arg3);
+ return commit_creds(new);
+ }
+
default:
/* No functionality available - continue with default */
return -ENOSYS;
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index bd536cb221e2..43b4cddbf2b3 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -848,6 +848,7 @@ void key_change_session_keyring(struct callback_head *twork)
new->cap_inheritable = old->cap_inheritable;
new->cap_permitted = old->cap_permitted;
new->cap_effective = old->cap_effective;
+ new->cap_ambient = old->cap_ambient;
new->cap_bset = old->cap_bset;
new->jit_keyring = old->jit_keyring;
--
2.1.0
^ permalink raw reply related
* [PATCH v2 0/2] capabilities: Ambient capabilities
From: Andy Lutomirski @ 2015-05-15 6:39 UTC (permalink / raw)
To: Serge Hallyn, Andrew Morton
Cc: Jarkko Sakkinen, Andrew Lutomirski, Ted Ts'o,
Andrew G. Morgan, Linux API, Mimi Zohar, Michael Kerrisk,
Austin S Hemmelgarn, linux-security-module, Aaron Jones,
Serge Hallyn, LKML, Markku Savela, Kees Cook, Jonathan Corbet
This adds ambient capabilities. See the individual patch changelogs
for details.
Preliminary userspace code is here:
https://git.kernel.org/cgit/linux/kernel/git/luto/util-linux-playground.git/commit/?h=cap_ambient&id=7f5afbd175d2
I'll follow up with a rough draft of a man-pages patch.
Changes from v1:
- Lots of cleanups to the ambient cap code.
- The securebit is new.
Andy Lutomirski (2):
capabilities: Ambient capabilities
capabilities: Add a securebit to disable PR_CAP_AMBIENT_RAISE
fs/proc/array.c | 5 ++-
include/linux/cred.h | 8 ++++
include/uapi/linux/prctl.h | 6 +++
include/uapi/linux/securebits.h | 11 +++++-
kernel/user_namespace.c | 1 +
security/commoncap.c | 88 ++++++++++++++++++++++++++++++++++++-----
security/keys/process_keys.c | 1 +
7 files changed, 108 insertions(+), 12 deletions(-)
--
2.1.0
^ permalink raw reply
* Re: [CFT][PATCH 0/10] Making new mounts of proc and sysfs as safe as bind mounts
From: Andy Lutomirski @ 2015-05-15 6:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Greg Kroah-Hartman, Linux Containers, Linux FS Devel, Linux API,
Serge E. Hallyn, Richard Weinberger, Kenton Varda,
Michael Kerrisk-manpages, Stéphane Graber, Eric Windisch,
Tejun Heo
In-Reply-To: <87oalmg90j.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Thu, May 14, 2015 at 2:10 PM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
>
>> On Thu, May 14, 2015 at 12:30:45PM -0500, Eric W. Biederman wrote:
>>>
>>> The code is currently available at:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-testing
>>>
>>> HEAD: a524faf520600968e58bbc732063fccf2fdf9199 mnt: Update fs_fully_visible to test for permanently empty directories
>>>
>>> The problem: Mounting a new instance of proc of sysfs can allow things
>>> that a bind mount of those filesystems would not.
>>>
>>> That is the cases I am dealing with are:
>>> unshare --user --net --mount ; mount -t sysfs ...
>>> unshare --user --pid --mount ; mount -t proc ...
>>>
>>> The big change is that this set of changes enforces the preservation of
>>> locked mount flags, from the existing mount to the current mount. Which
>>> means that if proc was mounted read-only the current current will allow
>>> a new instance of proc to be mounted read-write, and this set of changes
>>> enforces that proc remain read-only.
>>>
>>> The other gotcha is that the current code does not properly detect empty
>>> directories so to prevent things slipping through the cracks this set of
>>> changes annotates all mount points where nothing will be revealed if
>>> the filesystem mounted on top is removed.
>>>
>>> Enforcing the administrators policy can actually matter in the real
>>> world as has been shown by the recent docker issue.
>>>
>>> With this patchset I have two concerns:
>>> - The enforcement of mount flag preservation on proc and sysfs may break
>>> things. (I am especially worried about the implicit adding of nodev).
>>
>> What do you mean by this? What got added?
>
> In a user namespace mounting a filesystem implicitly adds nodev.
>
> When I started enforcing not clearing bits that root had set on a
> filesystem in mount -o remount the implicit nodev wound up being
> an issue that broke userspace for no good reason. The fix was
> to implicitly add nodev in remount as well.
>
> Taking a second look at this nodev is implicitly added before the
> fs_fully_visible check so even for applications that are know how the
> original proc was mounted (and don't see an implicit nodev) and that
> don't add nodev (because they ''know'' the mount flags) this change
> should not be a problem. Hooray! One less scary thing.
Can we please just get rid of this implicit nodev thing once and for all? If it
breaks some really weird /proc use case, then I think the right fix is to
stop enforcing the nodev lock for the proc fully visible check. After
all, /proc doesn't contain useful device nodes anyway.
Other than that, the code here looks okay to me on brief inspection.
--Andy
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Andy Lutomirski @ 2015-05-15 6:23 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, Steve Grubb, Eric W. Biederman, Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA, Eric Paris,
arozansk-H+wXaHxf7aLQT0dZR+AlfA, Serge E. Hallyn, Mimi Zohar,
Al Viro, Linux FS Devel, Linux API, Network Development
In-Reply-To: <20150515023221.GC965-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Thu, May 14, 2015 at 7:32 PM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 15/05/14, Paul Moore wrote:
>> * Look at our existing audit records to determine which records should have
>> namespace and container ID tokens added. We may only want to add the
>> additional fields in the case where the namespace/container ID tokens are not
>> the init namespace.
>
> If we have a record that ties a set of namespace IDs with a container
> ID, then I expect we only need to list the containerID along with auid
> and sessionID.
The problem here is that the kernel has no concept of a "container", and I
don't think it makes any sense to add one just for audit. "Container" is a
marketing term used by some userspace tools.
I can imagine that both audit could benefit from a concept of a
namespace *path* that understands nesting (e.g. root/2/5/1 or
something along those lines). Mapping these to "containers" belongs
in userspace, I think.
--Andy
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Richard Guy Briggs @ 2015-05-15 2:32 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, ebiederm, containers, linux-kernel, linux-audit,
eparis, arozansk, serge, zohar, viro, linux-fsdevel, linux-api,
netdev
In-Reply-To: <12675437.ssZNCck7zG@sifl>
On 15/05/14, Paul Moore wrote:
> On Thursday, May 14, 2015 10:57:14 AM Steve Grubb wrote:
> > On Tuesday, May 12, 2015 03:57:59 PM Richard Guy Briggs wrote:
> > > On 15/05/05, Steve Grubb wrote:
> > > > I think there needs to be some more discussion around this. It seems
> > > > like this is not exactly recording things that are useful for audit.
> > >
> > > It seems to me that either audit has to assemble that information, or
> > > the kernel has to do so. The kernel doesn't know about containers
> > > (yet?).
> >
> > Auditing is something that has a lot of requirements imposed on it by
> > security standards. There was no requirement to have an auid until audit
> > came along and said that uid is not good enough to know who is issuing
> > commands because of su or sudo. There was no requirement for sessionid
> > until we had to track each action back to a login so we could see if the
> > login came from the expected place.
> >
> > What I am saying is we have the same situation. Audit needs to track a
> > container and we need an ID. The information that is being logged is not
> > useful for auditing. Maybe someone wants that info in syslog, but I doubt
> > it. The audit trail's purpose is to allow a security officer to reconstruct
> > the events to determine what happened during some security incident.
>
> As Eric, and others, have stated, the container concept is a userspace idea,
> not a kernel idea; the kernel only knows, and cares about, namespaces. This
> is unlikely to change.
>
> However, as Steve points out, there is precedence for the kernel to record
> userspace tokens for the sake of audit. Personally I'm not a big fan of this
> in general, but I do recognize that it does satisfy a legitimate need. Think
> of things like auid and the sessionid as necessary evils; audit is already
> chock full of evilness I doubt one more will doom us all to hell.
>
> Moving forward, I'd like to see the following:
>
> * Record the creation/removal/mgmt of the individual namespaces as Richard's
> patchset currently does. However, I'd suggest using an explicit namespace
> value for the init namespace instead of the "unset" value in the V6 patchset
> (my apologies if you've already changed this Richard, I haven't looked at V7
> yet).
The "unset" (none) value is only there before the first namespaces have
been created. After that, any new ones are created relative to the init
namespace of that type.
> * Create a container ID token (unsigned 32-bit integer?), similar to
> auid/sessionid, that is set by userspace and carried by the kernel to be used
> in audit records. I'd like to see some discussion on how we manage this, e.g.
> how do handle container ID inheritance, how do we handle nested containers
> (setting the containerid when it is already set), do we care if multiple
> different containers share the same namespace config, etc.?
(Addressed in another reply.) Nested will need some careful thought...
> * When userspace sets the container ID, emit a new audit record with the
> associated namespace tokens and the container ID.
That was the goal of AUDIT_VIRT_CONTROL or AUDIT_NS_INFO messages from
userspace into the kernel.
> * Look at our existing audit records to determine which records should have
> namespace and container ID tokens added. We may only want to add the
> additional fields in the case where the namespace/container ID tokens are not
> the init namespace.
If we have a record that ties a set of namespace IDs with a container
ID, then I expect we only need to list the containerID along with auid
and sessionID.
> Can we all live with this? If not, please suggest some alternate ideas;
> simply shouting "IT'S ALL CRAP!" isn't helpful for anyone ... it may be true,
> but it doesn't help us solve the problem ;)
Thanks Paul.
> paul moore
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Richard Guy Briggs @ 2015-05-15 2:25 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Paul Moore, Steve Grubb, containers, linux-kernel, linux-audit,
eparis, arozansk, serge, zohar, viro, linux-fsdevel, linux-api,
netdev
In-Reply-To: <87bnhmbp8e.fsf@x220.int.ebiederm.org>
On 15/05/14, Eric W. Biederman wrote:
> Paul Moore <pmoore@redhat.com> writes:
> > As Eric, and others, have stated, the container concept is a userspace idea,
> > not a kernel idea; the kernel only knows, and cares about, namespaces. This
> > is unlikely to change.
> >
> > However, as Steve points out, there is precedence for the kernel to record
> > userspace tokens for the sake of audit. Personally I'm not a big fan of this
> > in general, but I do recognize that it does satisfy a legitimate need. Think
> > of things like auid and the sessionid as necessary evils; audit is already
> > chock full of evilness I doubt one more will doom us all to hell.
> >
> > Moving forward, I'd like to see the following:
>
> > * Create a container ID token (unsigned 32-bit integer?), similar to
> > auid/sessionid, that is set by userspace and carried by the kernel to be used
> > in audit records. I'd like to see some discussion on how we manage this, e.g.
> > how do handle container ID inheritance, how do we handle nested containers
> > (setting the containerid when it is already set), do we care if multiple
> > different containers share the same namespace config, etc.?
>
>
> > Can we all live with this? If not, please suggest some alternate ideas;
> > simply shouting "IT'S ALL CRAP!" isn't helpful for anyone ... it may be true,
> > but it doesn't help us solve the problem ;)
>
> Without stopping and defining what someone means by container I think it
> is pretty much nonsense.
Not complete, but this is why I'm asking for a standards document...
> Should every vsftp connection get a container every? Every chrome tab?
>
> At some of the connections per second numbers I have seen we might
> exhaust a 32bit number in an hour or two. Will any of that make sense
> to someone reading the audit logs?
So making it 64bits buys us some time, but sure... I think your
definition of a container may be a bit more liberal than what we're
trying to understand...
> Without considerning that container creation is an unprivileged
> operation I think it is pretty much nonsense. Do I get to say I am any
> container I want? That would seem to invalidate the concept of
> userspace setting a container id.
Ok, my impression was that we're dealing with a privileged application
as I alluded with the need to create a new CAP_AUDIT_CONTAINER_ID or
something...
> How does any of this interact with setns? AKA entering a container?
You mean entering another namespace that might all be part of one
container? Or an an application attempting to enter the namespace of
another container?
> I will go as far as looking at patches. If someone comes up with
> a mission statement about what they are actually trying to achieve and a
> mechanism that actually achieves that, and that allows for containers to
> nest we can talk about doing something like that.
I don't pretend these patches are anywhere near finished or ready for
upstream.
> But for right now I just hear proposals for things that make no sense
> and can not possibly work. Not least because it will require modifying
> every program that creates a container and who knows how many of them
> there are. Especially since you don't need to be root. Modifying
> /usr/bin/unshare seems a little far out to me.
My understanding is that just spawning or changing namespace doesn't
imply spawning or changing containers. I also don't necessarily assume
that creating a container is an atomic operation, though that concept
might make some sense to understand or predict the boundaries of
actions...
> Eric
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Richard Guy Briggs @ 2015-05-15 2:11 UTC (permalink / raw)
To: Oren Laadan
Cc: Steve Grubb, linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
linux-kernel, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
pmoore-H+wXaHxf7aLQT0dZR+AlfA, linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
eparis-FjpueFixGhCM4zKIHC2jIg,
zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Eric Biederman
In-Reply-To: <CAA4jN2bgynVTwF+owtXgq06JMLQJpy_qokpD0mAguNYeDxmh1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 15/05/14, Oren Laadan wrote:
> On Thu, May 14, 2015 at 8:48 PM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> >
> > > > > Recording each instance of a name space is giving me something that I
> > > > > cannot use to do queries required by the security target. Given these
> > > > > events, how do I locate a web server event where it accesses a
> > watched
> > > > > file? That authentication failed? That an update within the container
> > > > > failed?
> > > > >
> > > > > The requirements are that we have to log the creation, suspension,
> > > > > migration, and termination of a container. The requirements are not
> > on
> > > > > the individual name space.
> > > >
> > > > Ok. Do we have a robust definition of a container?
> > >
> > > We call the combination of name spaces, cgroups, and seccomp rules a
> > > container.
> >
> > Can you detail what information is required from each?
> >
> > > > Where is that definition managed?
> > >
> > > In the thing that invokes a container.
> >
> > I was looking for a reference to a standards document rather than an
> > application...
> >
> >
> [focusing on "containers id" - snipped the rest away]
>
> I am unfamiliar with the audit subsystem, but work with namespaces in other
> contexts. Perhaps the term "container" is overloaded here. The definition
> suggested by Steve in this thread makes sense to me: "a combination of
> namespaces". I imagine people may want to audit subsets of namespaces.
I assume it would be a bit more than that, including cgroup and seccomp info.
> For namespaces, can use a string like "A:B:C:D:E:F" as an identifier for a
> particular combination, where A-F are respective namespaces identifiers.
> (Can be taken for example from /proc/PID/ns/{mnt,uts,ipc,user,pid,net}).
> That will even be grep-able to locate records related to a particular
> subset
> of namespaces. So a "container" in the classic meaning would have all A-F
> unique and different from the init process, but processes separated only by
> e.g. mnt-ns and net-ns will differ from the init process in A and F.
>
> (If a string is a no go, then perhaps combine the IDs in a unique way into a
> super ID).
I'd be fine with either, even including the nsfs deviceID.
> Oren.
- RGB
--
Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Richard Guy Briggs @ 2015-05-15 2:03 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Steve Grubb,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA, eparis-FjpueFixGhCM4zKIHC2jIg,
pmoore-H+wXaHxf7aLQT0dZR+AlfA, arozansk-H+wXaHxf7aLQT0dZR+AlfA,
serge-A9i7LUbDfNHQT0dZR+AlfA,
zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87iobvnp1t.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On 15/05/14, Eric W. Biederman wrote:
> Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > On Tuesday, May 12, 2015 03:57:59 PM Richard Guy Briggs wrote:
> >> On 15/05/05, Steve Grubb wrote:
> >> > I think there needs to be some more discussion around this. It seems like
> >> > this is not exactly recording things that are useful for audit.
> >>
> >> It seems to me that either audit has to assemble that information, or
> >> the kernel has to do so. The kernel doesn't know about containers
> >> (yet?).
> >
> > Auditing is something that has a lot of requirements imposed on it by security
> > standards. There was no requirement to have an auid until audit came along and
> > said that uid is not good enough to know who is issuing commands because of su
> > or sudo. There was no requirement for sessionid until we had to track each
> > action back to a login so we could see if the login came from the expected
> > place.
>
> Stop right there.
>
> You want a global identifier in a realm where only relative identifiers
> exist, and make sense.
I am assuming he wants an identifier unique per container on one kernel
and what happens on other kernels is a matter for a management
application to take care of. This kernel doesn't have to deal with it
other than taking information from a container management application.
> I am sorry that isn't going to happen. EVER.
>
> Square peg, round hole. It doesn't work, it doesn't make sense, and
> most especially it doesn't allow anyone to reconstruct anything, because
> it does not make sense and does not match what the kernel is doing.
>
> Container IDs do not, and will not exist. There is probably something
> reasonable in your request but until you stop talking that nonsense I
> can't see it.
I didn't see anything in any of what Steve said that suggested it was to
be unique beyond that one kernel.
> Global IDs take us into the namespace of namespaces problem and that
> isn't going to happen. I have already bent as far in this direction as
> I can go. Further namespace creation is not a privileged event which
> makes the requestion for a container ID make even less sense. With
> anyone able to create whatever they want it will not be a identifier
> that makes any sense to someone reading an audit log.
Again, I assume this is up to a container management application that
will manage its pool of container hosts and an audit aggregator.
You keep raising an objection about the unworkability of a "namespace of
namespaces". Just so we are all on the same page here, can you explain
exactly what you mean with "namespace of namespaces"?
> Eric
- RGB
--
Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Eric W. Biederman @ 2015-05-15 1:31 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Richard Guy Briggs,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA, eparis-FjpueFixGhCM4zKIHC2jIg,
arozansk-H+wXaHxf7aLQT0dZR+AlfA, serge-A9i7LUbDfNHQT0dZR+AlfA,
zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <12675437.ssZNCck7zG@sifl>
Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> As Eric, and others, have stated, the container concept is a userspace idea,
> not a kernel idea; the kernel only knows, and cares about, namespaces. This
> is unlikely to change.
>
> However, as Steve points out, there is precedence for the kernel to record
> userspace tokens for the sake of audit. Personally I'm not a big fan of this
> in general, but I do recognize that it does satisfy a legitimate need. Think
> of things like auid and the sessionid as necessary evils; audit is already
> chock full of evilness I doubt one more will doom us all to hell.
>
> Moving forward, I'd like to see the following:
> * Create a container ID token (unsigned 32-bit integer?), similar to
> auid/sessionid, that is set by userspace and carried by the kernel to be used
> in audit records. I'd like to see some discussion on how we manage this, e.g.
> how do handle container ID inheritance, how do we handle nested containers
> (setting the containerid when it is already set), do we care if multiple
> different containers share the same namespace config, etc.?
> Can we all live with this? If not, please suggest some alternate ideas;
> simply shouting "IT'S ALL CRAP!" isn't helpful for anyone ... it may be true,
> but it doesn't help us solve the problem ;)
Without stopping and defining what someone means by container I think it
is pretty much nonsense.
Should every vsftp connection get a container every? Every chrome tab?
At some of the connections per second numbers I have seen we might
exhaust a 32bit number in an hour or two. Will any of that make sense
to someone reading the audit logs?
Without considerning that container creation is an unprivileged
operation I think it is pretty much nonsense. Do I get to say I am any
container I want? That would seem to invalidate the concept of
userspace setting a container id.
How does any of this interact with setns? AKA entering a container?
I will go as far as looking at patches. If someone comes up with
a mission statement about what they are actually trying to achieve and a
mechanism that actually achieves that, and that allows for containers to
nest we can talk about doing something like that.
But for right now I just hear proposals for things that make no sense
and can not possibly work. Not least because it will require modifying
every program that creates a container and who knows how many of them
there are. Especially since you don't need to be root. Modifying
/usr/bin/unshare seems a little far out to me.
Eric
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Oren Laadan @ 2015-05-15 1:10 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Containers, linux-kernel,
pmoore-H+wXaHxf7aLQT0dZR+AlfA, linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
eparis-FjpueFixGhCM4zKIHC2jIg, Steve Grubb,
zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Eric Biederman
In-Reply-To: <20150515004855.GB10526-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Thu, May 14, 2015 at 8:48 PM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> > > > Recording each instance of a name space is giving me something that I
> > > > cannot use to do queries required by the security target. Given these
> > > > events, how do I locate a web server event where it accesses a
> watched
> > > > file? That authentication failed? That an update within the container
> > > > failed?
> > > >
> > > > The requirements are that we have to log the creation, suspension,
> > > > migration, and termination of a container. The requirements are not
> on
> > > > the individual name space.
> > >
> > > Ok. Do we have a robust definition of a container?
> >
> > We call the combination of name spaces, cgroups, and seccomp rules a
> > container.
>
> Can you detail what information is required from each?
>
> > > Where is that definition managed?
> >
> > In the thing that invokes a container.
>
> I was looking for a reference to a standards document rather than an
> application...
>
>
[focusing on "containers id" - snipped the rest away]
I am unfamiliar with the audit subsystem, but work with namespaces in other
contexts. Perhaps the term "container" is overloaded here. The definition
suggested by Steve in this thread makes sense to me: "a combination of
namespaces". I imagine people may want to audit subsets of namespaces.
For namespaces, can use a string like "A:B:C:D:E:F" as an identifier for a
particular combination, where A-F are respective namespaces identifiers.
(Can be taken for example from /proc/PID/ns/{mnt,uts,ipc,user,pid,net}).
That will even be grep-able to locate records related to a particular
subset
of namespaces. So a "container" in the classic meaning would have all A-F
unique and different from the init process, but processes separated only by
e.g. mnt-ns and net-ns will differ from the init process in A and F.
(If a string is a no go, then perhaps combine the IDs in a unique way into a
super ID).
Oren.
^ permalink raw reply
* Re: [RFC PATCH 00/11] drm/i915: Expose OA metrics via perf PMU
From: Robert Bragg @ 2015-05-15 1:07 UTC (permalink / raw)
To: Peter Zijlstra
Cc: intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter,
Jani Nikula, David Airlie, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150508162452.GR27504-ndre7Fmf5hadTX5a5knrm8zTDFooKrT+cvkQGrU6aU0@public.gmane.org>
On Fri, May 8, 2015 at 5:24 PM, Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> On Thu, May 07, 2015 at 03:15:43PM +0100, Robert Bragg wrote:
>
>> I've changed the uapi for configuring the i915_oa specific attributes
>> when calling perf_event_open(2) whereby instead of cramming lots of
>> bitfields into the perf_event_attr config members, I'm now
>> daisy-chaining a drm_i915_oa_event_attr_t structure off of a single
>> config member that's extensible and validated in the same way as the
>> perf_event_attr struct. I've found this much nicer to work with while
>> being neatly extensible too.
>
> This worries me a bit.. is there more background for this?
Would it maybe be helpful to see the before and after? I had kept this
uapi change in a separate patch for a while locally but in the end
decided to squash it before sending out my updated series.
Although I did find it a bit awkward with the bitfields, I was mainly
concerned about the extensibility of packing logically separate
attributes into the config members and had heard similar concerns from
a few others who had been experimenting with my patches too.
A few simple attributes I can think of a.t.m that we might want to add
in the future are:
- control of the OABUFFER size
- a way to ask the kernel to collect reports at the beginning and end
of batch buffers, in addition to periodic reports
- alternative ways to uniquely identify a context to support tools
profiling a single context not necessarily owned by the current
process
It could also be interesting to expose some counter configuration
through these attributes too. E.g. on Broadwell+ we have 14 'Flexible
EU' counters included in the OA unit reports, each with a 16bit
configuration.
In a more extreme case it might also be useful to allow userspace to
specify a complete counter config, which (depending on the
configuration) could be over 100 32bit values to select the counter
signals + configure the corresponding combining logic.
Since this pmu is in a device driver it also seemed reasonably
appropriate to de-couple it slightly from the core perf_event_attr
structure by allowing driver extensible attributes.
I wonder if it might be less worrisome if the i915_oa_copy_attr() code
were instead a re-usable utility perhaps maintained in events/core.c,
so if other pmu drivers were to follow suite there would be less risk
of a mistake being made here?
Regards,
- Robert
^ permalink raw reply
* Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
From: Richard Guy Briggs @ 2015-05-15 0:48 UTC (permalink / raw)
To: Steve Grubb
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA, eparis-FjpueFixGhCM4zKIHC2jIg,
pmoore-H+wXaHxf7aLQT0dZR+AlfA, arozansk-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, serge-A9i7LUbDfNHQT0dZR+AlfA,
zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <2918460.dpKocsKt4o@x2>
On 15/05/14, Steve Grubb wrote:
> On Tuesday, May 12, 2015 03:57:59 PM Richard Guy Briggs wrote:
> > On 15/05/05, Steve Grubb wrote:
> > > I think there needs to be some more discussion around this. It seems like
> > > this is not exactly recording things that are useful for audit.
> >
> > It seems to me that either audit has to assemble that information, or
> > the kernel has to do so. The kernel doesn't know about containers
> > (yet?).
>
> Auditing is something that has a lot of requirements imposed on it by security
> standards. There was no requirement to have an auid until audit came along and
> said that uid is not good enough to know who is issuing commands because of su
> or sudo. There was no requirement for sessionid until we had to track each
> action back to a login so we could see if the login came from the expected
> place.
>
> What I am saying is we have the same situation. Audit needs to track a
> container and we need an ID. The information that is being logged is not
> useful for auditing. Maybe someone wants that info in syslog, but I doubt it.
> The audit trail's purpose is to allow a security officer to reconstruct the
> events to determine what happened during some security incident.
I agree the information being logged is not yet useful, but it is a
component of what would be. I wasn't ever thinking about syslog... It
is this trail that I was trying to help create.
> What they would want to know is what resources were assigned; if two
> containers shared a resource, what resource and container was it shared with;
> if two containers can communicate, we need to see or control information flow
> when necessary; and we need to see termination and release of resources.
So, namespaces are a big part of this. I understand how they are
spawned and potentially shared. I have a more vague idea about how
cgroups contribute to this concept of a container. So far, I have very
little idea how seccomp contributes, but I assume that it will also need
to be part of this tracking.
> Also, if the host OS cannot make sense of the information being logged because
> the pid maps to another process name, or a uid maps to another user, or a file
> access maps to something not in the host's, then we need the container to do
> its own auditing and resolve these mappings and optionally pass these to an
> aggregation server.
I'm open to both being possible.
> Nothing else makes sense.
>
> > > On Friday, April 17, 2015 03:35:52 AM Richard Guy Briggs wrote:
> > > > Log the creation and deletion of namespace instances in all 6 types of
> > > > namespaces.
> > > >
> > > > Twelve new audit message types have been introduced:
> > > > AUDIT_NS_INIT_MNT 1330 /* Record mount namespace instance
> > > > creation
> > > > */ AUDIT_NS_INIT_UTS 1331 /* Record UTS namespace instance
> > > > creation */ AUDIT_NS_INIT_IPC 1332 /* Record IPC namespace
> > > > instance creation */ AUDIT_NS_INIT_USER 1333 /* Record USER
> > > > namespace instance creation */ AUDIT_NS_INIT_PID 1334 /* Record
> > > > PID namespace instance creation */ AUDIT_NS_INIT_NET 1335 /*
> > > > Record NET namespace instance creation */ AUDIT_NS_DEL_MNT 1336
> > > > /* Record mount namespace instance deletion */ AUDIT_NS_DEL_UTS
> > > > 1337
> > > >
> > > > /* Record UTS namespace instance deletion */ AUDIT_NS_DEL_IPC
> > > >
> > > > 1338 /* Record IPC namespace instance deletion */ AUDIT_NS_DEL_USER
> > > >
> > > > 1339 /* Record USER namespace instance deletion */ AUDIT_NS_DEL_PID
> > > >
> > > > 1340 /* Record PID namespace instance deletion */ AUDIT_NS_DEL_NET
> > > >
> > > > 1341 /* Record NET namespace instance deletion */
> > >
> > > The requirements for auditing of containers should be derived from VPP. In
> > > it, it asks for selectable auditing, selective audit, and selective audit
> > > review. What this means is that we need the container and all its
> > > children to have one identifier that is inserted into all the events that
> > > are associated with the container.
> >
> > Is that requirement for the records that are sent from the kernel, or
> > for the records stored by auditd, or by another facility that delivers
> > those records to a final consumer?
>
> A little of both. Selective audit means that you can set rules to include or
> exclude an event. This is done in the kernel. Selectable review means that the
> user space tools need to be able to skip past records not of interest to a
> specific line of inquiry. Also, logging everything and letting user space work
> it out later is also not a solution because the needle is harder to find in a
> larger haystack. Or, the logs may rotate and its gone forever because the
> partition is filled.
I agree it needs to be a balance of flexibility and efficiency.
> > > With this, its possible to do a search for all events related to a
> > > container. Its possible to exclude events from a container. Its possible
> > > to not get any events.
> > >
> > > The requirements also call out for the identification of the subject. This
> > > means that the event should be bound to a syscall such as clone, setns, or
> > > unshare.
> >
> > Is it useful to have a reference of the init namespace set from which
> > all others are spawned?
>
> For things directly observable by the init name space, yes.
Ok, so we'll need to have a way to document that initial state on boot
before any other processes start, preferably in one clear brief record.
> > If it isn't bound, I assume the subject should be added to the message
> > format? I'm thinking of messages without an audit_context such as audit
> > user messages (such as AUDIT_NS_INFO and AUDIT_VIRT_CONTROL).
>
> Making these events auxiliary records to a syscall is all that is needed. The
> same way that PATH is added to an open event. If someone wants to have
> container/namespace events, they add a rule on clone(2).
This doesn't make sense. The point of this type of record is to have a
way for a userspace container manager (which maybe should have a new CAP
type) to tie the creation of namespaces to a specific container name or
ID. It might even contain cgroup and/or seccomp info.
> > For now, we should not need to log namespaces with AUDIT_FEATURE_CHANGE
> > or AUDIT_CONFIG_CHANGE messages since only initial user namespace with
> > initial pid namespace has permission to do so. This will need to be
> > addressed by having non-init config changes be limited to that container
> > or set of namespaces and possibly its children. The other possibility
> > is to add the subject to the stand-alone message.
> >
> > > Also, any user space events originating inside the container needs to have
> > > the container ID added to the user space event - just like auid and
> > > session id.
> >
> > This sounds like every task needs to record a container ID since that
> > information is otherwise unknown by the kernel except by what might be
> > provided by an audit user message such as AUDIT_VIRT_CONTROL or possibly
> > the new AUDIT_NS_INFO request.
>
> Right. The same as we record auid and ses on every event. We'll need a
> container ID logged with everything. -1 for unset, meaning init namespace.
Ok, that might remove the need for the reply I just wrote above.
> > It could be stored in struct task_struct or in struct audit_context. I
> > don't have a suggestion on how to get that information securely into the
> > kernel.
>
> That is where I'd suggest. Its for audit subsystem needs.
struct audit_context would be my choice.
> > > Recording each instance of a name space is giving me something that I
> > > cannot use to do queries required by the security target. Given these
> > > events, how do I locate a web server event where it accesses a watched
> > > file? That authentication failed? That an update within the container
> > > failed?
> > >
> > > The requirements are that we have to log the creation, suspension,
> > > migration, and termination of a container. The requirements are not on
> > > the individual name space.
> >
> > Ok. Do we have a robust definition of a container?
>
> We call the combination of name spaces, cgroups, and seccomp rules a
> container.
Can you detail what information is required from each?
> > Where is that definition managed?
>
> In the thing that invokes a container.
I was looking for a reference to a standards document rather than an
application...
> > If it is a userspace concept, then I think either userspace should be
> > assembling this information, or providing that information to the entity
> > that will be expected to know about and provide it.
>
> Well, uid is a userspace concept, too. But we record an auid and keep it
> immutable so that we can check enforcement of system security policy which is
> also a user space concept. These things need to be collected to a place that
> can be associated with events as needed. That place is the kernel.
I am fine with putting that in the kernel if that is what makes most
sense.
> > > Maybe I'm missing how these events give me that. But I'd like to
> > > hear how I would be able to meet requirements with these 12
> > > events.
> >
> > Adding the infrastructure to give each of those 12 events an audit
> > context to be able to give meaningful subject fields in audit records
> > appears to require adding a struct task_struct argument to calls to
> > copy_mnt_ns(), copy_utsname(), copy_ipcs(), copy_pid_ns(),
> > copy_net_ns(), create_user_ns() unless I use current. I think we must
> > use current since the userns is created before the spawned process is
> > mature or has an audit context in the case of clone.
>
> I think you are heading down the wrong path.
That's why I started questioning it...
> We can tell from syscall flags what is being done. Try this:
>
> ## Optional - log container creation
> -a always,exit -F arch=b32 -S clone -F a0&0x7C020000 -F key=container-create
> -a always,exit -F arch=b64 -S clone -F a0&0x7C020000 -F key=container-create
>
> ## Optional - watch for containers that may change their configuration
> -a always,exit -F arch=b32 -S unshare,setns -F key=container-config
> -a always,exit -F arch=b64 -S unshare,setns -F key=container-config
>
> Then muck with containers, then use ausearch --start recent -k container -i. I
> think you'll see that we know a bit about what's happening. What's needed is
> the breadcrumb trail to tie future events back to the container so that we can
> check for violations of host security policy.
Agreed.
> > Either that, or I have mis-understood and I should be stashing this
> > namespace ID information in an audit_aux_data structure or a more
> > permanent part of struct audit_context to be printed when required on
> > syscall exit. I'm trying to think through if it is needed in any
> > non-syscall audit messages.
>
> I think this is what is required. But we also have the issue where an event's
> meaning can't be determined outside of a container. (For example, login,
> account creation, password change, uid change, file access, etc.) So, I think
> auditing needs to be local to the container for enrichment and ultimately
> forwarded to an aggregating server.
There are some events that will mean more to different layers...
They should be determined by the rules in each auditd jurisdiction,
potentially one per user namespace.
> -Steve
- RGB
--
Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] mmap2: clarify MAP_POPULATE
From: David Rientjes @ 2015-05-15 0:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Michael Kerrisk, Andrew Morton, Linus Torvalds, LKML, Linux API,
linux-mm, Michal Hocko
In-Reply-To: <1431527892-2996-3-git-send-email-miso@dhcp22.suse.cz>
On Wed, 13 May 2015, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.cz>
>
> David Rientjes has noticed that MAP_POPULATE wording might promise much
> more than the kernel actually provides and intend to provide. The
> primary usage of the flag is to pre-fault the range. There is no
> guarantee that no major faults will happen later on. The pages might
> have been reclaimed by the time the process tries to access them.
>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Thanks for following up!
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/
From: Michael Ellerman @ 2015-05-14 22:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Ingo Molnar, Josh Triplett, Borislav Petkov, Jonathan Corbet,
Peter Zijlstra, Andy Lutomirski, Ingo Molnar, H. Peter Anvin,
Thomas Gleixner, Linus Torvalds, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-arch-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150514123835.1bf436bc35d711f8dbf9bc0f-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
On Thu, 2015-05-14 at 12:38 -0700, Andrew Morton wrote:
> > Add arch support matrices for more than 40 generic kernel features
> > that need per architecture support.
> >
> > Each feature has its own directory under Documentation/features/feature_name/,
> > and the arch-support.txt file shows its current arch porting status.
>
> It would be nice to provide people with commit IDs to look at, but the
> IDs won't be known at the time the documentation file is created. We
> could provide patch titles.
+1 on patch titles.
> But still, let's not overdo it - get something in there, see how well
> it works, evolve it over time.
>
> I don't think we've heard from any (non-x86) arch maintainers? Do they
> consider this useful at all? Poke.
Yes it is. I have my own version I've cobbled together for powerpc, but this is
much better.
I'd like to see more description in the individual files of what the feature
is, and preferably some pointers to what's needed to implement it.
The kconfig for HAVE_ARCH_SECCOMP_FILTER is a good example of what I mean.
I realise retrofitting that for all these existing features is quite time
consuming, but for new features hopefully the bar can be raised a little in
terms of that description.
cheers
^ permalink raw reply
* Re: [PATCH v2] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/
From: Stephen Rothwell @ 2015-05-14 22:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andrew Morton, Josh Triplett, Borislav Petkov, Jonathan Corbet,
Peter Zijlstra, Andy Lutomirski, Ingo Molnar, H. Peter Anvin,
Thomas Gleixner, Linus Torvalds, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-arch-u79uwXL29TY76Z2rM5mHXA, Yoshinori Sato
In-Reply-To: <20150514195925.GA27570-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 537 bytes --]
Hi Ingo,
Thanks for this. The concept is certainly good.
On Thu, 14 May 2015 21:59:25 +0200 Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> The patch that adds a new architecture to all these files would give
> us a good overview about how complete an initial port is.
If you want to test how hard that is (and what sort of patch it
produces), the h8300 architecture was added to linux-next recently.
--
Cheers,
Stephen Rothwell sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [CFT][PATCH 08/10] sysfs: Add support for permanently empty directories.
From: Eric W. Biederman @ 2015-05-14 21:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux Containers, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API,
Serge E. Hallyn, Andy Lutomirski, Richard Weinberger,
Kenton Varda, Michael Kerrisk-manpages, Stéphane Graber,
Eric Windisch, Tejun Heo
In-Reply-To: <20150514203131.GB16416-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
> On Thu, May 14, 2015 at 12:35:02PM -0500, Eric W. Biederman wrote:
>>
>> Add two functions sysfs_create_empty_dir and sysfs_remove_empty_dir
>> that hang a permanently empty directory off of a kobject or remove
>> a permanently emptpy directory hanging from a kobject. Export
>> these new functions so modular filesystems can use them.
>>
>> As all permanently empty directories are, are names and used
>> for mouting other filesystems this seems like the right abstraction.
>
> That sentence doesn't make much sense, cut and paste?
Probably one edit too many or too few depending on how you look at it.
What I meant is that since the only interesting thing about a
permanently empty directory is it's name, treating them like sysfs files
rather than normal sysfs directories which require a kobject seems like
the right abstraction.
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> ---
>> fs/sysfs/dir.c | 34 ++++++++++++++++++++++++++++++++++
>> include/linux/sysfs.h | 16 ++++++++++++++++
>> 2 files changed, 50 insertions(+)
>>
>> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>> index 0b45ff42f374..8244741474d7 100644
>> --- a/fs/sysfs/dir.c
>> +++ b/fs/sysfs/dir.c
>> @@ -121,3 +121,37 @@ int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj,
>>
>> return kernfs_rename_ns(kn, new_parent, kn->name, new_ns);
>> }
>> +
>> +/**
>> + * sysfs_create_empty_dir - create an always empty directory
>> + * @parent_kobj: kobject that will contain this always empty directory
>> + * @name: The name of the always empty directory to add
>> + */
>> +int sysfs_create_empty_dir(struct kobject *parent_kobj, const char *name)
>
> As this really is just a mount point, how about we be explicit with
> this and call the function:
> sysfs_create_mount_point()
> sysfs_remove_mount_point()
> That makes more sense in the long run, otherwise if you just want to
> create an empty directory in sysfs, you can do so without making an
> "empty" kobject and some people might do that accidentally in the
> future. This makes it more obvious as to what is going on.
Yeah. That seems fairly reasonable.
My brain is on the edge between the functional description of
creating a permanently empty directory, and the usage based
description (creating a directory to mount filesystems on).
But I agree a name that makes it totally obvious we are creating a
directory to mount something on is going to be more usable and
comprehensible.
My head doesn't like sysfs_create_mount_point() as a mount point can be
a file. But I will put it on the back burner and see if I can come up
with something better, and if not sysfs_create_mount_point it is.
Brainstorming:
sysfs_create_expected_mount_point()
sysfs_reserve_dir_for_mount()
sysfs_create_dir_mount_point()
sysfs_create_expected_mount_point()
Partly I think I would like to rename the proc, sysctl and
infrastructure bit as well (consistency and clarity is good).
Where I get stuck is how do I ask the question:
I see this directory is a mount point, is it a directory whose sole
purpose in life is to be a mount point?
In the context of that question I like my naming of empty_dir as it
conveys what I am interested in.
But I like the sysfs_create_mount_point for general use. Maybe I won't
make my names consistent.
I don't know. I am putting this naming question on the back burner for
a bit.
Eric
^ permalink raw reply
* Re: [CFT][PATCH 0/10] Making new mounts of proc and sysfs as safe as bind mounts
From: Eric W. Biederman @ 2015-05-14 21:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux Containers, linux-fsdevel, Linux API, Serge E. Hallyn,
Andy Lutomirski, Richard Weinberger, Kenton Varda,
Michael Kerrisk-manpages, Stéphane Graber, Eric Windisch,
Tejun Heo
In-Reply-To: <20150514202951.GA16416@kroah.com>
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Thu, May 14, 2015 at 12:30:45PM -0500, Eric W. Biederman wrote:
>>
>> The code is currently available at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-testing
>>
>> HEAD: a524faf520600968e58bbc732063fccf2fdf9199 mnt: Update fs_fully_visible to test for permanently empty directories
>>
>> The problem: Mounting a new instance of proc of sysfs can allow things
>> that a bind mount of those filesystems would not.
>>
>> That is the cases I am dealing with are:
>> unshare --user --net --mount ; mount -t sysfs ...
>> unshare --user --pid --mount ; mount -t proc ...
>>
>> The big change is that this set of changes enforces the preservation of
>> locked mount flags, from the existing mount to the current mount. Which
>> means that if proc was mounted read-only the current current will allow
>> a new instance of proc to be mounted read-write, and this set of changes
>> enforces that proc remain read-only.
>>
>> The other gotcha is that the current code does not properly detect empty
>> directories so to prevent things slipping through the cracks this set of
>> changes annotates all mount points where nothing will be revealed if
>> the filesystem mounted on top is removed.
>>
>> Enforcing the administrators policy can actually matter in the real
>> world as has been shown by the recent docker issue.
>>
>> With this patchset I have two concerns:
>> - The enforcement of mount flag preservation on proc and sysfs may break
>> things. (I am especially worried about the implicit adding of nodev).
>
> What do you mean by this? What got added?
In a user namespace mounting a filesystem implicitly adds nodev.
When I started enforcing not clearing bits that root had set on a
filesystem in mount -o remount the implicit nodev wound up being
an issue that broke userspace for no good reason. The fix was
to implicitly add nodev in remount as well.
Taking a second look at this nodev is implicitly added before the
fs_fully_visible check so even for applications that are know how the
original proc was mounted (and don't see an implicit nodev) and that
don't add nodev (because they ''know'' the mount flags) this change
should not be a problem. Hooray! One less scary thing.
>> - I missed a filesystem mountpoint on proc or sysfs which would make a
>> fresh copy unmountable for no good reason.
>>
>> I don't want to break userspace if I can help it, and the code has been
>> this way for a while so I figure there is time to find any pitfalls and
>> address them before this code gets merged.
>>
>> So if this works for you please give me your Tested-By
>>
>> The well known mountpoints for pseudo filesystems that I could find are:
>> /dev/ffs*/ functionfs
>> /dev/gadget/ gadgetfs
>> /dev/mqueue mqueue
>> /dev/oprofile/ oprofilefs
>> /dev/pts/ devpts
>
> /dev/shm gets a tmpfs, right? Or do those not matter here?
It does, but it doesn't matter in this context. I was looking for
things that mounted themselves on proc or sysfs and I catalogued the
rest just to know they were not mounted there.
>> /dlm/ ocfs2_dlmfs
>> /ipath/ ipathfs
>> /proc/fs/nfsd/ nfsd
>> /proc/openprom/ openpromfs
>> /proc/sys/fs/binfmt_misc/ binfmt_misc
>> /spu/ spufs
>
>> /sys/firmware/efi/efivars/ efivarfs
>> /sys/fs/cgroup/ cgroup
>> /sys/fs/fuse/connections/ fusectl
>
> I thought fuse mounted a few more things in here, but I don't know for
> sure.
There are definitely some fuse attributes under /sys/fs/fuse/ but
I don't see anything else in the code that could be creating a mount
point.
>> /sys/fs/pstore/ pstore
>> /sys/fs/selinux/ selinuxfs
>> /sys/fs/smackfs/ smackfs
>> /sys/hypervisor/s390/ s390_hypfs
>> /sys/kernel/config/ configfs
>> /sys/kernel/debug/ debugfs
>> /sys/kernel/security/ securityfs
>> /sys/kernel/tracing/ tracefs
>
> I think these are all correct for sysfs, I have a minor comment on the
> sysfs patch I'll make in it.
Good to hear and I will answer there as well.
Eric
^ permalink raw reply
* Re: [PATCH 4/6] nohz: support PR_DATAPLANE_QUIESCE
From: Chris Metcalf @ 2015-05-14 20:55 UTC (permalink / raw)
To: paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Andy Lutomirski
Cc: Ingo Molnar, Peter Zijlstra, Gilad Ben Yossef, Steven Rostedt,
Ingo Molnar, Andrew Morton, Rik van Riel, Tejun Heo,
Thomas Gleixner, Frederic Weisbecker, Christoph Lameter,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20150513175150.GL6776-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On 05/12/2015 08:52 AM, Ingo Molnar wrote:
> What I suggested is that it might make sense to offer a system call,
> for example a sched_setparam() variant, that makes such guarantees.
>
> Say if user-space does:
>
> ret = sched_setscheduler(0, BIND_ISOLATED, &isolation_params);
>
> ... then we would get the task moved to an isolated domain and get a 0
> return code if the kernel is able to do all that and if the current
> uid/namespace/etc. has the required permissions and such.
Unfortunately I don't know nearly as much about the scheduler
and scheduler policies as I might, since I mostly focused on
make the scheduler stay out of the way. :-) This does seem like
another way to set a policy bit on a process. I assume you
could only validly issue this call on a nohz_full core, and that
you're not assuming it migrates the cpu to such a core?
You suggested that BIND_ISOLATED would not replace the usual
scheduler policies, but perhaps SCHED_ISOLATED as a full
replacement would make sense - it would make it an error
to have any other schedulable task on that core. I guess that
brings it around to whether the "cpu_isolated" task just loses when
another task is scheduled on the core with it (the current
approach I'm proposing) or if it ends up truly owning the core
and other processes can be denied the right to run there:
which in that case clearly does get us into the area of requiring
privileges to set up, as Andy pointed out later.
This would leave the notion of "strict" as proposed elsewhere
as a separate thing, but presumably it could still be a prctl()
as originally proposed.
I admit I don't know enough to say whether this sounds like
a better approach than just using a prctl() to set the
cpu_isolated state. My instinct is that it's cleanest to avoid
requiring permissions to do this, and to simply enable the
quiescing semantics the process requested when it happens
to be alone on a core. If so, it's somewhat orthogonal to the
actual scheduler policy in force, so best not to conflate it with
the notion of scheduler code at all via sched_setscheduler()?
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
^ permalink raw reply
* Re: [PATCH 4/6] nohz: support PR_DATAPLANE_QUIESCE
From: Chris Metcalf @ 2015-05-14 20:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Gilad Ben Yossef, Steven Rostedt, Ingo Molnar, Andrew Morton,
Rik van Riel, Tejun Heo, Thomas Gleixner, Frederic Weisbecker,
Paul E. McKenney, Christoph Lameter, Viresh Kumar, linux-doc,
linux-api, linux-kernel
In-Reply-To: <20150512093349.GH21418@twins.programming.kicks-ass.net>
On 05/12/2015 05:33 AM, Peter Zijlstra wrote:
> On Fri, May 08, 2015 at 01:58:45PM -0400, Chris Metcalf wrote:
>> This prctl() flag for PR_SET_DATAPLANE sets a mode that requires the
>> kernel to quiesce any pending timer interrupts prior to returning
>> to userspace. When running with this mode set, sys calls (and page
>> faults, etc.) can be inordinately slow. However, user applications
>> that want to guarantee that no unexpected interrupts will occur
>> (even if they call into the kernel) can set this flag to guarantee
>> that semantics.
> Currently people hot-unplug and hot-plug the CPU to do this. Obviously
> that's a wee bit horrible :-)
>
> Not sure if a prctl like this is any better though. This is a CPU
> properly not a process one.
The CPU property aspects, I think, should be largely handled by
fixing kernel bugs that let work end up running on nohz_full cores
without having been explicitly requested to run there.
As you said in a follow-up email:
On 05/12/2015 06:38 AM, Peter Zijlstra wrote:
> Ideally we'd never have to clear the state because it should be
> impossible to get into this predicament in the first place.
What my prctl() proposal does is quiesce things that end up
happening specifically because the user process called on purpose
into the kernel. For example, perhaps RCU was invoked in the
kernel, and the core has to wait a timer tick to quiesce RCU.
Whatever causes it, the intent is that you're not allowed back into
userspace until everything has settled down from your call into
the kernel; the presumption is that it's all due to the kernel entry
that was just made, and not from other stray work.
In that sense, it's very appropriate for it to be a process property.
> ISTR people talking about 'quiesce' sysfs file, along side the hotplug
> stuff, I can't quite remember.
It seems somewhat similar (adding Viresh to the cc's) but does
seem like it might have been more intended to address the
CPU properties rather than process properties:
https://lkml.org/lkml/2014/4/4/99
One thing the original Tilera dataplane code did was to require
setting dataplane flags to succeed only on dataplane cores,
and only when the task had been affinitized to that single core.
This did not protect the task from later being re-affinitized in
a way that broke those assumptions, but I suppose you could
also imagine make sched_setaffinity() fail for such a process.
Somewhat unrelated, but it occurred to me in the context of this
reply, so what do you think? I can certainly add this to the
patch series if it seems like it makes setting the prctl() flags
more conservative.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
^ permalink raw reply
* Re: [PATCH v2] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/
From: Richard Weinberger @ 2015-05-14 20:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Ingo Molnar, Josh Triplett, Borislav Petkov, Jonathan Corbet,
Peter Zijlstra, Andy Lutomirski, Ingo Molnar, H. Peter Anvin,
Thomas Gleixner, Linus Torvalds, open list:ABI/API, LKML,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Linux-Arch
In-Reply-To: <20150514123835.1bf436bc35d711f8dbf9bc0f-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
On Thu, May 14, 2015 at 9:38 PM, Andrew Morton
<akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> I don't think we've heard from any (non-x86) arch maintainers? Do they
> consider this useful at all? Poke.
I like it. :-)
--
Thanks,
//richard
^ permalink raw reply
* Re: [CFT][PATCH 08/10] sysfs: Add support for permanently empty directories.
From: Greg Kroah-Hartman @ 2015-05-14 20:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API,
Serge E. Hallyn, Andy Lutomirski, Richard Weinberger,
Kenton Varda, Michael Kerrisk-manpages, Stéphane Graber,
Eric Windisch, Tejun Heo
In-Reply-To: <87fv6zhxkp.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Thu, May 14, 2015 at 12:35:02PM -0500, Eric W. Biederman wrote:
>
> Add two functions sysfs_create_empty_dir and sysfs_remove_empty_dir
> that hang a permanently empty directory off of a kobject or remove
> a permanently emptpy directory hanging from a kobject. Export
> these new functions so modular filesystems can use them.
>
> As all permanently empty directories are, are names and used
> for mouting other filesystems this seems like the right abstraction.
That sentence doesn't make much sense, cut and paste?
>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
> fs/sysfs/dir.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/sysfs.h | 16 ++++++++++++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 0b45ff42f374..8244741474d7 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -121,3 +121,37 @@ int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj,
>
> return kernfs_rename_ns(kn, new_parent, kn->name, new_ns);
> }
> +
> +/**
> + * sysfs_create_empty_dir - create an always empty directory
> + * @parent_kobj: kobject that will contain this always empty directory
> + * @name: The name of the always empty directory to add
> + */
> +int sysfs_create_empty_dir(struct kobject *parent_kobj, const char *name)
As this really is just a mount point, how about we be explicit with
this and call the function:
sysfs_create_mount_point()
sysfs_remove_mount_point()
That makes more sense in the long run, otherwise if you just want to
create an empty directory in sysfs, you can do so without making an
"empty" kobject and some people might do that accidentally in the
future. This makes it more obvious as to what is going on.
thanks,
greg k-h
^ permalink raw reply
* Re: [CFT][PATCH 0/10] Making new mounts of proc and sysfs as safe as bind mounts
From: Greg Kroah-Hartman @ 2015-05-14 20:29 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, linux-fsdevel, Linux API, Serge E. Hallyn,
Andy Lutomirski, Richard Weinberger, Kenton Varda,
Michael Kerrisk-manpages, Stéphane Graber, Eric Windisch,
Tejun Heo
In-Reply-To: <87pp63jcca.fsf@x220.int.ebiederm.org>
On Thu, May 14, 2015 at 12:30:45PM -0500, Eric W. Biederman wrote:
>
> The code is currently available at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-testing
>
> HEAD: a524faf520600968e58bbc732063fccf2fdf9199 mnt: Update fs_fully_visible to test for permanently empty directories
>
> The problem: Mounting a new instance of proc of sysfs can allow things
> that a bind mount of those filesystems would not.
>
> That is the cases I am dealing with are:
> unshare --user --net --mount ; mount -t sysfs ...
> unshare --user --pid --mount ; mount -t proc ...
>
> The big change is that this set of changes enforces the preservation of
> locked mount flags, from the existing mount to the current mount. Which
> means that if proc was mounted read-only the current current will allow
> a new instance of proc to be mounted read-write, and this set of changes
> enforces that proc remain read-only.
>
> The other gotcha is that the current code does not properly detect empty
> directories so to prevent things slipping through the cracks this set of
> changes annotates all mount points where nothing will be revealed if
> the filesystem mounted on top is removed.
>
> Enforcing the administrators policy can actually matter in the real
> world as has been shown by the recent docker issue.
>
> With this patchset I have two concerns:
> - The enforcement of mount flag preservation on proc and sysfs may break
> things. (I am especially worried about the implicit adding of nodev).
What do you mean by this? What got added?
> - I missed a filesystem mountpoint on proc or sysfs which would make a
> fresh copy unmountable for no good reason.
>
> I don't want to break userspace if I can help it, and the code has been
> this way for a while so I figure there is time to find any pitfalls and
> address them before this code gets merged.
>
> So if this works for you please give me your Tested-By
>
> The well known mountpoints for pseudo filesystems that I could find are:
> /dev/ffs*/ functionfs
> /dev/gadget/ gadgetfs
> /dev/mqueue mqueue
> /dev/oprofile/ oprofilefs
> /dev/pts/ devpts
/dev/shm gets a tmpfs, right? Or do those not matter here?
> /dlm/ ocfs2_dlmfs
> /ipath/ ipathfs
> /proc/fs/nfsd/ nfsd
> /proc/openprom/ openpromfs
> /proc/sys/fs/binfmt_misc/ binfmt_misc
> /spu/ spufs
> /sys/firmware/efi/efivars/ efivarfs
> /sys/fs/cgroup/ cgroup
> /sys/fs/fuse/connections/ fusectl
I thought fuse mounted a few more things in here, but I don't know for
sure.
> /sys/fs/pstore/ pstore
> /sys/fs/selinux/ selinuxfs
> /sys/fs/smackfs/ smackfs
> /sys/hypervisor/s390/ s390_hypfs
> /sys/kernel/config/ configfs
> /sys/kernel/debug/ debugfs
> /sys/kernel/security/ securityfs
> /sys/kernel/tracing/ tracefs
I think these are all correct for sysfs, I have a minor comment on the
sysfs patch I'll make in it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/
From: Ingo Molnar @ 2015-05-14 19:59 UTC (permalink / raw)
To: Andrew Morton
Cc: Josh Triplett, Borislav Petkov, Jonathan Corbet, Peter Zijlstra,
Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Thomas Gleixner,
Linus Torvalds, linux-api, linux-kernel, x86, linux-arch
In-Reply-To: <20150514123835.1bf436bc35d711f8dbf9bc0f@linux-foundation.org>
* Andrew Morton <akpm@linux-foundation.org> wrote:
> > Add arch support matrices for more than 40 generic kernel features
> > that need per architecture support.
> >
> > Each feature has its own directory under Documentation/features/feature_name/,
> > and the arch-support.txt file shows its current arch porting status.
> >
> > For example, lockdep support is shown the following way:
> >
> > triton:~/tip> cat Documentation/features/lockdep/arch-support.txt
> > #
> > # Feature name: lockdep
> > # Kconfig: LOCKDEP_SUPPORT
> > # description: arch supports the runtime locking correctness debug facility
> > #
> > -----------------------
> > | arch |status|
> > -----------------------
> > ...
> > | xtensa: | ok |
> > -----------------------
> >
> > For generic kernel features that need architecture support, the
> > arch-support.txt file in each feature directory shows the arch
> > support matrix, for all upstream Linux architectures.
> >
> > The meaning of entries in the tables is:
> >
> > | ok | # feature supported by the architecture
> > |TODO| # feature not yet supported by the architecture
> > | .. | # feature cannot be supported by the hardware
>
> Presumably there will be instances where the maintainer decides "we
> shall not implement that".
So I tried to limit the list of features to those that represent the
overall progress of the generic kernel and are recommended on all
architectures that are able to support it.
There are certainly features that are opt-in. We could still list them
here as well, because this is a convenient central index, but would
mark them not with 'TODO' but with a low-key '..' marking?
On the other end of the spectrum we could also define 'must have'
features for new architectures. For example modern-timekeeping and
clockevents.
The patch that adds a new architecture to all these files would give
us a good overview about how complete an initial port is.
> > This directory structure can be used in the future to add other
> > files - such as porting guides, testing description, etc.
>
> I suppose so. Having a great bunch of directories, each containing
> a single file is a bit odd.
It's a starting point and nicely extensible. I was thinking about one
more intermediate level:
Documentation/features/locking/lockdep/
Documentation/features/locking/rwsem-optimized/
Documentation/features/locking/queued-rwlocks/
Documentation/features/locking/queued-spinlocks/
...
Documentation/features/vm/PG_uncached/
Documentation/features/vm/pmdp_splitting_flush/
Documentation/features/vm/pte_special/
...
The advantage of this, beyond more structure, would be that I'd
probably move most of the Documentation/locking/*.txt files into
Documentation/features/locking/, for example lockdep-design.txt would
go into Documentation/features/locking/lockdep/.
I'd keep the hierarchy at a predictable depth though, i.e.:
Documentation/features/<subsystem>/<feature_name>/
> It would be nice to provide people with commit IDs to look at, but
> the IDs won't be known at the time the documentation file is
> created. We could provide patch titles.
Ok.
> But still, let's not overdo it - get something in there, see how
> well it works, evolve it over time.
Yeah.
> I don't think we've heard from any (non-x86) arch maintainers? Do
> they consider this useful at all? Poke.
I think we god some feedback from the PowerPC side, but yeah, would be
nice to get more reactions.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v2] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/
From: Andrew Morton @ 2015-05-14 19:38 UTC (permalink / raw)
To: Ingo Molnar
Cc: Josh Triplett, Borislav Petkov, Jonathan Corbet, Peter Zijlstra,
Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Thomas Gleixner,
Linus Torvalds, linux-api, linux-kernel, x86, linux-arch
In-Reply-To: <20150514103544.GA7027@gmail.com>
> Add arch support matrices for more than 40 generic kernel features
> that need per architecture support.
>
> Each feature has its own directory under Documentation/features/feature_name/,
> and the arch-support.txt file shows its current arch porting status.
>
> For example, lockdep support is shown the following way:
>
> triton:~/tip> cat Documentation/features/lockdep/arch-support.txt
> #
> # Feature name: lockdep
> # Kconfig: LOCKDEP_SUPPORT
> # description: arch supports the runtime locking correctness debug facility
> #
> -----------------------
> | arch |status|
> -----------------------
> ...
> | xtensa: | ok |
> -----------------------
>
> For generic kernel features that need architecture support, the
> arch-support.txt file in each feature directory shows the arch
> support matrix, for all upstream Linux architectures.
>
> The meaning of entries in the tables is:
>
> | ok | # feature supported by the architecture
> |TODO| # feature not yet supported by the architecture
> | .. | # feature cannot be supported by the hardware
Presumably there will be instances where the maintainer decides "we
shall not implement that".
> This directory structure can be used in the future to add other
> files - such as porting guides, testing description, etc.
I suppose so. Having a great bunch of directories, each containing a
single file is a bit odd.
It would be nice to provide people with commit IDs to look at, but the
IDs won't be known at the time the documentation file is created. We
could provide patch titles.
But still, let's not overdo it - get something in there, see how well
it works, evolve it over time.
I don't think we've heard from any (non-x86) arch maintainers? Do they
consider this useful at all? Poke.
^ 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