* Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile
From: Catalin Marinas @ 2014-11-28 17:24 UTC (permalink / raw)
To: Mark Rutland
Cc: andrew-g2DYL2Zd6BY@public.gmane.org,
heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org,
Will Deacon, Chunyan Zhang,
jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org,
lanqing.liu-lxIno14LUO0EEoCn2XhGlw@public.gmane.org,
arnd-r2nGTMty4D4@public.gmane.org,
corbet-T1hC0tSOHrs@public.gmane.org,
zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org,
geng.ren-lxIno14LUO0EEoCn2XhGlw@public.gmane.org,
m-karicheri2-l0cyMroinI0@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
"florian.vaussard-p8DiymsW2f8@public.gmane.org" <florian.vaussard>
In-Reply-To: <20141128164047.GL25883@leverpostej>
On Fri, Nov 28, 2014 at 04:40:47PM +0000, Mark Rutland wrote:
> [...]
>
> > In the meantime I think we can be more tolerant:
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index 2133f9d59d06..87f67a93fcc7 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > else
> > arch_timer_rate = arch_timer_get_cntfrq();
> > + } else if (IS_ENABLED(CONFIG_ARM64)) {
> > + pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
> > }
>
> That looks good to me. On a related note, it looks like my documentation
> patch [1] from a while back fell by the wayside. Does anyone fancy
> picking that up to go with this?
I can put the two together (keeping you as author). Do we still need the
IS_ENABLED(CONFIG_ARM64) part here or we extend the warning to to arm32
as well?
--
Catalin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RFC PATCH] userns: Disallow setgroups unless the gid_map writer is privileged
From: Andy Lutomirski @ 2014-11-28 22:53 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook,
Michael Kerrisk-manpages, Linux API, linux-man,
linux-kernel@vger.kernel.org, LSM, Casey Schaufler,
Serge E. Hallyn, Richard Weinberger, kenton, Andy Lutomirski,
stable
Classic unix permission checks have an interesting feature. The
group permissions for a file can be set to less than the other
permissions on a file. Occasionally this is used deliberately to
give a certain group of users fewer permissions than the default.
User namespaces break this usage. Groups set in rgid or egid are
unaffected because an unprivileged user namespace creator can only
map a single group, so setresgid inside and outside the namespace
have the same effect. However, an unprivileged user namespace
creator can currently use setgroups(2) to drop all supplementary
groups, so, if a supplementary group denies access to some resource,
user namespaces can be used to bypass that restriction.
To fix this issue without unduly breaking existing user code, this
introduces a new user namespace flag USERNS_SETGROUPS_ALLOWED. If
that flag is not set, then setgroups(2) will fail regardless of the
caller's capabilities.
USERNS_SETGROUPS_ALLOWED is cleared in a new user namespace. By
default, if the writer of gid_map has CAP_SETGID in the parent
userns and the parent userns has USERNS_SETGROUPS_ALLOWED, then the
USERNS_SETGROUPS_ALLOWED will be set in the child.
While it could be safe to set USERNS_SETGROUPS_ALLOWED if the user
namespace creator has no supplementary groups, doing so could be
surprising and could have unpleasant interactions with setns(2).
The default behavior can be overridden by adding a line of the form
"setgroups allow" or "setgroups deny" to gid_map. The former will
return -EPERM if the caller is not permitted to set
USERNS_SETGROUPS_ALLOWED, and the latter will clear
USERNS_SETGROUPS_ALLOWED even if the flag would otherwise be set.
This change should break userspace by the minimal amount needed to
fix this issue. Any application that uses newgidmap(1) should be
unaffected by this fix, and unprivileged users that create user
namespaces to manipulate mounts or sandbox themselves will also be
unaffected unless they try to use setgroups(2).
This should fix CVE-2014-8989.
Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
Eric, this is an alternative to your patch. I think it will cause
less breakage, and it will keep unprivileged user namespaces
more or less fully functional.
Kenton, I think that neither run-bundle nor supervisor-main will be
broken by this patch.
include/linux/user_namespace.h | 3 +++
kernel/groups.c | 3 +++
kernel/user.c | 1 +
kernel/user_namespace.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 43 insertions(+)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e95372654f09..a74c1f3d44fe 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -17,6 +17,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
} extent[UID_GID_MAP_MAX_EXTENTS];
};
+#define USERNS_SETGROUPS_ALLOWED 1
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -27,6 +29,7 @@ struct user_namespace {
kuid_t owner;
kgid_t group;
unsigned int proc_inum;
+ unsigned int flags;
/* Register of per-UID persistent keyrings for this namespace */
#ifdef CONFIG_PERSISTENT_KEYRINGS
diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f86cfa..e27433809978 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -6,6 +6,7 @@
#include <linux/slab.h>
#include <linux/security.h>
#include <linux/syscalls.h>
+#include <linux/user_namespace.h>
#include <asm/uaccess.h>
/* init to 2 - one for init_task, one to ensure it is never freed */
@@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
struct group_info *group_info;
int retval;
+ if (!(current_user_ns()->flags & USERNS_SETGROUPS_ALLOWED))
+ return -EPERM;
if (!ns_capable(current_user_ns(), CAP_SETGID))
return -EPERM;
if ((unsigned)gidsetsize > NGROUPS_MAX)
diff --git a/kernel/user.c b/kernel/user.c
index 4efa39350e44..f8cdb1ec6049 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
.owner = GLOBAL_ROOT_UID,
.group = GLOBAL_ROOT_GID,
.proc_inum = PROC_USER_INIT_INO,
+ .flags = USERNS_SETGROUPS_ALLOWED,
#ifdef CONFIG_PERSISTENT_KEYRINGS
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..6e7b9ee5bddc 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -600,6 +600,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
unsigned long page = 0;
char *kbuf, *pos, *next_line;
ssize_t ret = -EINVAL;
+ unsigned int gid_flags = 0;
+ bool seen_explicit_gid_flag = false;
/*
* The id_map_mutex serializes all writes to any given map.
@@ -633,6 +635,19 @@ static ssize_t map_write(struct file *file, const char __user *buf,
if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
goto out;
+ /* Deal with supplementary groups. */
+ if (map == &ns->gid_map) {
+ /*
+ * By default, setgroups is allowed inside the userns
+ * if the writer has no supplementary groups (making
+ * it useless) or if the writer is privileged.
+ */
+ if ((ns->parent->flags & USERNS_SETGROUPS_ALLOWED) &&
+ file_ns_capable(file, ns->parent, CAP_SETGID) &&
+ ns_capable(ns->parent, CAP_SETGID))
+ gid_flags = USERNS_SETGROUPS_ALLOWED;
+ }
+
/* Get a buffer */
ret = -ENOMEM;
page = __get_free_page(GFP_TEMPORARY);
@@ -667,6 +682,25 @@ static ssize_t map_write(struct file *file, const char __user *buf,
next_line = NULL;
}
+ /* Is this line a gid_map option? */
+ if (map == &ns->gid_map) {
+ if (!strcmp(pos, "setgroups deny")) {
+ if (seen_explicit_gid_flag)
+ goto out;
+ seen_explicit_gid_flag = 1;
+ gid_flags = 0;
+ continue;
+ } else if (!strcmp(pos, "setgroups allow")) {
+ if (seen_explicit_gid_flag)
+ goto out;
+ if (!(gid_flags & USERNS_SETGROUPS_ALLOWED)) {
+ ret = -EPERM;
+ goto out;
+ }
+ continue;
+ }
+ }
+
pos = skip_spaces(pos);
extent->first = simple_strtoul(pos, &pos, 10);
if (!isspace(*pos))
@@ -746,6 +780,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
new_map.nr_extents*sizeof(new_map.extent[0]));
smp_wmb();
map->nr_extents = new_map.nr_extents;
+ if (map == &ns->gid_map)
+ ns->flags |= gid_flags;
*ppos = count;
ret = count;
--
1.9.3
^ permalink raw reply related
* [RFC PATCH] proc, pidns: Add highpid
From: Andy Lutomirski @ 2014-11-28 23:05 UTC (permalink / raw)
To: David Herrmann, linux-api, linux-kernel, Eric W. Biederman
Cc: Andrew Morton, systemd Mailing List, Andy Lutomirski
Pid reuse is common, which means that it's difficult or impossible
to read information about a pid from /proc without races.
This introduces a second number associated with each (task, pidns)
pair called highpid. Highpid is a 64-bit number, and, barring
extremely unlikely circumstances or outright error, a (highpid, pid)
will never be reused.
With just this change, a program can open /proc/PID/status, read the
"Highpid" field, and confirm that it has the expected value. If the
pid has been reused, then highpid will be different.
The initial implementation is straightforward: highpid is simply a
64-bit counter. If a high-end system can fork every 3 ns (which
would be amazing, given that just allocating a pid requires at
atomic operation), it would take well over 1000 years for highpid to
wrap.
For CRIU's benefit, the next highpid can be set by a privileged
user.
NB: The sysctl stuff only works on 64-bit systems. If the approach
looks good, I'll fix that somehow.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
If this goes in, there's plenty of room to add new interfaces to
make this more useful. For example, we could add a fancier tgkill
that adds and validates hightgid and highpid, and we might want to
add a syscall to read one's own hightgid and highpid. These would
be quite useful for pidfiles.
David, would this be useful for kdbus?
CRIU people: will this be unduly difficult to support in CRIU?
If you all think this is a good idea, I'll fix the sysctl stuff and
document it. It might also be worth adding "Hightgid" to status.
fs/proc/array.c | 5 ++++-
include/linux/pid.h | 2 ++
include/linux/pid_namespace.h | 1 +
kernel/pid.c | 19 +++++++++++++++----
kernel/pid_namespace.c | 22 ++++++++++++++++++++++
5 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index cd3653e4f35c..f1e0e69d19f9 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -159,6 +159,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
int g;
struct fdtable *fdt = NULL;
const struct cred *cred;
+ const struct upid *upid;
pid_t ppid, tpid;
rcu_read_lock();
@@ -170,12 +171,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
if (tracer)
tpid = task_pid_nr_ns(tracer, ns);
}
+ upid = pid_upid_ns(pid, ns);
cred = get_task_cred(p);
seq_printf(m,
"State:\t%s\n"
"Tgid:\t%d\n"
"Ngid:\t%d\n"
"Pid:\t%d\n"
+ "Highpid:\t%llu\n"
"PPid:\t%d\n"
"TracerPid:\t%d\n"
"Uid:\t%d\t%d\t%d\t%d\n"
@@ -183,7 +186,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
get_task_state(p),
task_tgid_nr_ns(p, ns),
task_numa_group_id(p),
- pid_nr_ns(pid, ns),
+ upid ? upid->nr : 0, upid ? upid->highnr : 0,
ppid, tpid,
from_kuid_munged(user_ns, cred->uid),
from_kuid_munged(user_ns, cred->euid),
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 23705a53abba..ece70b64d04c 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -51,6 +51,7 @@ struct upid {
/* Try to keep pid_chain in the same cacheline as nr for find_vpid */
int nr;
struct pid_namespace *ns;
+ u64 highnr;
struct hlist_node pid_chain;
};
@@ -170,6 +171,7 @@ static inline pid_t pid_nr(struct pid *pid)
}
pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns);
+const struct upid *pid_upid_ns(struct pid *pid, struct pid_namespace *ns);
pid_t pid_vnr(struct pid *pid);
#define do_each_pid_task(pid, type, task) \
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 1997ffc295a7..1f9f0455d7ef 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -26,6 +26,7 @@ struct pid_namespace {
struct rcu_head rcu;
int last_pid;
unsigned int nr_hashed;
+ atomic64_t next_highpid;
struct task_struct *child_reaper;
struct kmem_cache *pid_cachep;
unsigned int level;
diff --git a/kernel/pid.c b/kernel/pid.c
index 9b9a26698144..863d11a9bfbf 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -312,6 +312,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
pid->numbers[i].nr = nr;
pid->numbers[i].ns = tmp;
+ pid->numbers[i].highnr =
+ atomic64_inc_return(&tmp->next_highpid) - 1;
tmp = tmp->parent;
}
@@ -492,17 +494,26 @@ struct pid *find_get_pid(pid_t nr)
}
EXPORT_SYMBOL_GPL(find_get_pid);
-pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns)
+const struct upid *pid_upid_ns(struct pid *pid, struct pid_namespace *ns)
{
struct upid *upid;
- pid_t nr = 0;
if (pid && ns->level <= pid->level) {
upid = &pid->numbers[ns->level];
if (upid->ns == ns)
- nr = upid->nr;
+ return upid;
}
- return nr;
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(pid_upid_ns);
+
+pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns)
+{
+ const struct upid *upid = pid_upid_ns(pid, ns);
+
+ if (!upid)
+ return 0;
+ return upid->nr;
}
EXPORT_SYMBOL_GPL(pid_nr_ns);
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index db95d8eb761b..e246802b4361 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -268,6 +268,22 @@ static int pid_ns_ctl_handler(struct ctl_table *table, int write,
return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
}
+static int pid_ns_next_highpid_handler(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct pid_namespace *pid_ns = task_active_pid_ns(current);
+ struct ctl_table tmp = *table;
+
+ if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
+ return -EPERM;
+
+ /* This needs to be fixed. */
+ BUILD_BUG_ON(sizeof(u64) != sizeof(unsigned long));
+
+ tmp.data = &pid_ns->next_highpid;
+ return proc_dointvec(&tmp, write, buffer, lenp, ppos);
+}
+
extern int pid_max;
static int zero = 0;
static struct ctl_table pid_ns_ctl_table[] = {
@@ -279,6 +295,12 @@ static struct ctl_table pid_ns_ctl_table[] = {
.extra1 = &zero,
.extra2 = &pid_max,
},
+ {
+ .procname = "ns_next_highpid",
+ .maxlen = sizeof(u64),
+ .mode = 0666, /* permissions are checked in the handler */
+ .proc_handler = pid_ns_next_highpid_handler,
+ },
{ }
};
static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
--
1.9.3
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
^ permalink raw reply related
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Andy Lutomirski @ 2014-11-28 23:06 UTC (permalink / raw)
To: David Herrmann, Linux API, linux-kernel@vger.kernel.org,
Eric W. Biederman, criu, Pavel Emelyanov, Cyrill Gorcunov
Cc: Andrew Morton, systemd Mailing List, Andy Lutomirski
In-Reply-To: <b0f6c4df0e8ef8afcc7b786edecb4be8c752941e.1417215468.git.luto@amacapital.net>
[Adding CRIU people. Whoops.]
On Fri, Nov 28, 2014 at 3:05 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> Pid reuse is common, which means that it's difficult or impossible
> to read information about a pid from /proc without races.
>
> This introduces a second number associated with each (task, pidns)
> pair called highpid. Highpid is a 64-bit number, and, barring
> extremely unlikely circumstances or outright error, a (highpid, pid)
> will never be reused.
>
> With just this change, a program can open /proc/PID/status, read the
> "Highpid" field, and confirm that it has the expected value. If the
> pid has been reused, then highpid will be different.
>
> The initial implementation is straightforward: highpid is simply a
> 64-bit counter. If a high-end system can fork every 3 ns (which
> would be amazing, given that just allocating a pid requires at
> atomic operation), it would take well over 1000 years for highpid to
> wrap.
>
> For CRIU's benefit, the next highpid can be set by a privileged
> user.
>
> NB: The sysctl stuff only works on 64-bit systems. If the approach
> looks good, I'll fix that somehow.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>
> If this goes in, there's plenty of room to add new interfaces to
> make this more useful. For example, we could add a fancier tgkill
> that adds and validates hightgid and highpid, and we might want to
> add a syscall to read one's own hightgid and highpid. These would
> be quite useful for pidfiles.
>
> David, would this be useful for kdbus?
>
> CRIU people: will this be unduly difficult to support in CRIU?
>
> If you all think this is a good idea, I'll fix the sysctl stuff and
> document it. It might also be worth adding "Hightgid" to status.
>
> fs/proc/array.c | 5 ++++-
> include/linux/pid.h | 2 ++
> include/linux/pid_namespace.h | 1 +
> kernel/pid.c | 19 +++++++++++++++----
> kernel/pid_namespace.c | 22 ++++++++++++++++++++++
> 5 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index cd3653e4f35c..f1e0e69d19f9 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -159,6 +159,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> int g;
> struct fdtable *fdt = NULL;
> const struct cred *cred;
> + const struct upid *upid;
> pid_t ppid, tpid;
>
> rcu_read_lock();
> @@ -170,12 +171,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> if (tracer)
> tpid = task_pid_nr_ns(tracer, ns);
> }
> + upid = pid_upid_ns(pid, ns);
> cred = get_task_cred(p);
> seq_printf(m,
> "State:\t%s\n"
> "Tgid:\t%d\n"
> "Ngid:\t%d\n"
> "Pid:\t%d\n"
> + "Highpid:\t%llu\n"
> "PPid:\t%d\n"
> "TracerPid:\t%d\n"
> "Uid:\t%d\t%d\t%d\t%d\n"
> @@ -183,7 +186,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> get_task_state(p),
> task_tgid_nr_ns(p, ns),
> task_numa_group_id(p),
> - pid_nr_ns(pid, ns),
> + upid ? upid->nr : 0, upid ? upid->highnr : 0,
> ppid, tpid,
> from_kuid_munged(user_ns, cred->uid),
> from_kuid_munged(user_ns, cred->euid),
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index 23705a53abba..ece70b64d04c 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -51,6 +51,7 @@ struct upid {
> /* Try to keep pid_chain in the same cacheline as nr for find_vpid */
> int nr;
> struct pid_namespace *ns;
> + u64 highnr;
> struct hlist_node pid_chain;
> };
>
> @@ -170,6 +171,7 @@ static inline pid_t pid_nr(struct pid *pid)
> }
>
> pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns);
> +const struct upid *pid_upid_ns(struct pid *pid, struct pid_namespace *ns);
> pid_t pid_vnr(struct pid *pid);
>
> #define do_each_pid_task(pid, type, task) \
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 1997ffc295a7..1f9f0455d7ef 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -26,6 +26,7 @@ struct pid_namespace {
> struct rcu_head rcu;
> int last_pid;
> unsigned int nr_hashed;
> + atomic64_t next_highpid;
> struct task_struct *child_reaper;
> struct kmem_cache *pid_cachep;
> unsigned int level;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 9b9a26698144..863d11a9bfbf 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -312,6 +312,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>
> pid->numbers[i].nr = nr;
> pid->numbers[i].ns = tmp;
> + pid->numbers[i].highnr =
> + atomic64_inc_return(&tmp->next_highpid) - 1;
> tmp = tmp->parent;
> }
>
> @@ -492,17 +494,26 @@ struct pid *find_get_pid(pid_t nr)
> }
> EXPORT_SYMBOL_GPL(find_get_pid);
>
> -pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns)
> +const struct upid *pid_upid_ns(struct pid *pid, struct pid_namespace *ns)
> {
> struct upid *upid;
> - pid_t nr = 0;
>
> if (pid && ns->level <= pid->level) {
> upid = &pid->numbers[ns->level];
> if (upid->ns == ns)
> - nr = upid->nr;
> + return upid;
> }
> - return nr;
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(pid_upid_ns);
> +
> +pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns)
> +{
> + const struct upid *upid = pid_upid_ns(pid, ns);
> +
> + if (!upid)
> + return 0;
> + return upid->nr;
> }
> EXPORT_SYMBOL_GPL(pid_nr_ns);
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index db95d8eb761b..e246802b4361 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -268,6 +268,22 @@ static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> }
>
> +static int pid_ns_next_highpid_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + struct pid_namespace *pid_ns = task_active_pid_ns(current);
> + struct ctl_table tmp = *table;
> +
> + if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /* This needs to be fixed. */
> + BUILD_BUG_ON(sizeof(u64) != sizeof(unsigned long));
> +
> + tmp.data = &pid_ns->next_highpid;
> + return proc_dointvec(&tmp, write, buffer, lenp, ppos);
> +}
> +
> extern int pid_max;
> static int zero = 0;
> static struct ctl_table pid_ns_ctl_table[] = {
> @@ -279,6 +295,12 @@ static struct ctl_table pid_ns_ctl_table[] = {
> .extra1 = &zero,
> .extra2 = &pid_max,
> },
> + {
> + .procname = "ns_next_highpid",
> + .maxlen = sizeof(u64),
> + .mode = 0666, /* permissions are checked in the handler */
> + .proc_handler = pid_ns_next_highpid_handler,
> + },
> { }
> };
> static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
> --
> 1.9.3
>
--
Andy Lutomirski
AMA Capital Management, LLC
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
^ permalink raw reply
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Eric W. Biederman @ 2014-11-29 3:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Herrmann, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton,
systemd Mailing List, criu-GEFAQzZX7r8dnm+yROfE0A,
Pavel Emelyanov, Cyrill Gorcunov
In-Reply-To: <b0f6c4df0e8ef8afcc7b786edecb4be8c752941e.1417215468.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
> Pid reuse is common, which means that it's difficult or impossible
> to read information about a pid from /proc without races.
Sigh.
What we need are not race free pids, but a file descriptor based process
management api. Possibly one that starts by handing you a proc
directory.
Which probably means that we need a proc file we can write to and send
signals to a process, and another proc file we can select on and wait
for the process to exit.
Making pids bigger just looks like bandaid.
Remember evovle things in the direction of an object capability system
things wind up being more maintainable.
Eric
^ permalink raw reply
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Greg KH @ 2014-11-29 4:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Herrmann, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
Andrew Morton, systemd Mailing List
In-Reply-To: <b0f6c4df0e8ef8afcc7b786edecb4be8c752941e.1417215468.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
On Fri, Nov 28, 2014 at 03:05:01PM -0800, Andy Lutomirski wrote:
> Pid reuse is common, which means that it's difficult or impossible
> to read information about a pid from /proc without races.
>
> This introduces a second number associated with each (task, pidns)
> pair called highpid. Highpid is a 64-bit number, and, barring
> extremely unlikely circumstances or outright error, a (highpid, pid)
> will never be reused.
>
> With just this change, a program can open /proc/PID/status, read the
> "Highpid" field, and confirm that it has the expected value. If the
> pid has been reused, then highpid will be different.
>
> The initial implementation is straightforward: highpid is simply a
> 64-bit counter. If a high-end system can fork every 3 ns (which
> would be amazing, given that just allocating a pid requires at
> atomic operation), it would take well over 1000 years for highpid to
> wrap.
>
> For CRIU's benefit, the next highpid can be set by a privileged
> user.
>
> NB: The sysctl stuff only works on 64-bit systems. If the approach
> looks good, I'll fix that somehow.
>
> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> ---
>
> If this goes in, there's plenty of room to add new interfaces to
> make this more useful. For example, we could add a fancier tgkill
> that adds and validates hightgid and highpid, and we might want to
> add a syscall to read one's own hightgid and highpid. These would
> be quite useful for pidfiles.
>
> David, would this be useful for kdbus?
>
> CRIU people: will this be unduly difficult to support in CRIU?
>
> If you all think this is a good idea, I'll fix the sysctl stuff and
> document it. It might also be worth adding "Hightgid" to status.
>
> fs/proc/array.c | 5 ++++-
> include/linux/pid.h | 2 ++
> include/linux/pid_namespace.h | 1 +
> kernel/pid.c | 19 +++++++++++++++----
> kernel/pid_namespace.c | 22 ++++++++++++++++++++++
> 5 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index cd3653e4f35c..f1e0e69d19f9 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -159,6 +159,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> int g;
> struct fdtable *fdt = NULL;
> const struct cred *cred;
> + const struct upid *upid;
> pid_t ppid, tpid;
>
> rcu_read_lock();
> @@ -170,12 +171,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> if (tracer)
> tpid = task_pid_nr_ns(tracer, ns);
> }
> + upid = pid_upid_ns(pid, ns);
> cred = get_task_cred(p);
> seq_printf(m,
> "State:\t%s\n"
> "Tgid:\t%d\n"
> "Ngid:\t%d\n"
> "Pid:\t%d\n"
> + "Highpid:\t%llu\n"
> "PPid:\t%d\n"
> "TracerPid:\t%d\n"
> "Uid:\t%d\t%d\t%d\t%d\n"
Changing existing proc files like this is dangerous and can cause lots
of breakage in userspace programs if you are not careful. Usually
adding fields to the end of the file is best, but sometimes even then
things can break :(
^ permalink raw reply
* Charity Work
From: luv2charitys-Re5JQEeQqe8AvxtiuMwx3w @ 2014-11-29 4:54 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA
Hello,this is Mr Paul N,i sent you an email on charity work but i am yet to hear fom you,do reply with this code CHA-2015 to my email address paulcharity-9uewiaClKEY@public.gmane.org i Look forward to hearing from you this time,God bless Brother Paul
^ permalink raw reply
* Why not make kdbus use CUSE?
From: Richard Yao @ 2014-11-29 6:34 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Greg Kroah-Hartman, linux-api-u79uwXL29TY76Z2rM5mHXA
I had the opportunity at LinuxCon Europe to chat with Greg and some other kdbus
developers. A few things stood out from our conversation that I thought I would
bring to the list for discussion.
The first is that I asked them why we need to add yet another IPC mechanism (and
quite possibly another exploit target) to the kernel. Apparently, they want to
use dbus to do multicast and the existing dbus software is not peformant enough.
There was not much discussion of why the existing network stack is not usable
for this, but I was not terribly concerned about it, so the remainder of our
discussion focused on compatibility.
They regard a userland compatibility shim in the systemd repostory to provide
backward compatibility for applications. Unfortunately, this is insufficient to
ensure compatibility because dependency trees have multiple levels. If cross
platform package A depends on cross platform library B, which depends on dbus,
and cross platform library B decides to switch to kdbus, then it ceases to be
cross platform and cross platform package A is now dependent on Linux kernels
with kdbus. Not only does that affect other POSIX systems, but it also affects
LTS versions of Linux.
It is somewhat tempting to think that being in the kernel is necessary for
performance, this does not appear to be true from my discussion with Greg and
others. In specific, a key advantage of being in the kernel is a reduction in
context switches and consequently, one would expect programs using the old API
to benefit, but they were quite clear to me that programs using the old API do
not benefit. At the same time, we had a similar situation where people thought
that the httpd server had to be inside the kernel until Linux 2.6, when our
userland APIs improved to the point where we were able to get similar if not
better performance in userland compared to the implementation of khttpd in Linux
2.4.y.
Putting daemons in the kernel is always more performant than putting daemons
into userland, but it has the drawback of violating the principle of least
privilege. When code is in userland, we can apply security mechanisms to it via
things like SELinux and seccomp to limit the damage caused by compromise. With
an in-kernel component, there is no way of doing that. One might be tempted to
think that controlling the IPC mechanism is as good as controlling the system,
but this is not true when we consider things like lxc, where compromise of dbus
in a container does not give full control over the system.
I started to think that we probably ought to design a way to put kdbus into
userland and then I realized that we already have one in the form of CUSE. This
would not only makes kdbus play nicely with SELinux and lxc, but also other
POSIX systems that currently share dbus with Linux systems, which includes older
Linux kernels. Greg claimed that the kdbus code was fairly self contained and
was just a character device, so I assume this is possible and I am curious why
it is not done.
I should probably mention one other thing that I recall from my discussion with
Greg and others, which is that the systemd project wants to depend on it. The
nature of controlling pid 1 means that systemd is more than capable of starting
dbus before anything that needs it and that includes its own components (aside
from its pid 1). The systemd project wanting the API is not a valid reason for
why it should be in the kernel, although it could be a reason to make a CUSE
version go into systemd's pid 1.
That said, why not make kdbus use CUSE?
P.S. I also mentioned my concern that having the shim in the systemd repository
would have a negative effect on distributons that use alterntaive libc libraries
because the systemd developers refuse to support alternative libc libraries. I
mentioned this to one of the people to whom Greg introduced me (and whose name
escapes me) as we were walking to Michael Kerrisk's talk on API design. I was
told quite plainly that such distributions are not worth consideration. If kdbus
is merged despite concerns about security and backward compatibility, could we
at least have the shim moved to libc netural place, like Linus' tree?
^ permalink raw reply
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Andy Lutomirski @ 2014-11-29 15:19 UTC (permalink / raw)
To: Greg KH
Cc: systemd Mailing List, Linux API, linux-kernel@vger.kernel.org,
Eric W. Biederman, Andrew Morton
In-Reply-To: <20141129042308.GB30450@kroah.com>
On Nov 28, 2014 9:24 PM, "Greg KH" <greg@kroah.com> wrote:
>
> On Fri, Nov 28, 2014 at 03:05:01PM -0800, Andy Lutomirski wrote:
> > Pid reuse is common, which means that it's difficult or impossible
> > to read information about a pid from /proc without races.
> >
> > This introduces a second number associated with each (task, pidns)
> > pair called highpid. Highpid is a 64-bit number, and, barring
> > extremely unlikely circumstances or outright error, a (highpid, pid)
> > will never be reused.
> >
> > With just this change, a program can open /proc/PID/status, read the
> > "Highpid" field, and confirm that it has the expected value. If the
> > pid has been reused, then highpid will be different.
> >
> > The initial implementation is straightforward: highpid is simply a
> > 64-bit counter. If a high-end system can fork every 3 ns (which
> > would be amazing, given that just allocating a pid requires at
> > atomic operation), it would take well over 1000 years for highpid to
> > wrap.
> >
> > For CRIU's benefit, the next highpid can be set by a privileged
> > user.
> >
> > NB: The sysctl stuff only works on 64-bit systems. If the approach
> > looks good, I'll fix that somehow.
> >
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> >
> > If this goes in, there's plenty of room to add new interfaces to
> > make this more useful. For example, we could add a fancier tgkill
> > that adds and validates hightgid and highpid, and we might want to
> > add a syscall to read one's own hightgid and highpid. These would
> > be quite useful for pidfiles.
> >
> > David, would this be useful for kdbus?
> >
> > CRIU people: will this be unduly difficult to support in CRIU?
> >
> > If you all think this is a good idea, I'll fix the sysctl stuff and
> > document it. It might also be worth adding "Hightgid" to status.
> >
> > fs/proc/array.c | 5 ++++-
> > include/linux/pid.h | 2 ++
> > include/linux/pid_namespace.h | 1 +
> > kernel/pid.c | 19 +++++++++++++++----
> > kernel/pid_namespace.c | 22 ++++++++++++++++++++++
> > 5 files changed, 44 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> > index cd3653e4f35c..f1e0e69d19f9 100644
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -159,6 +159,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> > int g;
> > struct fdtable *fdt = NULL;
> > const struct cred *cred;
> > + const struct upid *upid;
> > pid_t ppid, tpid;
> >
> > rcu_read_lock();
> > @@ -170,12 +171,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> > if (tracer)
> > tpid = task_pid_nr_ns(tracer, ns);
> > }
> > + upid = pid_upid_ns(pid, ns);
> > cred = get_task_cred(p);
> > seq_printf(m,
> > "State:\t%s\n"
> > "Tgid:\t%d\n"
> > "Ngid:\t%d\n"
> > "Pid:\t%d\n"
> > + "Highpid:\t%llu\n"
> > "PPid:\t%d\n"
> > "TracerPid:\t%d\n"
> > "Uid:\t%d\t%d\t%d\t%d\n"
>
> Changing existing proc files like this is dangerous and can cause lots
> of breakage in userspace programs if you are not careful. Usually
> adding fields to the end of the file is best, but sometimes even then
> things can break :(
Point taken. I had hoped that everything would parse /proc/PID/status
by looking for the lamed headers. I'll see what the history of new
fields in there is, but I can change this easily enough.
--Andy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
^ permalink raw reply
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Andy Lutomirski @ 2014-11-29 15:32 UTC (permalink / raw)
To: Eric W. Biederman
Cc: systemd Mailing List, Pavel Emelyanov, Linux API,
linux-kernel@vger.kernel.org, criu, Cyrill Gorcunov,
Andrew Morton
In-Reply-To: <87mw7aof2m.fsf@x220.int.ebiederm.org>
On Fri, Nov 28, 2014 at 7:34 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> Pid reuse is common, which means that it's difficult or impossible
>> to read information about a pid from /proc without races.
>
> Sigh.
>
> What we need are not race free pids, but a file descriptor based process
> management api. Possibly one that starts by handing you a proc
> directory.
I agree completely, and the Capsicum stuff from FreeBSD would probably
be a very good start here.
>
> Which probably means that we need a proc file we can write to and send
> signals to a process, and another proc file we can select on and wait
> for the process to exit.
That too. In fact, I have an old patch that went nowhere that makes
the proc directory fd itself pollable.
>
> Making pids bigger just looks like bandaid.
>
> Remember evovle things in the direction of an object capability system
> things wind up being more maintainable.
Yes, but this really is intended to be a much better bandaid than what
people currently use.
I'm aiming this at two main use cases that aren't going to switch to
using fds any time soon. One is shell stuff and PID files. If we can
put something like "12345@81726162" in /run/lock/foo.pid and safely
kill `cat /run/lock/foo.pid`, that would be nice (even though sensible
users don't use pid files any more, there are *tons* of things that
still use them). This could also be used for diagnostics. Suppose
the kernel log indicates a misbehaving pid. There's currently no
race-free way to poke at those pids.
The much more pressing issue is that there are apparently programs
that think that checking the process's starttime is a good idea for
avoiding PID reuse races. Both kdbus v1 and v2 do this, hopefully
only for diagnostics. This is, IMO, completely unacceptable, but I
really don't expect kdbus to start passing even more fds around when
they'll be ignored most of the time. So, if kdbus is going to be
merged at some point, I'd like to give it the opportunity to name the
sender of a message in a way that is only mildly dangerous (in non
race-related ways) as opposed to being totally broken.
Now if someone wants to implement real light-weight capability-ish fds
in Linux, that would be neat. IIRC someone tried several years ago
using fds with some high bits set, but it never went anywhere.
FWIW, given that I seem to be the most vocal reviewer of the kdbus
patches, I feel like it's nice to offer some assistance in a piece of
the kdbus stuff that I think really would benefit from a new kernel
feature.
--Andy
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
^ permalink raw reply
* Re: [RFC PATCH] userns: Disallow setgroups unless the gid_map writer is privileged
From: serge-A9i7LUbDfNHQT0dZR+AlfA @ 2014-11-29 15:55 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett,
stable-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
kenton-AuYgBwuPrUQTaNkGU808tA, LSM, Michael Kerrisk-manpages,
Richard Weinberger, Casey Schaufler, Andrew Morton,
Andy Lutomirski
In-Reply-To: <81ba656ffbdc62a7f7e94ad17f0238ee063fe6b3.1417214430.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
iiuc this should be ok for lxc since it always has a privileged map writer.
(sorry I'm pretty much afk until dec 10)
Thanks,
- sergeOn 11/28/14 16:53 Andy Lutomirski wrote:
Classic unix permission checks have an interesting feature. The
group permissions for a file can be set to less than the other
permissions on a file. Occasionally this is used deliberately to
give a certain group of users fewer permissions than the default.
User namespaces break this usage. Groups set in rgid or egid are
unaffected because an unprivileged user namespace creator can only
map a single group, so setresgid inside and outside the namespace
have the same effect. However, an unprivileged user namespace
creator can currently use setgroups(2) to drop all supplementary
groups, so, if a supplementary group denies access to some resource,
user namespaces can be used to bypass that restriction.
To fix this issue without unduly breaking existing user code, this
introduces a new user namespace flag USERNS_SETGROUPS_ALLOWED. If
that flag is not set, then setgroups(2) will fail regardless of the
caller's capabilities.
USERNS_SETGROUPS_ALLOWED is cleared in a new user namespace. By
default, if the writer of gid_map has CAP_SETGID in the parent
userns and the parent userns has USERNS_SETGROUPS_ALLOWED, then the
USERNS_SETGROUPS_ALLOWED will be set in the child.
While it could be safe to set USERNS_SETGROUPS_ALLOWED if the user
namespace creator has no supplementary groups, doing so could be
surprising and could have unpleasant interactions with setns(2).
The default behavior can be overridden by adding a line of the form
"setgroups allow" or "setgroups deny" to gid_map. The former will
return -EPERM if the caller is not permitted to set
USERNS_SETGROUPS_ALLOWED, and the latter will clear
USERNS_SETGROUPS_ALLOWED even if the flag would otherwise be set.
This change should break userspace by the minimal amount needed to
fix this issue. Any application that uses newgidmap(1) should be
unaffected by this fix, and unprivileged users that create user
namespaces to manipulate mounts or sandbox themselves will also be
unaffected unless they try to use setgroups(2).
This should fix CVE-2014-8989.
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
Eric, this is an alternative to your patch. I think it will cause
less breakage, and it will keep unprivileged user namespaces
more or less fully functional.
Kenton, I think that neither run-bundle nor supervisor-main will be
broken by this patch.
include/linux/user_namespace.h | 3 +++
kernel/groups.c | 3 +++
kernel/user.c | 1 +
kernel/user_namespace.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 43 insertions(+)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e95372654f09..a74c1f3d44fe 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -17,6 +17,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
} extent[UID_GID_MAP_MAX_EXTENTS];
};
+#define USERNS_SETGROUPS_ALLOWED 1
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -27,6 +29,7 @@ struct user_namespace {
kuid_t owner;
kgid_t group;
unsigned int proc_inum;
+ unsigned int flags;
/* Register of per-UID persistent keyrings for this namespace */
#ifdef CONFIG_PERSISTENT_KEYRINGS
diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f86cfa..e27433809978 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -6,6 +6,7 @@
#include <linux/slab.h>
#include <linux/security.h>
#include <linux/syscalls.h>
+#include <linux/user_namespace.h>
#include <asm/uaccess.h>
/* init to 2 - one for init_task, one to ensure it is never freed */
@@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
struct group_info *group_info;
int retval;
+ if (!(current_user_ns()->flags & USERNS_SETGROUPS_ALLOWED))
+ return -EPERM;
if (!ns_capable(current_user_ns(), CAP_SETGID))
return -EPERM;
if ((unsigned)gidsetsize > NGROUPS_MAX)
diff --git a/kernel/user.c b/kernel/user.c
index 4efa39350e44..f8cdb1ec6049 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
.owner = GLOBAL_ROOT_UID,
.group = GLOBAL_ROOT_GID,
.proc_inum = PROC_USER_INIT_INO,
+ .flags = USERNS_SETGROUPS_ALLOWED,
#ifdef CONFIG_PERSISTENT_KEYRINGS
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..6e7b9ee5bddc 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -600,6 +600,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
unsigned long page = 0;
char *kbuf, *pos, *next_line;
ssize_t ret = -EINVAL;
+ unsigned int gid_flags = 0;
+ bool seen_explicit_gid_flag = false;
/*
* The id_map_mutex serializes all writes to any given map.
@@ -633,6 +635,19 @@ static ssize_t map_write(struct file *file, const char __user *buf,
if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
goto out;
+ /* Deal with supplementary groups. */
+ if (map == &ns->gid_map) {
+ /*
+ * By default, setgroups is allowed inside the userns
+ * if the writer has no supplementary groups (making
+ * it useless) or if the writer is privileged.
+ */
+ if ((ns->parent->flags & USERNS_SETGROUPS_ALLOWED) &&
+ file_ns_capable(file, ns->parent, CAP_SETGID) &&
+ ns_capable(ns->parent, CAP_SETGID))
+ gid_flags = USERNS_SETGROUPS_ALLOWED;
+ }
+
/* Get a buffer */
ret = -ENOMEM;
page = __get_free_page(GFP_TEMPORARY);
@@ -667,6 +682,25 @@ static ssize_t map_write(struct file *file, const char __user *buf,
next_line = NULL;
}
+ /* Is this line a gid_map option? */
+ if (map == &ns->gid_map) {
+ if (!strcmp(pos, "setgroups deny")) {
+ if (seen_explicit_gid_flag)
+ goto out;
+ seen_explicit_gid_flag = 1;
+ gid_flags = 0;
+ continue;
+ } else if (!strcmp(pos, "setgroups allow")) {
+ if (seen_explicit_gid_flag)
+ goto out;
+ if (!(gid_flags & USERNS_SETGROUPS_ALLOWED)) {
+ ret = -EPERM;
+ goto out;
+ }
+ continue;
+ }
+ }
+
pos = skip_spaces(pos);
extent->first = simple_strtoul(pos, &pos, 10);
if (!isspace(*pos))
@@ -746,6 +780,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
new_map.nr_extents*sizeof(new_map.extent[0]));
smp_wmb();
map->nr_extents = new_map.nr_extents;
+ if (map == &ns->gid_map)
+ ns->flags |= gid_flags;
*ppos = count;
ret = count;
--
1.9.3
^ permalink raw reply related
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Cyrill Gorcunov @ 2014-11-29 16:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Herrmann, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
Andrew Morton, systemd Mailing List
In-Reply-To: <b0f6c4df0e8ef8afcc7b786edecb4be8c752941e.1417215468.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
On Fri, Nov 28, 2014 at 03:05:01PM -0800, Andy Lutomirski wrote:
> Pid reuse is common, which means that it's difficult or impossible
> to read information about a pid from /proc without races.
>
> This introduces a second number associated with each (task, pidns)
> pair called highpid. Highpid is a 64-bit number, and, barring
> extremely unlikely circumstances or outright error, a (highpid, pid)
> will never be reused.
>
> With just this change, a program can open /proc/PID/status, read the
> "Highpid" field, and confirm that it has the expected value. If the
> pid has been reused, then highpid will be different.
>
> The initial implementation is straightforward: highpid is simply a
> 64-bit counter. If a high-end system can fork every 3 ns (which
> would be amazing, given that just allocating a pid requires at
> atomic operation), it would take well over 1000 years for highpid to
> wrap.
>
> For CRIU's benefit, the next highpid can be set by a privileged
> user.
>
> NB: The sysctl stuff only works on 64-bit systems. If the approach
> looks good, I'll fix that somehow.
>
> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> ---
>
> If this goes in, there's plenty of room to add new interfaces to
> make this more useful. For example, we could add a fancier tgkill
> that adds and validates hightgid and highpid, and we might want to
> add a syscall to read one's own hightgid and highpid. These would
> be quite useful for pidfiles.
>
> David, would this be useful for kdbus?
>
> CRIU people: will this be unduly difficult to support in CRIU?
Hi Andy. I think it won't be hard to support.
^ permalink raw reply
* Re: [RFC PATCH] userns: Disallow setgroups unless the gid_map writer is privileged
From: Eric W. Biederman @ 2014-11-29 16:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook,
Michael Kerrisk-manpages, Linux API, linux-man,
linux-kernel@vger.kernel.org, LSM, Casey Schaufler,
Serge E. Hallyn, Richard Weinberger,
kenton-AuYgBwuPrUQTaNkGU808tA, stable-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <81ba656ffbdc62a7f7e94ad17f0238ee063fe6b3.1417214430.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
The patch is buggy.
Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
>
> Eric, this is an alternative to your patch. I think it will cause
> less breakage, and it will keep unprivileged user namespaces
> more or less fully functional.
>
> Kenton, I think that neither run-bundle nor supervisor-main will be
> broken by this patch.
>
> include/linux/user_namespace.h | 3 +++
> kernel/groups.c | 3 +++
> kernel/user.c | 1 +
> kernel/user_namespace.c | 36 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 43 insertions(+)
>
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index e95372654f09..a74c1f3d44fe 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -17,6 +17,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
> } extent[UID_GID_MAP_MAX_EXTENTS];
> };
>
> +#define USERNS_SETGROUPS_ALLOWED 1
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -27,6 +29,7 @@ struct user_namespace {
> kuid_t owner;
> kgid_t group;
> unsigned int proc_inum;
> + unsigned int flags;
If you are going to add a flags field it needs to be atomic as otherwise
changing or reading individual flags won't be safe without a lock.
> /* Register of per-UID persistent keyrings for this namespace */
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> diff --git a/kernel/groups.c b/kernel/groups.c
> index 451698f86cfa..e27433809978 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -6,6 +6,7 @@
> #include <linux/slab.h>
> #include <linux/security.h>
> #include <linux/syscalls.h>
> +#include <linux/user_namespace.h>
> #include <asm/uaccess.h>
>
> /* init to 2 - one for init_task, one to ensure it is never freed */
> @@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
> struct group_info *group_info;
> int retval;
>
> + if (!(current_user_ns()->flags & USERNS_SETGROUPS_ALLOWED))
> + return -EPERM;
> if (!ns_capable(current_user_ns(), CAP_SETGID))
> return -EPERM;
> if ((unsigned)gidsetsize > NGROUPS_MAX)
> diff --git a/kernel/user.c b/kernel/user.c
> index 4efa39350e44..f8cdb1ec6049 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
> .owner = GLOBAL_ROOT_UID,
> .group = GLOBAL_ROOT_GID,
> .proc_inum = PROC_USER_INIT_INO,
> + .flags = USERNS_SETGROUPS_ALLOWED,
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index aa312b0dc3ec..6e7b9ee5bddc 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -600,6 +600,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> unsigned long page = 0;
> char *kbuf, *pos, *next_line;
> ssize_t ret = -EINVAL;
> + unsigned int gid_flags = 0;
> + bool seen_explicit_gid_flag = false;
>
> /*
> * The id_map_mutex serializes all writes to any given map.
> @@ -633,6 +635,19 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
> goto out;
>
> + /* Deal with supplementary groups. */
> + if (map == &ns->gid_map) {
> + /*
> + * By default, setgroups is allowed inside the userns
> + * if the writer has no supplementary groups (making
> + * it useless) or if the writer is privileged.
> + */
> + if ((ns->parent->flags & USERNS_SETGROUPS_ALLOWED) &&
> + file_ns_capable(file, ns->parent, CAP_SETGID) &&
> + ns_capable(ns->parent, CAP_SETGID))
> + gid_flags = USERNS_SETGROUPS_ALLOWED;
We can't do this.
It is wrong to mix permissions and flags to request functionality.
That way lies madness, and impossible maintenance, and it will silently
break every application that expects setgroups to work if they have
CAP_SETGID after a mapping has been established.
> + }
> +
> /* Get a buffer */
> ret = -ENOMEM;
> page = __get_free_page(GFP_TEMPORARY);
> @@ -667,6 +682,25 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> next_line = NULL;
> }
>
> + /* Is this line a gid_map option? */
> + if (map == &ns->gid_map) {
> + if (!strcmp(pos, "setgroups deny")) {
> + if (seen_explicit_gid_flag)
> + goto out;
> + seen_explicit_gid_flag = 1;
> + gid_flags = 0;
> + continue;
> + } else if (!strcmp(pos, "setgroups allow")) {
> + if (seen_explicit_gid_flag)
> + goto out;
> + if (!(gid_flags & USERNS_SETGROUPS_ALLOWED)) {
> + ret = -EPERM;
> + goto out;
> + }
> + continue;
> + }
> + }
> +
> pos = skip_spaces(pos);
> extent->first = simple_strtoul(pos, &pos, 10);
> if (!isspace(*pos))
> @@ -746,6 +780,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> new_map.nr_extents*sizeof(new_map.extent[0]));
> smp_wmb();
> map->nr_extents = new_map.nr_extents;
> + if (map == &ns->gid_map)
> + ns->flags |= gid_flags;
>
> *ppos = count;
> ret = count;
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] userns: Disallow setgroups unless the gid_map writer is privileged
From: Andy Lutomirski @ 2014-11-29 16:24 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook,
Michael Kerrisk-manpages, Linux API, linux-man,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LSM,
Casey Schaufler, Serge E. Hallyn, Richard Weinberger,
Kenton Varda, stable
In-Reply-To: <87vblym17s.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Sat, Nov 29, 2014 at 8:16 AM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>
> The patch is buggy.
>
> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>
>> ---
>>
>> Eric, this is an alternative to your patch. I think it will cause
>> less breakage, and it will keep unprivileged user namespaces
>> more or less fully functional.
>>
>> Kenton, I think that neither run-bundle nor supervisor-main will be
>> broken by this patch.
>>
>> include/linux/user_namespace.h | 3 +++
>> kernel/groups.c | 3 +++
>> kernel/user.c | 1 +
>> kernel/user_namespace.c | 36 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 43 insertions(+)
>>
>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
>> index e95372654f09..a74c1f3d44fe 100644
>> --- a/include/linux/user_namespace.h
>> +++ b/include/linux/user_namespace.h
>> @@ -17,6 +17,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
>> } extent[UID_GID_MAP_MAX_EXTENTS];
>> };
>>
>> +#define USERNS_SETGROUPS_ALLOWED 1
>> +
>> struct user_namespace {
>> struct uid_gid_map uid_map;
>> struct uid_gid_map gid_map;
>> @@ -27,6 +29,7 @@ struct user_namespace {
>> kuid_t owner;
>> kgid_t group;
>> unsigned int proc_inum;
>> + unsigned int flags;
>
> If you are going to add a flags field it needs to be atomic as otherwise
> changing or reading individual flags won't be safe without a lock.
Will fix.
>
>> /* Register of per-UID persistent keyrings for this namespace */
>> #ifdef CONFIG_PERSISTENT_KEYRINGS
>> diff --git a/kernel/groups.c b/kernel/groups.c
>> index 451698f86cfa..e27433809978 100644
>> --- a/kernel/groups.c
>> +++ b/kernel/groups.c
>> @@ -6,6 +6,7 @@
>> #include <linux/slab.h>
>> #include <linux/security.h>
>> #include <linux/syscalls.h>
>> +#include <linux/user_namespace.h>
>> #include <asm/uaccess.h>
>>
>> /* init to 2 - one for init_task, one to ensure it is never freed */
>> @@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>> struct group_info *group_info;
>> int retval;
>>
>> + if (!(current_user_ns()->flags & USERNS_SETGROUPS_ALLOWED))
>> + return -EPERM;
>> if (!ns_capable(current_user_ns(), CAP_SETGID))
>> return -EPERM;
>> if ((unsigned)gidsetsize > NGROUPS_MAX)
>> diff --git a/kernel/user.c b/kernel/user.c
>> index 4efa39350e44..f8cdb1ec6049 100644
>> --- a/kernel/user.c
>> +++ b/kernel/user.c
>> @@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
>> .owner = GLOBAL_ROOT_UID,
>> .group = GLOBAL_ROOT_GID,
>> .proc_inum = PROC_USER_INIT_INO,
>> + .flags = USERNS_SETGROUPS_ALLOWED,
>> #ifdef CONFIG_PERSISTENT_KEYRINGS
>> .persistent_keyring_register_sem =
>> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
>> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>> index aa312b0dc3ec..6e7b9ee5bddc 100644
>> --- a/kernel/user_namespace.c
>> +++ b/kernel/user_namespace.c
>> @@ -600,6 +600,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
>> unsigned long page = 0;
>> char *kbuf, *pos, *next_line;
>> ssize_t ret = -EINVAL;
>> + unsigned int gid_flags = 0;
>> + bool seen_explicit_gid_flag = false;
>>
>> /*
>> * The id_map_mutex serializes all writes to any given map.
>> @@ -633,6 +635,19 @@ static ssize_t map_write(struct file *file, const char __user *buf,
>> if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
>> goto out;
>>
>> + /* Deal with supplementary groups. */
>> + if (map == &ns->gid_map) {
>> + /*
>> + * By default, setgroups is allowed inside the userns
>> + * if the writer has no supplementary groups (making
>> + * it useless) or if the writer is privileged.
>> + */
>> + if ((ns->parent->flags & USERNS_SETGROUPS_ALLOWED) &&
>> + file_ns_capable(file, ns->parent, CAP_SETGID) &&
>> + ns_capable(ns->parent, CAP_SETGID))
>> + gid_flags = USERNS_SETGROUPS_ALLOWED;
>
> We can't do this.
>
> It is wrong to mix permissions and flags to request functionality.
>
> That way lies madness, and impossible maintenance, and it will silently
> break every application that expects setgroups to work if they have
> CAP_SETGID after a mapping has been established.
We can make writing gid_map fail unless you either have permissions or
add the "setgroups disallow" option. Would that be better? It avoids
silent creation of a strangely behaving userns. This would be an easy
adjustment to this patch.
I don't like the "allow setgroups if there are no supplementary
groups" because it's weird and unexpected (and will therefore break
things, I expect) and because it may interact insecurely with setns
and other such things.
--Andy
>
>> + }
>> +
>> /* Get a buffer */
>> ret = -ENOMEM;
>> page = __get_free_page(GFP_TEMPORARY);
>> @@ -667,6 +682,25 @@ static ssize_t map_write(struct file *file, const char __user *buf,
>> next_line = NULL;
>> }
>>
>> + /* Is this line a gid_map option? */
>> + if (map == &ns->gid_map) {
>> + if (!strcmp(pos, "setgroups deny")) {
>> + if (seen_explicit_gid_flag)
>> + goto out;
>> + seen_explicit_gid_flag = 1;
>> + gid_flags = 0;
>> + continue;
>> + } else if (!strcmp(pos, "setgroups allow")) {
>> + if (seen_explicit_gid_flag)
>> + goto out;
>> + if (!(gid_flags & USERNS_SETGROUPS_ALLOWED)) {
>> + ret = -EPERM;
>> + goto out;
>> + }
>> + continue;
>> + }
>> + }
>> +
>> pos = skip_spaces(pos);
>> extent->first = simple_strtoul(pos, &pos, 10);
>> if (!isspace(*pos))
>> @@ -746,6 +780,8 @@ static ssize_t map_write(struct file *file, const char __user *buf,
>> new_map.nr_extents*sizeof(new_map.extent[0]));
>> smp_wmb();
>> map->nr_extents = new_map.nr_extents;
>> + if (map == &ns->gid_map)
>> + ns->flags |= gid_flags;
>>
>> *ppos = count;
>> ret = count;
>
> Eric
--
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 37/46] tun: move internal flag defines out of uapi
From: Michael S. Tsirkin @ 2014-11-29 17:17 UTC (permalink / raw)
To: Jason Wang
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Miller,
cornelia.huck-tA70FqPdS9bQT0dZR+AlfA,
rusty-8fk3Idey6ehBDgjK7y7TUQ, nab-IzHhD5pYlfBP7FQvKIMDCQ,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA,
thuth-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Zhi Yong Wu, Ben Hutchings,
Herbert Xu, Tom Herbert, Masatake YAMATO, Xi Wang,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1417162052.5822.0-LLcwxoN42L4NLKR9yMNcA1aTQe2KTcn/@public.gmane.org>
On Fri, Nov 28, 2014 at 08:15:32AM +0008, Jason Wang wrote:
>
>
> On Fri, Nov 28, 2014 at 4:10 AM, Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >TUN_ flags are internal and never exposed
> >to userspace. Any application using it is almost
> >certainly buggy.
> >
> >Move them out to tun.c, we'll remove them in follow-up patches.
> >
> >Signed-off-by: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >---
> > include/uapi/linux/if_tun.h | 16 ++--------
> > drivers/net/tun.c | 74
> >++++++++++++++-------------------------------
> > 2 files changed, 26 insertions(+), 64 deletions(-)
> >
> >diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
> >index e9502dd..277a260 100644
> >--- a/include/uapi/linux/if_tun.h
> >+++ b/include/uapi/linux/if_tun.h
> >@@ -22,21 +22,11 @@
> > /* Read queue size */
> > #define TUN_READQ_SIZE 500
> >-
> >-/* TUN device flags */
> >-#define TUN_TUN_DEV 0x0001
> >-#define TUN_TAP_DEV 0x0002
> >+/* TUN device type flags: deprecated. Use IFF_TUN/IFF_TAP instead. */
> >+#define TUN_TUN_DEV IFF_TUN
> >+#define TUN_TAP_DEV IFF_TAP
> > #define TUN_TYPE_MASK 0x000f
> >-#define TUN_FASYNC 0x0010
> >-#define TUN_NOCHECKSUM 0x0020
> >-#define TUN_NO_PI 0x0040
> >-/* This flag has no real effect */
> >-#define TUN_ONE_QUEUE 0x0080
> >-#define TUN_PERSIST 0x0100
> >-#define TUN_VNET_HDR 0x0200
> >-#define TUN_TAP_MQ 0x0400
> >-
> > /* Ioctl defines */
> > #define TUNSETNOCSUM _IOW('T', 200, int) #define TUNSETDEBUG
> >_IOW('T', 201, int) diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >index 9dd3746..bc89d07 100644
> >--- a/drivers/net/tun.c
> >+++ b/drivers/net/tun.c
> >@@ -103,6 +103,21 @@ do { \
> > } while (0)
> > #endif
> >+/* TUN device flags */
> >+
> >+/* IFF_ATTACH_QUEUE is never stored in device flags,
> >+ * overload it to mean fasync when stored there.
> >+ */
> >+#define TUN_FASYNC IFF_ATTACH_QUEUE
> >+#define TUN_NO_PI IFF_NO_PI
> >+/* This flag has no real effect */
> >+#define TUN_ONE_QUEUE IFF_ONE_QUEUE
> >+#define TUN_PERSIST IFF_PERSIST
> >+#define TUN_VNET_HDR IFF_VNET_HDR
> >+#define TUN_TAP_MQ IFF_MULTI_QUEUE
> >+
> >+#define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
> >+ IFF_MULTI_QUEUE)
> > #define GOODCOPY_LEN 128
> > #define FLT_EXACT_COUNT 8
> >@@ -1521,32 +1536,7 @@ static struct proto tun_proto = {
> > static int tun_flags(struct tun_struct *tun)
> > {
> >- int flags = 0;
> >-
> >- if (tun->flags & TUN_TUN_DEV)
> >- flags |= IFF_TUN;
> >- else
> >- flags |= IFF_TAP;
> >-
> >- if (tun->flags & TUN_NO_PI)
> >- flags |= IFF_NO_PI;
> >-
> >- /* This flag has no real effect. We track the value for backwards
> >- * compatibility.
> >- */
> >- if (tun->flags & TUN_ONE_QUEUE)
> >- flags |= IFF_ONE_QUEUE;
> >-
> >- if (tun->flags & TUN_VNET_HDR)
> >- flags |= IFF_VNET_HDR;
> >-
> >- if (tun->flags & TUN_TAP_MQ)
> >- flags |= IFF_MULTI_QUEUE;
> >-
> >- if (tun->flags & TUN_PERSIST)
> >- flags |= IFF_PERSIST;
> >-
> >- return flags;
> >+ return tun->flags & (TUN_FEATURES | IFF_PERSIST | IFF_TUN | IFF_TAP);
> > }
> > static ssize_t tun_show_flags(struct device *dev, struct
> >device_attribute *attr,
> >@@ -1706,28 +1696,8 @@ static int tun_set_iff(struct net *net, struct file
> >*file, struct ifreq *ifr)
> > tun_debug(KERN_INFO, tun, "tun_set_iff\n");
> >- if (ifr->ifr_flags & IFF_NO_PI)
> >- tun->flags |= TUN_NO_PI;
> >- else
> >- tun->flags &= ~TUN_NO_PI;
> >-
> >- /* This flag has no real effect. We track the value for backwards
> >- * compatibility.
> >- */
> >- if (ifr->ifr_flags & IFF_ONE_QUEUE)
> >- tun->flags |= TUN_ONE_QUEUE;
> >- else
> >- tun->flags &= ~TUN_ONE_QUEUE;
> >-
> >- if (ifr->ifr_flags & IFF_VNET_HDR)
> >- tun->flags |= TUN_VNET_HDR;
> >- else
> >- tun->flags &= ~TUN_VNET_HDR;
> >-
> >- if (ifr->ifr_flags & IFF_MULTI_QUEUE)
> >- tun->flags |= TUN_TAP_MQ;
> >- else
> >- tun->flags &= ~TUN_TAP_MQ;
> >+ tun->flags = (tun->flags & ~TUN_FEATURES) |
> >+ (ifr->ifr_flags & TUN_FEATURES);
> > /* Make sure persistent devices do not get stuck in
> > * xoff state.
> >@@ -1890,9 +1860,11 @@ static long __tun_chr_ioctl(struct file *file,
> >unsigned int cmd,
> > if (cmd == TUNGETFEATURES) {
> > /* Currently this just means: "what IFF flags are valid?".
> > * This is needed because we never checked for invalid flags on
> >- * TUNSETIFF. */
> >- return put_user(IFF_TUN | IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE |
> >- IFF_VNET_HDR | IFF_MULTI_QUEUE,
> >+ * TUNSETIFF. Why do we report IFF_TUN and IFF_TAP which are
> >+ * not legal for TUNSETIFF here? It's probably a bug, but it
> >+ * doesn't seem to be worth fixing.
> >+ */
>
> The question is not very clear. IFF_TUN and IFF_TAP are legal for
> ifr.ifr_flags during TUNSETIFF. No?
Absolutely. I'm not sure what I meant anymore. I will drop this chunk -
either by replacing this patch if I have to post another version, or
by a separate patch if I don't.
> >
> >+ return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES,
> > (unsigned int __user*)argp);
> > } else if (cmd == TUNSETQUEUE)
> > return tun_set_queue(file, &ifr);
> >--
> >MST
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] userns: Disallow setgroups unless the gid_map writer is privileged
From: Andy Lutomirski @ 2014-11-29 17:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook,
Michael Kerrisk-manpages, Linux API, linux-man,
linux-kernel@vger.kernel.org, LSM, Casey Schaufler,
Serge E. Hallyn, Richard Weinberger, kenton, Andy Lutomirski,
stable
Classic unix permission checks have an interesting feature. The
group permissions for a file can be set to less than the other
permissions on a file. Occasionally this is used deliberately to
give a certain group of users fewer permissions than the default.
User namespaces break this usage. Groups set in rgid or egid are
unaffected because an unprivileged user namespace creator can only
map a single group, so setresgid inside and outside the namespace
have the same effect. However, an unprivileged user namespace
creator can currently use setgroups(2) to drop all supplementary
groups, so, if a supplementary group denies access to some resource,
user namespaces can be used to bypass that restriction.
To fix this issue, this introduces a new user namespace flag
USERNS_SETGROUPS_ALLOWED. If that flag is not set, then
setgroups(2) will fail regardless of the caller's capabilities.
USERNS_SETGROUPS_ALLOWED is cleared in a new user namespace. By
default, if the writer of gid_map has CAP_SETGID in the parent
userns and the parent userns has USERNS_SETGROUPS_ALLOWED, then the
USERNS_SETGROUPS_ALLOWED will be set in the child. If the writer is
not so privileged, then writing to gid_map will fail unless the
writer adds "setgroups deny" to gid_map, in which case the check is
skipped but USERNS_SETGROUPS_ALLOWED will remain cleared.
The full semantics are:
If "setgroups allow" is present or no explicit "setgroups" setting
is written to gid_map, then writing to gid_map will fail with -EPERM
unless the opener and writer have CAP_SETGID in the parent namespace
and the parent namespace has USERNS_SETGROUPS_ALLOWED.
If "setgroups deny" is present, then writing gid_map will work as
before, but USERNS_SETGROUPS_ALLOWED will remain cleared. This will
result in processes in the userns that have CAP_SETGID to be
nontheless unable to use setgroups(2). If this breaks something
inside the userns, then this is okay -- the userns creator
specifically requested this behavior.
While it could be safe to set USERNS_SETGROUPS_ALLOWED if the user
namespace creator has no supplementary groups, doing so could be
surprising and could have unpleasant interactions with setns(2).
Any application that uses newgidmap(1) should be unaffected by this
fix, but unprivileged users that create user namespaces to
manipulate mounts or sandbox themselves will break until they start
using "setgroups deny".
This should fix CVE-2014-8989.
Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
Unlike v1, this *will* break things like Sandstorm. Fixing them will be
easy. I agree that this will result in better long-term semantics, but
I'm not so happy about breaking working software.
If this is unpalatable, here's a different option: get rid of all these
permission checks and just change setgroups. Specifically, make it so
that setgroups(2) in a userns will succeed but will silently refuse to
remove unmapped groups.
Changes from v1:
- Userns flags are now properly atomic.
- "setgroups allow" is now the default, so legacy unprivileged gid_map
writers will start to fail.
include/linux/user_namespace.h | 3 +++
kernel/groups.c | 3 +++
kernel/user.c | 1 +
kernel/user_namespace.c | 42 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 49 insertions(+)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e95372654f09..0ae4a8c97165 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -17,6 +17,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
} extent[UID_GID_MAP_MAX_EXTENTS];
};
+#define USERNS_SETGROUPS_ALLOWED 0
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -27,6 +29,7 @@ struct user_namespace {
kuid_t owner;
kgid_t group;
unsigned int proc_inum;
+ unsigned long flags;
/* Register of per-UID persistent keyrings for this namespace */
#ifdef CONFIG_PERSISTENT_KEYRINGS
diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f86cfa..b5ec42423202 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -6,6 +6,7 @@
#include <linux/slab.h>
#include <linux/security.h>
#include <linux/syscalls.h>
+#include <linux/user_namespace.h>
#include <asm/uaccess.h>
/* init to 2 - one for init_task, one to ensure it is never freed */
@@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
struct group_info *group_info;
int retval;
+ if (!test_bit(USERNS_SETGROUPS_ALLOWED, ¤t_user_ns()->flags))
+ return -EPERM;
if (!ns_capable(current_user_ns(), CAP_SETGID))
return -EPERM;
if ((unsigned)gidsetsize > NGROUPS_MAX)
diff --git a/kernel/user.c b/kernel/user.c
index 4efa39350e44..58fba8ea0845 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
.owner = GLOBAL_ROOT_UID,
.group = GLOBAL_ROOT_GID,
.proc_inum = PROC_USER_INIT_INO,
+ .flags = (1 << USERNS_SETGROUPS_ALLOWED),
#ifdef CONFIG_PERSISTENT_KEYRINGS
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..1f63935483e9 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -601,6 +601,10 @@ static ssize_t map_write(struct file *file, const char __user *buf,
char *kbuf, *pos, *next_line;
ssize_t ret = -EINVAL;
+ bool may_setgroups = false;
+ bool setgroups_requested = true;
+ bool seen_explicit_setgroups = false;
+
/*
* The id_map_mutex serializes all writes to any given map.
*
@@ -633,6 +637,18 @@ static ssize_t map_write(struct file *file, const char __user *buf,
if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
goto out;
+ if (map == &ns->gid_map) {
+ /*
+ * Setgroups is permitted if the writer and the
+ * parent ns are privileged.
+ */
+ may_setgroups =
+ test_bit(USERNS_SETGROUPS_ALLOWED,
+ &ns->parent->flags) &&
+ file_ns_capable(file, ns->parent, CAP_SETGID) &&
+ ns_capable(ns->parent, CAP_SETGID);
+ }
+
/* Get a buffer */
ret = -ENOMEM;
page = __get_free_page(GFP_TEMPORARY);
@@ -667,6 +683,23 @@ static ssize_t map_write(struct file *file, const char __user *buf,
next_line = NULL;
}
+ /* Is this line a gid_map option? */
+ if (map == &ns->gid_map) {
+ if (!strcmp(pos, "setgroups deny")) {
+ if (seen_explicit_setgroups)
+ goto out;
+ seen_explicit_setgroups = true;
+ setgroups_requested = false;
+ continue;
+ } else if (!strcmp(pos, "setgroups allow")) {
+ if (seen_explicit_setgroups)
+ goto out;
+ seen_explicit_setgroups = true;
+ setgroups_requested = true;
+ continue;
+ }
+ }
+
pos = skip_spaces(pos);
extent->first = simple_strtoul(pos, &pos, 10);
if (!isspace(*pos))
@@ -741,6 +774,15 @@ static ssize_t map_write(struct file *file, const char __user *buf,
extent->lower_first = lower_first;
}
+ /* Validate and install setgroups permission. */
+ if (map == &ns->gid_map && setgroups_requested) {
+ if (!may_setgroups) {
+ ret = -EPERM;
+ goto out;
+ }
+ set_bit(USERNS_SETGROUPS_ALLOWED, &ns->flags);
+ }
+
/* Install the map */
memcpy(map->extent, new_map.extent,
new_map.nr_extents*sizeof(new_map.extent[0]));
--
1.9.3
^ permalink raw reply related
* Re: Why not make kdbus use CUSE?
From: Greg Kroah-Hartman @ 2014-11-29 17:59 UTC (permalink / raw)
To: Richard Yao
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20141129063416.GE32286-WkJ4KB730YtOk+SH+krGketnjj8NnB7F@public.gmane.org>
On Sat, Nov 29, 2014 at 06:34:16AM +0000, Richard Yao wrote:
> I had the opportunity at LinuxCon Europe to chat with Greg and some other kdbus
> developers. A few things stood out from our conversation that I thought I would
> bring to the list for discussion.
Any reason why you didn't respond to the kdbus patches themselves?
Critiquing the specific code is much better than random discussions.
> They regard a userland compatibility shim in the systemd repostory to provide
> backward compatibility for applications. Unfortunately, this is insufficient to
> ensure compatibility because dependency trees have multiple levels. If cross
> platform package A depends on cross platform library B, which depends on dbus,
> and cross platform library B decides to switch to kdbus, then it ceases to be
> cross platform and cross platform package A is now dependent on Linux kernels
> with kdbus. Not only does that affect other POSIX systems, but it also affects
> LTS versions of Linux.
What does LTS versions have anything to do here? And what specific
dependancies are you worried about?
> It is somewhat tempting to think that being in the kernel is necessary for
> performance, this does not appear to be true from my discussion with Greg and
> others. In specific, a key advantage of being in the kernel is a reduction in
> context switches and consequently, one would expect programs using the old API
> to benefit, but they were quite clear to me that programs using the old API do
> not benefit. At the same time, we had a similar situation where people thought
> that the httpd server had to be inside the kernel until Linux 2.6, when our
> userland APIs improved to the point where we were able to get similar if not
> better performance in userland compared to the implementation of khttpd in Linux
> 2.4.y.
Again, please see the kernel patches for lots of detail as to why this
should be in the kernel. If you disagree with the specific statements I
have listed there, please respond with specifics.
> I started to think that we probably ought to design a way to put kdbus into
> userland and then I realized that we already have one in the form of CUSE. This
> would not only makes kdbus play nicely with SELinux and lxc, but also other
> POSIX systems that currently share dbus with Linux systems, which includes older
> Linux kernels. Greg claimed that the kdbus code was fairly self contained and
> was just a character device, so I assume this is possible and I am curious why
> it is not done.
The latest version is a filesystem not a character device, your
information is out of date :)
> P.S. I also mentioned my concern that having the shim in the systemd repository
> would have a negative effect on distributons that use alterntaive libc libraries
> because the systemd developers refuse to support alternative libc libraries. I
> mentioned this to one of the people to whom Greg introduced me (and whose name
> escapes me) as we were walking to Michael Kerrisk's talk on API design. I was
> told quite plainly that such distributions are not worth consideration. If kdbus
> is merged despite concerns about security and backward compatibility, could we
> at least have the shim moved to libc netural place, like Linus' tree?
Take that up on the systemd mailing list, it's not a kernel issue.
greg k-h
^ permalink raw reply
* Re: [PATCH net-next 3/6] samples: bpf: example of stateful socket filtering
From: David Miller @ 2014-11-30 5:01 UTC (permalink / raw)
To: ast-uqk4Ao+rVK5Wk0Htik3J/w
Cc: mingo-DgEjT+Ai2ygdnm+yROfE0A, luto-kltTT9wpgjJwATOyAt5JVQ,
dborkman-H+wXaHxf7aLQT0dZR+AlfA,
hannes-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1417066951-1999-4-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
Date: Wed, 26 Nov 2014 21:42:28 -0800
> this socket filter example does:
> - creates arraymap in kernel with key 4 bytes and value 8 bytes
>
> - loads eBPF program:
> r0 = skb[14 + 9]; // load one byte of ip->proto
...
> + BPF_LD_ABS(BPF_B, 14 + 9 /* R0 = ip->proto */),
I do not want anything having to do with fixed offsets from
the skb.
Nothing should know where things are in the SKB structure,
especially user facing things.
That's why we have explicit BPF operations for fetching
specific SKB members, so that the layout is completely
transparent to the entity generating BPF programs.
Besides retaining the flexibility of changing the SKB
layout arbitrarily without breaking bpf programs, there
are also security considerations from allowing bpf
programs to load arbitrary offsets.
Sorry, I do not like this patch series at all.
^ permalink raw reply
* Re: [PATCH net-next 3/6] samples: bpf: example of stateful socket filtering
From: Alexei Starovoitov @ 2014-11-30 6:24 UTC (permalink / raw)
To: David Miller
Cc: Ingo Molnar, Andy Lutomirski, Daniel Borkmann,
Hannes Frederic Sowa, Eric Dumazet, Linux API,
Network Development, LKML
In-Reply-To: <20141129.210158.2021042941461629799.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Sat, Nov 29, 2014 at 9:01 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> Date: Wed, 26 Nov 2014 21:42:28 -0800
>
>> this socket filter example does:
>> - creates arraymap in kernel with key 4 bytes and value 8 bytes
>>
>> - loads eBPF program:
>> r0 = skb[14 + 9]; // load one byte of ip->proto
> ...
>> + BPF_LD_ABS(BPF_B, 14 + 9 /* R0 = ip->proto */),
>
> I do not want anything having to do with fixed offsets from
> the skb.
>
> Nothing should know where things are in the SKB structure,
> especially user facing things.
>
> That's why we have explicit BPF operations for fetching
> specific SKB members, so that the layout is completely
> transparent to the entity generating BPF programs.
>
> Besides retaining the flexibility of changing the SKB
> layout arbitrarily without breaking bpf programs, there
> are also security considerations from allowing bpf
> programs to load arbitrary offsets.
>
> Sorry, I do not like this patch series at all.
sigh.
You misunderstood one wrong comment in the whole series.
r0 = skb[14 + 9]; // load one byte of ip->proto
is saying
r0 = skb->data[14 + 9]; // load one byte of ip->proto
it's loading one byte of packet payload and
not one byte of skb structure.
There are plenty of other comments where I mentioned that.
Including cover letter:
"Though native eBPF programs are way more powerful than classic filters
(attachable through similar setsockopt() call), they don't have skb field
accessors yet. Like skb->pkt_type, skb->dev->ifindex are not accessible.
There are sevaral ways to achieve that. That will be in the next set of patches.
So in this set native eBPF programs can only read data from packet and
access maps"
and in patch #2:
+static bool sock_filter_is_valid_access(int off, int size, enum
bpf_access_type type)
+{
+ /* skb fields cannot be accessed yet */
+ return false;
+}
In other words, there is no way to access skb fields yet.
There are several different ways I've implemented to
access skb fields, but they all had their cons and pros, so I
dropped them all to focus on basic things first.
Which is: read packet data and access maps.
Please let me know if you want me to fix this comment
in this patch of sample code.
^ permalink raw reply
* Re: [RFC PATCH] proc, pidns: Add highpid
From: Florian Weimer @ 2014-11-30 8:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Herrmann, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
Andrew Morton, systemd Mailing List, criu-GEFAQzZX7r8dnm+yROfE0A,
Pavel Emelyanov, Cyrill Gorcunov
In-Reply-To: <b0f6c4df0e8ef8afcc7b786edecb4be8c752941e.1417215468.git.luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
* Andy Lutomirski:
> The initial implementation is straightforward: highpid is simply a
> 64-bit counter. If a high-end system can fork every 3 ns (which
> would be amazing, given that just allocating a pid requires at
> atomic operation), it would take well over 1000 years for highpid to
> wrap.
I'm not sure if I'm reading the patch correctly, but is the counter
namespaced? If yes, why?
^ permalink raw reply
* Re: kdbus: add documentation
From: Florian Weimer @ 2014-11-30 8:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: arnd, ebiederm, gnomes, teg, jkosina, luto, linux-api,
linux-kernel, daniel, dh.herrmann, tixxdz
In-Reply-To: <1416546149-24799-2-git-send-email-gregkh@linuxfoundation.org>
* Greg Kroah-Hartman:
> +The focus of this document is an overview of the low-level, native kernel D-Bus
> +transport called kdbus. Kdbus exposes its functionality via files in a
> +filesystem called 'kdbusfs'. All communication between processes takes place
> +via ioctls on files exposed through the mount point of a kdbusfs. The default
> +mount point of kdbusfs is /sys/fs/kdbus.
Does this mean the bus does not enforce the correctness of the D-Bus
introspection metadata? That's really unfortunate. Classic D-Bus
does not do this, either, and combined with the variety of approaches
used to implement D-Bus endpoints, it makes it really difficult to
figure out what D-Bus services, exactly, a process provides.
^ permalink raw reply
* Re: kdbus: add documentation
From: Florian Weimer @ 2014-11-30 9:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: arnd-r2nGTMty4D4, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io, teg-B22kvLQNl6c,
jkosina-AlSwsSmVLrQ, luto-kltTT9wpgjJwATOyAt5JVQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
daniel-cYrQPVfZoowdnm+yROfE0A, dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w,
tixxdz-Umm1ozX2/EEdnm+yROfE0A
In-Reply-To: <1416546149-24799-2-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
* Greg Kroah-Hartman:
> +7.4 Receiving messages
> +Also, if the connection allowed for file descriptor to be passed
> +(KDBUS_HELLO_ACCEPT_FD), and if the message contained any, they will be
> +installed into the receiving process after the KDBUS_CMD_MSG_RECV ioctl
> +returns. The receiving task is obliged to close all of them appropriately.
What happens if this is not possible because the file descriptor limit
of the processes would be exceeded? EMFILE, and the message will not
be received?
^ permalink raw reply
* Re: kdbus: add documentation
From: Florian Weimer @ 2014-11-30 9:08 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg Kroah-Hartman, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Jiri Kosina, Linux API,
linux-kernel@vger.kernel.org, Daniel Mack, David Herrmann,
Djalal Harouni
In-Reply-To: <CALCETrWUekPq6pquuiVperBihQHgcvCF=WzVJTZziHhUJYsh4Q@mail.gmail.com>
* Andy Lutomirski:
> At the risk of opening a can of worms, wouldn't this be much more
> useful if you could share a pool between multiple connections?
They would also be useful to reduce context switches when receiving
data from all kinds of descriptors. At present, when polling, you
receive notification, and then you have to call into the kernel,
again, to actually fetch the data and associated information. The
kernel could also queue the data for one specific recipient,
addressing the same issue that SO_REUSEPORT tries to solve (on poll
notification, the kernel does not know which recipient will eventually
retrieve the data, so it has to notify and wake up all of them).
^ permalink raw reply
* Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step
From: Christoffer Dall @ 2014-11-30 10:10 UTC (permalink / raw)
To: Peter Maydell
Cc: Alex Bennée, kvm-devel, arm-mail-list,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org,
Marc Zyngier, Alexander Graf, J. Kiszka, David Hildenbrand,
Bharat Bhushan, bp-l3A5Bk7waGM, Paolo Bonzini, Gleb Natapov,
Russell King, Catalin Marinas, Will Deacon, Lorenzo Pieralisi,
open list, open list:ABI/API
In-Reply-To: <CAFEAcA-cjM9yUXi6tc79UP0fBKAagtFKQgV3iAjz5DWr9yxZUQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Nov 26, 2014 at 07:27:06PM +0000, Peter Maydell wrote:
> On 25 November 2014 at 16:10, Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > This adds support for single-stepping the guest. As userspace can and
> > will manipulate guest registers before restarting any tweaking of the
> > registers has to occur just before control is passed back to the guest.
> > Furthermore while guest debugging is in effect we need to squash the
> > ability of the guest to single-step itself as we have no easy way of
> > re-entering the guest after the exception has been delivered to the
> > hypervisor.
>
> A corner case I don't think this patch handles: if the debugger
> tries to single step an insn which is emulated by the
> hypervisor (because it's a load/store which is trapped and
> handled as emulated mmio in userspace) then we won't
> correctly update the single-step state machine (and so we'll end
> up incorrectly stopping after the following insn rather than
> before, I think).
>
> You should be able to achieve this effect by simply always clearing
> the guest's PSTATE.SS when you advance the PC to skip the emulated
> instruction (cf the comment in the pseudocode SSAdvance() function).
>
> I think we should also be doing this PC advance on return from
> userspace's handling of the mmio rather than before we drop back
> to userspace as we do now, but I can't remember why I think that.
> Christoffer, I don't suppose you recall, do you? I think it was
> you I had this conversation with on IRC a month or so back...
>
I don't remember clearly, no. Was it not during lunch at LCU we had
this conversation?
In any case, I think it was related to how userspace observes the state
of the CPU, because when you do the MMIO operation emulation in
userspace, currently if you observe the PC though GET_ONE_REG, you'll
see a PC pointing to the next instruction, not the one you're emulating
which is strange.
Not sure what the relation to a guest single-stepping itself was.
-Christoffer
^ permalink raw reply
* Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step
From: Peter Maydell @ 2014-11-30 10:20 UTC (permalink / raw)
To: Christoffer Dall
Cc: Alex Bennée, kvm-devel, arm-mail-list,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org,
Marc Zyngier, Alexander Graf, J. Kiszka, David Hildenbrand,
Bharat Bhushan, bp-l3A5Bk7waGM, Paolo Bonzini, Gleb Natapov,
Russell King, Catalin Marinas, Will Deacon, Lorenzo Pieralisi,
open list, open list:ABI/API
In-Reply-To: <20141130101056.GB484@cbox>
On 30 November 2014 at 10:10, Christoffer Dall
<christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> In any case, I think it was related to how userspace observes the state
> of the CPU, because when you do the MMIO operation emulation in
> userspace, currently if you observe the PC though GET_ONE_REG, you'll
> see a PC pointing to the next instruction, not the one you're emulating
> which is strange.
Also if we ever add support for userspace to say "this MMIO should
fault" then we definitely need the PC-advance to happen afterwards,
not before.
> Not sure what the relation to a guest single-stepping itself was.
I think it just came up in the course of that discussion, because
single-step handling also needs to perform an action (clear PSTATE.SS)
as part of the "advance over this insn" operation. But I think that
you're right that doing the advance before dropping out to userspace
is no worse for singlestep than it is for any other case.
-- PMM
^ 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