* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 22:15 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, simon.mcvittie,
daniel, alban.crequy, javier.martinez, Tom Gundersen
In-Reply-To: <1414620056-6675-1-git-send-email-gregkh@linuxfoundation.org>
(reply 1/2 -- I'm replying twice to keep the threading sane)
On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> kdbus is a kernel-level IPC implementation that aims for resemblance to
> the the protocol layer with the existing userspace D-Bus daemon while
> enabling some features that couldn't be implemented before in userspace.
>
> * Support for multiple domains, completely separated from each other,
> allowing multiple virtualized instances to be used at the same time.
Given that there is no such thing as a device namespace, how does this work?
The docs seem a bit confusing to me as to whether there's a hierarchy
of domains. Do domains have a concept of a parent?
What's "container-name"?
Given that domains have random IDs, how can they be checkpointed and restored?
--Andy
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 22:19 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <1414620056-6675-1-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> * Attachment of trustable metadata to each message on demand, such as
> the sending peer's timestamp, creds, auxgroups, comm, exe, cmdline,
> cgroup path, capabilities, security label, audit information, etc,
> each taken at the time the sender issued the ioctl to send the
> message. Which of those are actually recorded and attached is
> controlled by the receiving peer.
I think that each piece of trustable metadata needs to be explicitly
opted-in to by the sender at the time of capture. Otherwise you're
asking for lots of information leaks and privilege escalations. This
is especially important given that some of the items in the current
list could be rather sensitive.
NB: UNIX sockets get this wrong, too, but that doesn't mean that kdbus
gets to blindly follow SCM_CREDENTIALS's lead. Also, there is no
excuse here about legacy code that won't opt in when needed.
--Andy
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 22:25 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, simon.mcvittie,
daniel, alban.crequy, javier.martinez, Tom Gundersen
In-Reply-To: <CALCETrUBegZ4F1sKq3LxUgANX3=syYOrqOp9=F--g9pkVHHgUA@mail.gmail.com>
On Wed, Oct 29, 2014 at 03:19:21PM -0700, Andy Lutomirski wrote:
> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > * Attachment of trustable metadata to each message on demand, such as
> > the sending peer's timestamp, creds, auxgroups, comm, exe, cmdline,
> > cgroup path, capabilities, security label, audit information, etc,
> > each taken at the time the sender issued the ioctl to send the
> > message. Which of those are actually recorded and attached is
> > controlled by the receiving peer.
>
> I think that each piece of trustable metadata needs to be explicitly
> opted-in to by the sender at the time of capture. Otherwise you're
> asking for lots of information leaks and privilege escalations. This
> is especially important given that some of the items in the current
> list could be rather sensitive.
You do have to opt-in for this information at time of capture, so I
don't understand the issue here. This is the same type of thing that
dbus does today, and I don't see the information leaks happening there,
do you?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 22:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <CALCETrWrxc8foPYbRPtxwNX0sHK_=vLFLDXXyXu+2U2=B+=qCQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Oct 29, 2014 at 03:15:51PM -0700, Andy Lutomirski wrote:
> (reply 1/2 -- I'm replying twice to keep the threading sane)
>
> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > kdbus is a kernel-level IPC implementation that aims for resemblance to
> > the the protocol layer with the existing userspace D-Bus daemon while
> > enabling some features that couldn't be implemented before in userspace.
> >
>
> > * Support for multiple domains, completely separated from each other,
> > allowing multiple virtualized instances to be used at the same time.
>
> Given that there is no such thing as a device namespace, how does this work?
See the document for the details.
> The docs seem a bit confusing to me as to whether there's a hierarchy
> of domains. Do domains have a concept of a parent?
Yes.
> What's "container-name"?
Is that used in the documentation?
> Given that domains have random IDs, how can they be checkpointed and restored?
Good question, I don't know about checkpoint/restore, but I think that
has been done. Daniel would know more than I do about that.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 22:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, simon.mcvittie,
daniel, alban.crequy, javier.martinez, Tom Gundersen
In-Reply-To: <20141029222531.GA8129@kroah.com>
On Wed, Oct 29, 2014 at 3:25 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Oct 29, 2014 at 03:19:21PM -0700, Andy Lutomirski wrote:
>> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > * Attachment of trustable metadata to each message on demand, such as
>> > the sending peer's timestamp, creds, auxgroups, comm, exe, cmdline,
>> > cgroup path, capabilities, security label, audit information, etc,
>> > each taken at the time the sender issued the ioctl to send the
>> > message. Which of those are actually recorded and attached is
>> > controlled by the receiving peer.
>>
>> I think that each piece of trustable metadata needs to be explicitly
>> opted-in to by the sender at the time of capture. Otherwise you're
>> asking for lots of information leaks and privilege escalations. This
>> is especially important given that some of the items in the current
>> list could be rather sensitive.
>
> You do have to opt-in for this information at time of capture, so I
> don't understand the issue here. This is the same type of thing that
> dbus does today, and I don't see the information leaks happening there,
> do you?
>
The docs suggest that the *receiver* opts in.
I don't think that current dbus has severe information leaks because
the total scope for information transparently sent to dbus is rather
small (struct ucred only, presumably).
--Andy
^ permalink raw reply
* Re: kdbus: add code to gather metadata
From: Andy Lutomirski @ 2014-10-29 22:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <1414620056-6675-7-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> From: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>
> A connection chooses which metadata it wants to have attached to each
> message it receives with kdbus_cmd_hello.attach_flags. The metadata
> will be attached as items to the messages. All metadata refers to
> information about the sending task at sending time, unless otherwise
> stated. Also, the metadata is copied, not referenced, so even if the
> sending task doesn't exist anymore at the time the message is received,
> the information is still preserved.
>
> See kdbus.txt for more details on which metadata can currently be
> attached to messages.
>
> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> ---
> drivers/misc/kdbus/metadata.c | 626 ++++++++++++++++++++++++++++++++++++++++++
> drivers/misc/kdbus/metadata.h | 51 ++++
> 2 files changed, 677 insertions(+)
> create mode 100644 drivers/misc/kdbus/metadata.c
> create mode 100644 drivers/misc/kdbus/metadata.h
>
> diff --git a/drivers/misc/kdbus/metadata.c b/drivers/misc/kdbus/metadata.c
> new file mode 100644
> index 000000000000..8323e6d7a071
> --- /dev/null
> +++ b/drivers/misc/kdbus/metadata.c
> @@ -0,0 +1,626 @@
> +/*
> + * Copyright (C) 2013-2014 Kay Sievers
> + * Copyright (C) 2013-2014 Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> + * Copyright (C) 2013-2014 Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> + * Copyright (C) 2013-2014 David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * Copyright (C) 2013-2014 Linux Foundation
> + *
> + * kdbus is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU Lesser General Public License as published by the
> + * Free Software Foundation; either version 2.1 of the License, or (at
> + * your option) any later version.
> + */
> +
> +#include <linux/audit.h>
> +#include <linux/capability.h>
> +#include <linux/cgroup.h>
> +#include <linux/cred.h>
> +#include <linux/file.h>
> +#include <linux/init.h>
> +#include <linux/mutex.h>
> +#include <linux/pid_namespace.h>
> +#include <linux/sched.h>
> +#include <linux/security.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/user_namespace.h>
> +#include <linux/version.h>
> +
> +#include "connection.h"
> +#include "item.h"
> +#include "message.h"
> +#include "metadata.h"
> +#include "names.h"
> +
> +/**
> + * kdbus_meta_new() - create new metadata object
> + * @meta: New metadata object
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int kdbus_meta_new(struct kdbus_meta **meta)
> +{
> + struct kdbus_meta *m;
> +
> + BUG_ON(*meta);
> +
> + m = kzalloc(sizeof(*m), GFP_KERNEL);
> + if (!m)
> + return -ENOMEM;
> +
> + /*
> + * Remember the PID and user namespaces our credentials belong to;
> + * we need to prevent leaking authorization and security-relevant
> + * data across different namespaces.
> + */
> + m->pid_namespace = get_pid_ns(task_active_pid_ns(current));
> + m->user_namespace = get_user_ns(current_user_ns());
> +
This is unusual, and it could be very expensive (it will serialize
essentially everyone on an exclusive cacheline). What attack is it
protecting against?
> +static int kdbus_meta_append_cred(struct kdbus_meta *meta)
> +{
> + struct kdbus_creds creds = {
> + .uid = from_kuid_munged(current_user_ns(), current_uid()),
> + .gid = from_kgid_munged(current_user_ns(), current_gid()),
> + .pid = task_pid_vnr(current),
> + .tid = task_tgid_vnr(current),
> + .starttime = current->start_time,
> + };
> +
> + return kdbus_meta_append_data(meta, KDBUS_ITEM_CREDS,
> + &creds, sizeof(creds));
> +}
This seems wrong to me. Shouldn't this store kuid_t, etc. directly?
Also, why pid, tid, and starttime?
> +
> + for (i = 0; i < info->ngroups; i++)
> + gid[i] = from_kgid_munged(current_user_ns(), GROUP_AT(info, i));
Ditto.
--Andy
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 22:34 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, simon.mcvittie,
daniel, alban.crequy, javier.martinez, Tom Gundersen
In-Reply-To: <20141029222729.GB8129@kroah.com>
On Wed, Oct 29, 2014 at 3:27 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Oct 29, 2014 at 03:15:51PM -0700, Andy Lutomirski wrote:
>> (reply 1/2 -- I'm replying twice to keep the threading sane)
>>
>> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > kdbus is a kernel-level IPC implementation that aims for resemblance to
>> > the the protocol layer with the existing userspace D-Bus daemon while
>> > enabling some features that couldn't be implemented before in userspace.
>> >
>>
>> > * Support for multiple domains, completely separated from each other,
>> > allowing multiple virtualized instances to be used at the same time.
>>
>> Given that there is no such thing as a device namespace, how does this work?
>
> See the document for the details.
>
>> The docs seem a bit confusing to me as to whether there's a hierarchy
>> of domains. Do domains have a concept of a parent?
>
> Yes.
Why? Aren't they completely isolated? Confused.
>
>> What's "container-name"?
>
> Is that used in the documentation?
/dev/kdbus/domain/<container-name>/+ directory shows up inside the
domain as /dev/kdbus/.
I guess that's the thing that the creator requests.
--Andy
>
>> Given that domains have random IDs, how can they be checkpointed and restored?
>
> Good question, I don't know about checkpoint/restore, but I think that
> has been done. Daniel would know more than I do about that.
>
> thanks,
>
> greg k-h
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 22:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, simon.mcvittie,
daniel, alban.crequy, javier.martinez, Tom Gundersen
In-Reply-To: <CALCETrX6vf7cKy=XDhDtn9hn1W930MRxBa=pk93RnyuZ-EaNyw@mail.gmail.com>
On Wed, Oct 29, 2014 at 3:28 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Oct 29, 2014 at 3:25 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Oct 29, 2014 at 03:19:21PM -0700, Andy Lutomirski wrote:
>>> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>> > * Attachment of trustable metadata to each message on demand, such as
>>> > the sending peer's timestamp, creds, auxgroups, comm, exe, cmdline,
>>> > cgroup path, capabilities, security label, audit information, etc,
>>> > each taken at the time the sender issued the ioctl to send the
>>> > message. Which of those are actually recorded and attached is
>>> > controlled by the receiving peer.
>>>
>>> I think that each piece of trustable metadata needs to be explicitly
>>> opted-in to by the sender at the time of capture. Otherwise you're
>>> asking for lots of information leaks and privilege escalations. This
>>> is especially important given that some of the items in the current
>>> list could be rather sensitive.
>>
>> You do have to opt-in for this information at time of capture, so I
>> don't understand the issue here. This is the same type of thing that
>> dbus does today, and I don't see the information leaks happening there,
>> do you?
>>
>
> The docs suggest that the *receiver* opts in.
>
So does the code:
+ /*
+ * The first receiver which requests additional
+ * metadata causes the message to carry it; all
+ * receivers after that will see all of the added
+ * data, even when they did not ask for it.
+ */
+ if (conn_src) {
+ /* Check if conn_src is allowed to signal */
+ ret = kdbus_ep_policy_check_broadcast(conn_dst->ep,
+ conn_src,
+ conn_dst);
+ if (ret < 0)
+ continue;
+
+ ret = kdbus_ep_policy_check_src_names(conn_dst->ep,
+ conn_src,
+ conn_dst);
+ if (ret < 0)
+ continue;
+
+ ret = kdbus_kmsg_attach_metadata(kmsg, conn_src,
+ conn_dst);
+ if (ret < 0)
+ goto exit_unlock;
+ }
+
I'd like this if the sender chose the metadata flags. In fact, I'd
want to make that feature available on regular UNIX sockets, too
(search the archives for SCM_IDENTITY).
--Andy
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Jiri Kosina @ 2014-10-29 23:00 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <1414620056-6675-1-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
> kdbus is a kernel-level IPC implementation that aims for resemblance to
> the the protocol layer with the existing userspace D-Bus daemon while
> enabling some features that couldn't be implemented before in userspace.
I'd be interested in the features that can't be implemented in userspace
(and therefore would justify existence of kdbus in the kernel). Could you
please point me to such list / documentation?
It seems to me that most of the highlight features from the cover letter
can be "easily" (for certain definition of that word, of course)
implemented in userspace (vmsplice(), sending fd through unix socket, user
namespaces, UUID management, etc).
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 23:11 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <alpine.LRH.2.00.1410292354480.11562-1ReQVI26iDCaZKY3DrU6dA@public.gmane.org>
On Thu, Oct 30, 2014 at 12:00:16AM +0100, Jiri Kosina wrote:
> On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
>
> > kdbus is a kernel-level IPC implementation that aims for resemblance to
> > the the protocol layer with the existing userspace D-Bus daemon while
> > enabling some features that couldn't be implemented before in userspace.
>
> I'd be interested in the features that can't be implemented in userspace
> (and therefore would justify existence of kdbus in the kernel). Could you
> please point me to such list / documentation?
Lennart has given whole talks about this in the past, here's a recent
talk going into the details:
https://www.youtube.com/watch?v=HPbQzm_iz_k
> It seems to me that most of the highlight features from the cover letter
> can be "easily" (for certain definition of that word, of course)
> implemented in userspace (vmsplice(), sending fd through unix socket, user
> namespaces, UUID management, etc).
We have dbus in userspace today, but that requires extra copies of data,
and isn't easy, or even possible, to do some of the application-specific
bus logic that kdbus provides. See the talk above for details, there
are slides around somewhere with just text that we can add to the cover
letter if that will help out in future spins of this patch series.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 23:12 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <20141029231106.GB16548-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, Oct 29, 2014 at 04:11:06PM -0700, Greg Kroah-Hartman wrote:
> On Thu, Oct 30, 2014 at 12:00:16AM +0100, Jiri Kosina wrote:
> > On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
> >
> > > kdbus is a kernel-level IPC implementation that aims for resemblance to
> > > the the protocol layer with the existing userspace D-Bus daemon while
> > > enabling some features that couldn't be implemented before in userspace.
> >
> > I'd be interested in the features that can't be implemented in userspace
> > (and therefore would justify existence of kdbus in the kernel). Could you
> > please point me to such list / documentation?
>
> Lennart has given whole talks about this in the past, here's a recent
> talk going into the details:
> https://www.youtube.com/watch?v=HPbQzm_iz_k
>
> > It seems to me that most of the highlight features from the cover letter
> > can be "easily" (for certain definition of that word, of course)
> > implemented in userspace (vmsplice(), sending fd through unix socket, user
> > namespaces, UUID management, etc).
>
> We have dbus in userspace today, but that requires extra copies of data,
> and isn't easy, or even possible, to do some of the application-specific
> bus logic that kdbus provides. See the talk above for details, there
> are slides around somewhere with just text that we can add to the cover
> letter if that will help out in future spins of this patch series.
Here's an article describing it as well:
https://lwn.net/Articles/580194/
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Jiri Kosina @ 2014-10-29 23:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Kosina, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <20141029231106.GB16548-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
> > > kdbus is a kernel-level IPC implementation that aims for resemblance to
> > > the the protocol layer with the existing userspace D-Bus daemon while
> > > enabling some features that couldn't be implemented before in userspace.
> >
> > I'd be interested in the features that can't be implemented in userspace
> > (and therefore would justify existence of kdbus in the kernel). Could you
> > please point me to such list / documentation?
>
> Lennart has given whole talks about this in the past, here's a recent
> talk going into the details:
> https://www.youtube.com/watch?v=HPbQzm_iz_k
I think it's a reasonable expectation that kernel patch submissions should
be reasonably self-contained though. We've always been very strict about
pushing everybody to provide extensive cover letters, changelogs and
explanations, so this shouldn't really be an exception, I think.
> > It seems to me that most of the highlight features from the cover letter
> > can be "easily" (for certain definition of that word, of course)
> > implemented in userspace (vmsplice(), sending fd through unix socket, user
> > namespaces, UUID management, etc).
>
> We have dbus in userspace today, but that requires extra copies of data,
But we can do zero-copy between processess for quite some time already, so
what exactly is the issue here?
> and isn't easy, or even possible, to do some of the application-specific
> bus logic that kdbus provides.
I unfortunately have absolutely no idea what should I imagine here.
> See the talk above for details, there are slides around somewhere with
> just text that we can add to the cover letter if that will help out in
> future spins of this patch series.
I think that would be very helpful. Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Jiri Kosina @ 2014-10-29 23:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, Marcel Holtmann, desrt-0xnayjDhYQY,
hadess-0MeiytkfxGOsTnJN9+BGXg, dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w,
tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <alpine.LRH.2.00.1410300019570.11562-1ReQVI26iDCaZKY3DrU6dA@public.gmane.org>
On Thu, 30 Oct 2014, Jiri Kosina wrote:
> > > It seems to me that most of the highlight features from the cover letter
> > > can be "easily" (for certain definition of that word, of course)
> > > implemented in userspace (vmsplice(), sending fd through unix socket, user
> > > namespaces, UUID management, etc).
> >
> > We have dbus in userspace today, but that requires extra copies of data,
>
> But we can do zero-copy between processess for quite some time already, so
> what exactly is the issue here?
>
> > and isn't easy, or even possible, to do some of the application-specific
> > bus logic that kdbus provides.
>
> I unfortunately have absolutely no idea what should I imagine here.
Also, I think I have heard that binder is going out of staging now, right?
I admittedly have very limited understanding of both binder and kdbus, but
I guess that is the case for many folks. My understanding is that they are
providing very similar functionality, so explanation why we need *both* in
the kernel would be very interesting as well.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 23:34 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, Marcel Holtmann, desrt-0xnayjDhYQY,
hadess-0MeiytkfxGOsTnJN9+BGXg, dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w,
tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <alpine.LRH.2.00.1410300024530.11562-1ReQVI26iDCaZKY3DrU6dA@public.gmane.org>
On Thu, Oct 30, 2014 at 12:26:33AM +0100, Jiri Kosina wrote:
> On Thu, 30 Oct 2014, Jiri Kosina wrote:
>
> > > > It seems to me that most of the highlight features from the cover letter
> > > > can be "easily" (for certain definition of that word, of course)
> > > > implemented in userspace (vmsplice(), sending fd through unix socket, user
> > > > namespaces, UUID management, etc).
> > >
> > > We have dbus in userspace today, but that requires extra copies of data,
> >
> > But we can do zero-copy between processess for quite some time already, so
> > what exactly is the issue here?
> >
> > > and isn't easy, or even possible, to do some of the application-specific
> > > bus logic that kdbus provides.
> >
> > I unfortunately have absolutely no idea what should I imagine here.
>
> Also, I think I have heard that binder is going out of staging now, right?
Yes, but that needs documentation, which I'm working on at the moment :)
> I admittedly have very limited understanding of both binder and kdbus, but
> I guess that is the case for many folks. My understanding is that they are
> providing very similar functionality, so explanation why we need *both* in
> the kernel would be very interesting as well.
They do very different things, see this writeup I did a while ago about
the differences between them:
http://kroah.com/log/blog/2014/01/15/kdbus-details/
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Greg Kroah-Hartman @ 2014-10-29 23:40 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <alpine.LRH.2.00.1410300019570.11562-1ReQVI26iDCaZKY3DrU6dA@public.gmane.org>
On Thu, Oct 30, 2014 at 12:24:02AM +0100, Jiri Kosina wrote:
> On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
>
> > > > kdbus is a kernel-level IPC implementation that aims for resemblance to
> > > > the the protocol layer with the existing userspace D-Bus daemon while
> > > > enabling some features that couldn't be implemented before in userspace.
> > >
> > > I'd be interested in the features that can't be implemented in userspace
> > > (and therefore would justify existence of kdbus in the kernel). Could you
> > > please point me to such list / documentation?
> >
> > Lennart has given whole talks about this in the past, here's a recent
> > talk going into the details:
> > https://www.youtube.com/watch?v=HPbQzm_iz_k
>
> I think it's a reasonable expectation that kernel patch submissions should
> be reasonably self-contained though. We've always been very strict about
> pushing everybody to provide extensive cover letters, changelogs and
> explanations, so this shouldn't really be an exception, I think.
There is a 1815 line documentation file in this series, so we aren't
trying to not provide this type of information here at all. But yes,
more background, about why this can't be done in userspace (zero copy,
less context switches, proper credential passing, timestamping, availble
at early-boot, LSM hooks for security models to tie into, race-free
interfaces, container/namespace support, etc.) should be added to the
docs as well.
> > > It seems to me that most of the highlight features from the cover letter
> > > can be "easily" (for certain definition of that word, of course)
> > > implemented in userspace (vmsplice(), sending fd through unix socket, user
> > > namespaces, UUID management, etc).
> >
> > We have dbus in userspace today, but that requires extra copies of data,
>
> But we can do zero-copy between processess for quite some time already, so
> what exactly is the issue here?
See the above list for more details.
We'll work on this for the next round of patches, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-29 23:55 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Kosina, Linux API, linux-kernel@vger.kernel.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, Simon McVittie,
daniel, alban.crequy, Javier Martinez Canillas, Tom Gundersen
In-Reply-To: <20141029234001.GB16520@kroah.com>
On Wed, Oct 29, 2014 at 4:40 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 30, 2014 at 12:24:02AM +0100, Jiri Kosina wrote:
>> On Wed, 29 Oct 2014, Greg Kroah-Hartman wrote:
>>
>> > > > kdbus is a kernel-level IPC implementation that aims for resemblance to
>> > > > the the protocol layer with the existing userspace D-Bus daemon while
>> > > > enabling some features that couldn't be implemented before in userspace.
>> > >
>> > > I'd be interested in the features that can't be implemented in userspace
>> > > (and therefore would justify existence of kdbus in the kernel). Could you
>> > > please point me to such list / documentation?
>> >
>> > Lennart has given whole talks about this in the past, here's a recent
>> > talk going into the details:
>> > https://www.youtube.com/watch?v=HPbQzm_iz_k
>>
>> I think it's a reasonable expectation that kernel patch submissions should
>> be reasonably self-contained though. We've always been very strict about
>> pushing everybody to provide extensive cover letters, changelogs and
>> explanations, so this shouldn't really be an exception, I think.
>
> There is a 1815 line documentation file in this series, so we aren't
> trying to not provide this type of information here at all. But yes,
> more background, about why this can't be done in userspace (zero copy,
> less context switches, proper credential passing, timestamping, availble
> at early-boot, LSM hooks for security models to tie into, race-free
> interfaces, container/namespace support, etc.) should be added to the
> docs as well.
It's worth noting that:
- Proper credential passing could be added to UNIX sockets, and we
may want to do that anyway. Also, the current kdbus semantics seem to
be "spew lots of credentials and other miscellaneous
potentially-sensitive and sometime spoofable information all over the
place", which isn't obviously an improvement. (This is fixable, but
it will almost certainly not be compatible with current systemd kdbus
code if fixed.)
- The current kdbus patches seem to be worse than UNIX sockets from a
namespace perspective, but maybe I'm misunderstanding how it's
supposed to work. UNIX sockets work quite nicely in containers.
- There's an obvious interface to add timestamping to UNIX sockets
(it could work exactly the way it does for UDP / PTP).
- I'm unconvinced by this performance argument without numbers. The
kdbus credential code, at least, looks to be quite heavy on allocation
and atomics. This isn't to say that the current userspace D-Bus
daemon doesn't also serialize everything, but it could be made
multithreaded.
- Race-free? What are the races that are inherent to UNIX sockets?
--Andy
^ permalink raw reply
* Re: kdbus: add code to gather metadata
From: Andy Lutomirski @ 2014-10-30 0:13 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
Simon McVittie, daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ, Javier Martinez Canillas,
Tom Gundersen
In-Reply-To: <CALCETrWqbpxk83L0k0_78JZCO+ntZhx_hHMcRu=vxs6VE2f5JQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Oct 29, 2014 at 3:33 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> From: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>>
>> A connection chooses which metadata it wants to have attached to each
>> message it receives with kdbus_cmd_hello.attach_flags. The metadata
>> will be attached as items to the messages. All metadata refers to
>> information about the sending task at sending time, unless otherwise
>> stated. Also, the metadata is copied, not referenced, so even if the
>> sending task doesn't exist anymore at the time the message is received,
>> the information is still preserved.
>>
Also, in general, the comments seem to talk about capturing metadata
at the time that a connection is opened, but the actual code seems to
capture metadata all over the place. I think it needs to be very
clear, both in the code and the interface, when metadata is captured.
And the ns_eq stuff is too far buried (and not even contained in this
patch!) to be easily verified as being correct, whatever correct means
in that context.
--Andy
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Andy Lutomirski @ 2014-10-30 2:27 UTC (permalink / raw)
To: Greg Kroah-Hartman, Eric W. Biederman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <20141029222729.GB8129-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, Oct 29, 2014 at 3:27 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, Oct 29, 2014 at 03:15:51PM -0700, Andy Lutomirski wrote:
>> (reply 1/2 -- I'm replying twice to keep the threading sane)
>>
>> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
>> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> > kdbus is a kernel-level IPC implementation that aims for resemblance to
>> > the the protocol layer with the existing userspace D-Bus daemon while
>> > enabling some features that couldn't be implemented before in userspace.
>> >
>>
>> > * Support for multiple domains, completely separated from each other,
>> > allowing multiple virtualized instances to be used at the same time.
>>
>> Given that there is no such thing as a device namespace, how does this work?
>
> See the document for the details.
They seem insufficient to me, so I tried to dig in to the code. My
understanding is:
The parent container has /dev mounted. It sends an IOCTL (which
requires global capabilities). In response, kdbus creates a whole
bunch of devices that get put (by udev or devtmpfs, I presume) in a
subdirectory. Then the parent container mounts that subdirectory in
the new container.
This is IMO rather problematic.
First, it enforces the existence of a kdbus domain hierarchy where
none should be needed.
Second, it's incompatible with nested user namespaces. The middle
namespace can't issue the ioctl.
Third, it requires a devtmpfs submount in the child container. This
scares me, especially since there are no device namespaces. Also, the
child container appears to be dependent on the host udev to arbitrate
everything, which seems totally wrong to me. (Also, now we're exposed
to attacks where the child container creates busses or endpoints or
whatever with malicious names to try to trick the host into screwing
up.)
ISTM this should be solved either with device namespaces (which is
well known to be a giant can of worms) or by abandoning the concept of
kdbus using device nodes entirely.
What if kdbus were kdbusfs? If you want to use it in a container, you
mount a brand-new kdbusfs there. No weird domain hierarchy, no global
privilege, no need to name containers, obvious migration semantics, no
dependence on udev/devtmpfs at all, etc.
Eric, any thoughts here?
--Andy
^ permalink raw reply
* Re: kdbus: add driver skeleton, ioctl entry points and utility functions
From: Eric W. Biederman @ 2014-10-30 3:50 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c
In-Reply-To: <1414620056-6675-4-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
> From: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>
> Add the basic driver structure.
>
> handle.c is the main ioctl command dispatcher that calls into other parts
> of the driver.
>
> main.c contains the code that creates the initial domain at startup, and
> util.c has utility functions such as item iterators that are shared with
> other files.
>
> limits.h describes limits on things like maximum data structure sizes,
> number of messages per users and suchlike. Some of the numbers currently
> picked are rough ideas of what what might be sufficient and are probably
> rather conservative.
>
> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> +/* kdbus control device commands */
> +static long kdbus_handle_ioctl_control(struct file *file, unsigned int cmd,
> + void __user *buf)
> +{
> + case KDBUS_CMD_DOMAIN_MAKE: {
> + const char *name;
> +
> + if (!capable(CAP_IPC_OWNER)) {
> + ret = -EPERM;
> + break;
> + }
I don't know if this is exploitable (given that this happens in an
ioctl) but capable checks outside of open usually are.
Eric
^ permalink raw reply
* Re: kdbus: add connection, queue handling and message validation code
From: Andy Lutomirski @ 2014-10-30 3:55 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Greg Kroah-Hartman, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, Simon McVittie,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ, Javier Martinez Canillas,
Tom Gundersen
In-Reply-To: <87k33iw759.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Wed, Oct 29, 2014 at 8:47 PM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
>
>> From: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>>
>> This patch adds code to create and destroy connections, to validate
>> incoming messages and to maintain the queue of messages that are
>> associated with a connection.
>>
>> Note that connection and queue have a 1:1 relation, the code is only
>> split in two parts for cleaner separation and better readability.
>
> You are not performing capability checks at open time.
>
> As such this API is suceptible to a host of file descriptor passing attacks.
To be fair, write(2) doesn't work on these fds, so the usual attacks
don't work. But who knows what absurd things kdbus clients will do
with fd passing?
--Andy
>
>> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>> ---
>
>> +/*
>> + * Check for maximum number of messages per individual user. This
>> + * should prevent a single user from being able to fill the receiver's
>> + * queue.
>> + */
>> +static int kdbus_conn_queue_user_quota(struct kdbus_conn *conn,
>> + const struct kdbus_conn *conn_src,
>> + struct kdbus_queue_entry *entry)
>> +{
>> + unsigned int user;
>> +
>> + if (!conn_src)
>> + return 0;
>> +
>> + if (ns_capable(&init_user_ns, CAP_IPC_OWNER))
>> + return 0;
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: kdbus: add code for buses, domains and endpoints
From: Eric W. Biederman @ 2014-10-30 3:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c,
Andy Lutomirski
In-Reply-To: <1414620056-6675-9-git-send-email-gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
The way capabilities are checked in this patch make me very nervous.
We are not checking permissions at open time. Every other location
of calling capable on file like objects has been show to be suceptible
to file descriptor pass attacks.
> See Documentation/kdbus.txt for more details.
>
> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> ---
> diff --git a/drivers/misc/kdbus/bus.c b/drivers/misc/kdbus/bus.c
> new file mode 100644
> index 000000000000..6dcaf22f5d59
> --- /dev/null
> +++ b/drivers/misc/kdbus/bus.c
> @@ -0,0 +1,450 @@
> +/**
> + * kdbus_bus_cred_is_privileged() - check whether the given credentials in
> + * combination with the capabilities of the
> + * current thead are privileged on the bus
> + * @bus: The bus to check
> + * @cred: The credentials to match
> + *
> + * Return: true if the credentials are privileged, otherwise false.
> + */
> +bool kdbus_bus_cred_is_privileged(const struct kdbus_bus *bus,
> + const struct cred *cred)
> +{
> + /* Capabilities are *ALWAYS* tested against the current thread, they're
> + * never remembered from conn-credentials. */
> + if (ns_capable(&init_user_ns, CAP_IPC_OWNER))
> + return true;
> +
> + return uid_eq(bus->uid_owner, cred->fsuid);
> +}
> +
> +/**
> + * kdbus_bus_uid_is_privileged() - check whether the current user is a
> + * priviledged bus user
> + * @bus: The bus to check
> + *
> + * Return: true if the current user has CAP_IPC_OWNER capabilities, or
> + * if it has the same UID as the user that created the bus. Otherwise,
> + * false is returned.
> + */
> +bool kdbus_bus_uid_is_privileged(const struct kdbus_bus *bus)
> +{
> + return kdbus_bus_cred_is_privileged(bus, current_cred());
> +}
> +/**
> + * kdbus_bus_new() - create a new bus
> + * @domain: The domain to work on
> + * @make: Pointer to a struct kdbus_cmd_make containing the
> + * details for the bus creation
> + * @name: Name of the bus
> + * @bloom: Bloom parameters for this bus
> + * @mode: The access mode for the device node
> + * @uid: The uid of the device node
> + * @gid: The gid of the device node
> + * @bus: Pointer to a reference where the new bus is stored
> + *
> + * This function will allocate a new kdbus_bus and link it to the given
> + * domain.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int kdbus_bus_new(struct kdbus_domain *domain,
> + const struct kdbus_cmd_make *make,
> + const char *name,
> + const struct kdbus_bloom_parameter *bloom,
> + umode_t mode, kuid_t uid, kgid_t gid,
> + struct kdbus_bus **bus)
> +{
[snip]
> +
> + if (!capable(CAP_IPC_OWNER) &&
> + atomic_inc_return(&b->user->buses) > KDBUS_USER_MAX_BUSES) {
> + atomic_dec(&b->user->buses);
> + ret = -EMFILE;
> + goto exit_unref_user_unlock;
> + }
> +
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Eric W. Biederman @ 2014-10-30 4:04 UTC (permalink / raw)
To: Greg KH
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c,
Andy Lutomirski
In-Reply-To: <20141029221505.GA7812-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> writes:
> On Wed, Oct 29, 2014 at 03:00:44PM -0700, Greg Kroah-Hartman wrote:
>> kdbus is a kernel-level IPC implementation that aims for resemblance to
>> the the protocol layer with the existing userspace D-Bus daemon while
>> enabling some features that couldn't be implemented before in userspace.
>
> {sigh}
>
> I'll blame it on the jet-lag for the lack of [XX/12] markings on the
> patches. I'll give it a day for review before resending if people
> really want to know the ordering. It doesn't matter except for the
> final patch that adds the code to the build file.
>
> sorry about that,
For what it is worth these patches are also poorly split up. Every
patch I looked at in detail had functions that were being introduced
that did not have callers.
That poor split up of the patches makes it difficult to see how
the functionality that is being introduced is being used.
Eric
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Eric W. Biederman @ 2014-10-30 4:20 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Djalal Harouni, Arnd Bergmann, Ryan Lortie, Greg Kroah-Hartman,
Marcel Holtmann, David Herrmann,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Tom Gundersen, simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ, John Stultz,
Bastien Nocera, Linux API, Tejun Heo, Linux Containers,
Linus Torvalds, javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ,
daniel-cYrQPVfZoowdnm+yROfE0A
In-Reply-To: <CALCETrVxvF2ie=vVgpjeqikn+nci_9jyKfU4s3t=4cjyNZNaNQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
The userspace API breaks userspace in an unfixable way.
Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Problem the first.
- Using global names for containers makes it impossible to create
unprivileged containers.
This is a back to the drawing board problem, and makes device
nodes fundamentally unsuited to what you are doing.
There is no way that I can see to make it safe for an unprivileged
user to create arbitrary named busses. Especially in the presence
of allowing unprivileged checkpoint/restart.
This is particularly bad as kdbus explicitly allows unprivielged
creation of new kdbus instances.
This problem is a userspace regression.
Problem the second.
- The security checks in the code are not based on who opens the
file descriptors but instead based on who is used the file
descriptors at any give moment.
That pattern has been shown to be exploitable.
I expect the policy database makes this poor choice of permission
checks even worse. Pass a more privileged user a kdbus file
descriptor and all of sudden things that were not possible on
that file descriptor become possible.
Problem the third.
- You are using device numbers for things created by unprivileged
users. That breaks checkpoint/restart. Aka CRIU.
We can not migrate a container to a new machine and preserve the
device numbers.
We can not migrate a container to a new machine and have any hope
of preserving the container patsh under /dev/kdbus/...
Both of which look like fundamental show stoppers for
checkpoint/restart.
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
> On Wed, Oct 29, 2014 at 3:27 PM, Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> On Wed, Oct 29, 2014 at 03:15:51PM -0700, Andy Lutomirski wrote:
>>> (reply 1/2 -- I'm replying twice to keep the threading sane)
>>>
>>> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman
>>> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>>> > kdbus is a kernel-level IPC implementation that aims for resemblance to
>>> > the the protocol layer with the existing userspace D-Bus daemon while
>>> > enabling some features that couldn't be implemented before in userspace.
>>> >
>>>
>>> > * Support for multiple domains, completely separated from each other,
>>> > allowing multiple virtualized instances to be used at the same time.
>>>
>>> Given that there is no such thing as a device namespace, how does this work?
>>
>> See the document for the details.
>
> They seem insufficient to me, so I tried to dig in to the code. My
> understanding is:
>
> The parent container has /dev mounted. It sends an IOCTL (which
> requires global capabilities). In response, kdbus creates a whole
> bunch of devices that get put (by udev or devtmpfs, I presume) in a
> subdirectory. Then the parent container mounts that subdirectory in
> the new container.
>
> This is IMO rather problematic.
>
> First, it enforces the existence of a kdbus domain hierarchy where
> none should be needed.
>
> Second, it's incompatible with nested user namespaces. The middle
> namespace can't issue the ioctl.
>
> Third, it requires a devtmpfs submount in the child container. This
> scares me, especially since there are no device namespaces. Also, the
> child container appears to be dependent on the host udev to arbitrate
> everything, which seems totally wrong to me. (Also, now we're exposed
> to attacks where the child container creates busses or endpoints or
> whatever with malicious names to try to trick the host into screwing
> up.)
>
> ISTM this should be solved either with device namespaces (which is
> well known to be a giant can of worms) or by abandoning the concept of
> kdbus using device nodes entirely.
>
> What if kdbus were kdbusfs? If you want to use it in a container, you
> mount a brand-new kdbusfs there. No weird domain hierarchy, no global
> privilege, no need to name containers, obvious migration semantics, no
> dependence on udev/devtmpfs at all, etc.
>
> Eric, any thoughts here?
I think a kdbusfs modeled on devpts with newinstance at
mount time would solve the naming problems.
That would break one of the current kdbus use cases that allows an
unprivileged user to create a bus.
Eric
p.s. Please excuse my brevity I have am in the middle of packing up my
possessions (including my main machine), as I move this week.
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Daniel Mack @ 2014-10-30 7:12 UTC (permalink / raw)
To: Eric W. Biederman, Greg KH
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
john.stultz-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
tj-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
desrt-0xnayjDhYQY, hadess-0MeiytkfxGOsTnJN9+BGXg,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, teg-B22kvLQNl6c,
Andy Lutomirski
In-Reply-To: <87egtqurrp.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On 10/30/2014 05:04 AM, Eric W. Biederman wrote:
> For what it is worth these patches are also poorly split up. Every
> patch I looked at in detail had functions that were being introduced
> that did not have callers.
Yes, we wanted to keep the reply threading cleaner and the individual
patches short. With a patch set that avoids introducing functions
without callers, each patch would have grown substantially. But I know
that's unusual to do it that way.
> That poor split up of the patches makes it difficult to see how
> the functionality that is being introduced is being used.
Ok, I see. For now, I think it's probably easiest to pull the patches
from here, and then look at the resulting files directly:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/log/?h=kdbus
Other than that, please give us some time to respond to your longer
reply. Thanks for taking the time to write this up!
Daniel
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Daniel Mack @ 2014-10-30 7:44 UTC (permalink / raw)
To: Andy Lutomirski, Greg Kroah-Hartman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <CALCETrX6vf7cKy=XDhDtn9hn1W930MRxBa=pk93RnyuZ-EaNyw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 10/29/2014 11:28 PM, Andy Lutomirski wrote:
> On Wed, Oct 29, 2014 at 3:25 PM, Greg Kroah-Hartman
>> You do have to opt-in for this information at time of capture, so
>> I don't understand the issue here. This is the same type of thing
>> that dbus does today, and I don't see the information leaks
>> happening there, do you?
>
> The docs suggest that the *receiver* opts in.
Yes, that's true.
> I don't think that current dbus has severe information leaks because
> the total scope for information transparently sent to dbus is rather
> small (struct ucred only, presumably).
Which piece of credential information are you concerned about,
particularly? I might miss something, but AFAICS, all of that
information can be queried by a remote peer anyway, through /proc for
instance. The reason why we (optionally) attach them to messages is that
we want to let the other side know which information was authoritative,
precisely at the time the message was sent. Current implementation can't
do that in a race-free way.
Also note that we currently drop all such metadata whenever a message
crosses a PID or user namespace boundary. This is because we currently
don't know yet which information we would want to transport in such
cases, and how the translation in both directions would look like, from
a semantic perspective. Hence, we decided to leave that for later.
I'll go through your other replies during the day. Thanks for your input
on that RFC, everyone.
Daniel
^ 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