Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934334AbbGHQYK (ORCPT ); Wed, 8 Jul 2015 12:24:10 -0400 Received: from smtp105.biz.mail.bf1.yahoo.com ([98.139.221.43]:31529 "EHLO smtp105.biz.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934273AbbGHQYF (ORCPT ); Wed, 8 Jul 2015 12:24:05 -0400 X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: Iu7qbfwVM1kzojBHspHuC.FSTZrJmMz9pb8AEkiyYb05b.Z abrn6CAzjEJOfsy1FlQVEspJEdQPHd8.Ify.Qn3W8RW5Vd3DQ4U3_TWsUgZJ 0.rLU97O4FfAa4nI2LUB4d8ZQGkGVWxWMo59iUuFBtMYxaZn3NhKDjUbG_eY c9ByDteTsyPg7jufAVwYmhNYTJIeNBc2VWrKzN7XbwaO9yTMZDiodzTsDX68 kbQBNvN5aYE0Dk6J4Iar3sBRzJCeOOA3xI1QngJZr50C6wQVZhMomjZTiX6H n62EenAQ8fjMJ.94VgiA09WkDOGOLGkdfneYojs_aOFCqBiZP1swcgg.gb9X 4dvBkivktrvVfvhLlDJsP60t.bZ2Qe8RikQ2AKUJNjpTqLeYAoq.rRYzRtSY Lud8TC0iO7Tx9htGDVFbWncfzEn4eLNnvNq9cB1.DKwHZXBqdJz6M5vQDaRq EWpj04RaXO65YDT.XHjFv2TuK9uqRNuGn0q6Uui5e9JI_UQz59erLvaSl3Zw - X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Message-ID: <559D4EA1.9000300@schaufler-ca.com> Date: Wed, 08 Jul 2015 09:24:01 -0700 From: Casey Schaufler User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paul Osmialowski , Paul Moore , James Morris , "Serge E. Hallyn" , Kees Cook , Tetsuo Handa , Stephen Smalley , Neil Brown , Mark Rustad , Greg Kroah-Hartman , Daniel Mack , David Herrmann , Djalal Harouni , Shuah Khan , Al Viro , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org CC: Karol Lewandowski , Lukasz Skalski Subject: Re: [RFC 5/8] kdbus: use LSM hooks in kdbus code References: <1436351110-5902-1-git-send-email-p.osmialowsk@samsung.com> <1436351110-5902-6-git-send-email-p.osmialowsk@samsung.com> In-Reply-To: <1436351110-5902-6-git-send-email-p.osmialowsk@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14405 Lines: 481 On 7/8/2015 3:25 AM, Paul Osmialowski wrote: > Originates from: > > https://github.com/lmctl/kdbus.git (branch: kdbus-lsm-v4.for-systemd-v212) > commit: aa0885489d19be92fa41c6f0a71df28763228a40 > > Signed-off-by: Karol Lewandowski > Signed-off-by: Paul Osmialowski > --- > ipc/kdbus/bus.c | 12 ++++++++++- > ipc/kdbus/bus.h | 3 +++ > ipc/kdbus/connection.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ > ipc/kdbus/connection.h | 4 ++++ > ipc/kdbus/domain.c | 9 ++++++++- > ipc/kdbus/domain.h | 2 ++ > ipc/kdbus/endpoint.c | 11 ++++++++++ > ipc/kdbus/names.c | 11 ++++++++++ > ipc/kdbus/queue.c | 30 ++++++++++++++++++---------- > 9 files changed, 124 insertions(+), 12 deletions(-) > > diff --git a/ipc/kdbus/bus.c b/ipc/kdbus/bus.c > index bbdf0f2..9894895 100644 > --- a/ipc/kdbus/bus.c > +++ b/ipc/kdbus/bus.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > > #include "bus.h" > #include "notify.h" > @@ -51,6 +52,7 @@ static void kdbus_bus_free(struct kdbus_node *node) > kdbus_domain_unref(bus->domain); > kdbus_policy_db_clear(&bus->policy_db); > kdbus_meta_proc_unref(bus->creator_meta); > + security_kdbus_bus_free(bus); > kfree(bus); > } > > @@ -161,6 +163,12 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain, > goto exit_unref; > } > > + ret = security_kdbus_bus_alloc(b); > + if (ret) { > + ret = -EPERM; > + goto exit_unref; > + } > + > /* > * Bus-limits of the creator are accounted on its real UID, just like > * all other per-user limits. > @@ -169,11 +177,13 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain, > if (IS_ERR(b->creator)) { > ret = PTR_ERR(b->creator); > b->creator = NULL; > - goto exit_unref; > + goto exit_free_security; > } > > return b; > > +exit_free_security: > + security_kdbus_bus_free(b); > exit_unref: > kdbus_node_deactivate(&b->node); > kdbus_node_unref(&b->node); > diff --git a/ipc/kdbus/bus.h b/ipc/kdbus/bus.h > index 5bea5ef..03e4a54 100644 > --- a/ipc/kdbus/bus.h > +++ b/ipc/kdbus/bus.h > @@ -53,6 +53,7 @@ struct kdbus_user; > * @notify_list: List of pending kernel-generated messages > * @notify_lock: Notification list lock > * @notify_flush_lock: Notification flushing lock > + * @security: LSM security blob > */ > struct kdbus_bus { > struct kdbus_node node; > @@ -81,6 +82,8 @@ struct kdbus_bus { > struct list_head notify_list; > spinlock_t notify_lock; > struct mutex notify_flush_lock; > + > + void *security; > }; > > struct kdbus_bus *kdbus_bus_ref(struct kdbus_bus *bus); > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c > index 9993753..b85cdc7 100644 > --- a/ipc/kdbus/connection.c > +++ b/ipc/kdbus/connection.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > > #include "bus.h" > #include "connection.h" > @@ -73,6 +74,8 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged, > bool is_activator; > bool is_monitor; > struct kvec kvec; > + u32 sid, len; > + char *label; > int ret; > > struct { > @@ -222,6 +225,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged, > } > } > > + security_task_getsecid(current, &sid); > + security_secid_to_secctx(sid, &label, &len); > + ret = security_kdbus_connect(conn, label, len); This is horrid. It's maximally inefficient for everyone. Change the label argument to a task, and pass current. Let the module decide what to do with it. While Smack and SELinux will be making the decision based on the label, you can't count on that for all modules. If you think you need to do *anything* using secids you are mistaken. The only case where a secid should be used is in legacy networking code where the maintainer has decided that security is only worth 32 bits and can never be worth more. > + if (ret) { > + ret = -EPERM; > + goto exit_unref; > + } > + > if (atomic_inc_return(&conn->user->connections) > KDBUS_USER_MAX_CONN) { > /* decremented by destructor as conn->user is valid */ > ret = -EMFILE; > @@ -276,6 +287,7 @@ static void __kdbus_conn_free(struct kref *kref) > kdbus_pool_free(conn->pool); > kdbus_ep_unref(conn->ep); > put_cred(conn->cred); > + security_kdbus_conn_free(conn); > kfree(conn->description); > kfree(conn->quota); > kfree(conn); > @@ -1107,6 +1119,12 @@ static int kdbus_conn_reply(struct kdbus_conn *src, struct kdbus_kmsg *kmsg) > if (ret < 0) > goto exit; > > + ret = security_kdbus_talk(src, dst); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } Why not return what the module reports? Why EPERM? EPERM means "you needed privilege". EACCES means "you were denied access by normal means". > + > mutex_lock(&dst->lock); > reply = kdbus_reply_find(src, dst, kmsg->msg.cookie_reply); > if (reply) { > @@ -1187,6 +1205,12 @@ static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src, > if (ret < 0) > goto exit; > > + ret = security_kdbus_talk(src, dst); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } > + > if (!kdbus_conn_policy_talk(src, current_cred(), dst)) { > ret = -EPERM; > goto exit; > @@ -1248,6 +1272,12 @@ static int kdbus_conn_unicast(struct kdbus_conn *src, struct kdbus_kmsg *kmsg) > if (ret < 0) > goto exit; > > + ret = security_kdbus_talk(src, dst); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } > + > if (is_signal) { > /* like broadcasts we eavesdrop even if the msg is dropped */ > kdbus_bus_eavesdrop(bus, src, kmsg); > @@ -1639,6 +1669,12 @@ struct kdbus_conn *kdbus_cmd_hello(struct kdbus_ep *ep, bool privileged, > if (ret < 0) > goto exit; > > + ret = security_kdbus_ep_setpolicy(c->ep->bus); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } > + > ret = kdbus_policy_set(&c->ep->bus->policy_db, args.items, > args.items_size, 1, > kdbus_conn_is_policy_holder(c), c); > @@ -1732,6 +1768,10 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) > if (ret != 0) > return ret; > > + ret = security_kdbus_conn_info(conn); > + if (ret) > + return -EPERM; > + > /* registry must be held throughout lookup *and* collecting data */ > down_read(&bus->name_registry->rwlock); > > @@ -1905,6 +1945,12 @@ int kdbus_cmd_update(struct kdbus_conn *conn, void __user *argp) > /* now that we verified the input, update the connection */ > > if (item_policy) { > + ret = security_kdbus_ep_setpolicy(conn->ep->bus); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } > + > ret = kdbus_policy_set(&conn->ep->bus->policy_db, cmd->items, > KDBUS_ITEMS_SIZE(cmd, items), > 1, true, conn); > @@ -1948,6 +1994,10 @@ int kdbus_cmd_send(struct kdbus_conn *conn, struct file *f, void __user *argp) > .argc = ARRAY_SIZE(argv), > }; > > + ret = security_kdbus_send(conn, conn->ep->bus); > + if (ret) > + return -EPERM; > + > if (!kdbus_conn_is_ordinary(conn)) > return -EOPNOTSUPP; > > @@ -2044,6 +2094,10 @@ int kdbus_cmd_recv(struct kdbus_conn *conn, void __user *argp) > .argc = ARRAY_SIZE(argv), > }; > > + ret = security_kdbus_recv(conn, conn->ep->bus); > + if (ret) > + return -EPERM; > + > if (!kdbus_conn_is_ordinary(conn) && > !kdbus_conn_is_monitor(conn) && > !kdbus_conn_is_activator(conn)) > diff --git a/ipc/kdbus/connection.h b/ipc/kdbus/connection.h > index d1ffe90..1f91d39 100644 > --- a/ipc/kdbus/connection.h > +++ b/ipc/kdbus/connection.h > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include "limits.h" > #include "metadata.h" > @@ -73,6 +74,7 @@ struct kdbus_kmsg; > * @names_queue_list: Well-known names this connection waits for > * @privileged: Whether this connection is privileged on the bus > * @faked_meta: Whether the metadata was faked on HELLO > + * @security: LSM security blob > */ > struct kdbus_conn { > struct kref kref; > @@ -113,6 +115,8 @@ struct kdbus_conn { > > bool privileged:1; > bool faked_meta:1; > + > + void *security; > }; > > struct kdbus_conn *kdbus_conn_ref(struct kdbus_conn *conn); > diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c > index ac9f760..da9cdab 100644 > --- a/ipc/kdbus/domain.c > +++ b/ipc/kdbus/domain.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > > #include "bus.h" > #include "domain.h" > @@ -73,6 +74,7 @@ static void kdbus_domain_free(struct kdbus_node *node) > put_user_ns(domain->user_namespace); > ida_destroy(&domain->user_ida); > idr_destroy(&domain->user_idr); > + security_kdbus_domain_free(domain); > kfree(domain); > } > > @@ -104,6 +106,10 @@ struct kdbus_domain *kdbus_domain_new(unsigned int access) > idr_init(&d->user_idr); > ida_init(&d->user_ida); > > + ret = security_kdbus_domain_alloc(d); > + if (ret) > + return ERR_PTR(-EPERM); > + > /* Pin user namespace so we can guarantee domain-unique bus * names. */ > d->user_namespace = get_user_ns(current_user_ns()); > > @@ -116,6 +122,7 @@ struct kdbus_domain *kdbus_domain_new(unsigned int access) > exit_unref: > kdbus_node_deactivate(&d->node); > kdbus_node_unref(&d->node); > + security_kdbus_domain_free(d); > return ERR_PTR(ret); > } > > @@ -264,7 +271,7 @@ static void __kdbus_user_free(struct kref *kref) > if (uid_valid(user->uid)) > idr_remove(&user->domain->user_idr, __kuid_val(user->uid)); > mutex_unlock(&user->domain->lock); > - > + security_kdbus_domain_free(user->domain); > kdbus_domain_unref(user->domain); > kfree(user); > } > diff --git a/ipc/kdbus/domain.h b/ipc/kdbus/domain.h > index 447a2bd..3db06d8 100644 > --- a/ipc/kdbus/domain.h > +++ b/ipc/kdbus/domain.h > @@ -31,6 +31,7 @@ > * @user_ida: Set of all users to compute small indices > * @user_namespace: User namespace, pinned at creation time > * @dentry: Root dentry of VFS mount (don't use outside of kdbusfs) > + * @security: LSM security blob > */ > struct kdbus_domain { > struct kdbus_node node; > @@ -40,6 +41,7 @@ struct kdbus_domain { > struct ida user_ida; > struct user_namespace *user_namespace; > struct dentry *dentry; > + void *security; > }; > > /** > diff --git a/ipc/kdbus/endpoint.c b/ipc/kdbus/endpoint.c > index 9a95a5e..380228f 100644 > --- a/ipc/kdbus/endpoint.c > +++ b/ipc/kdbus/endpoint.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include > > #include "bus.h" > #include "connection.h" > @@ -122,6 +123,12 @@ struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name, > kdbus_policy_db_init(&e->policy_db); > e->bus = kdbus_bus_ref(bus); > > + ret = security_kdbus_ep_create(bus); > + if (ret) { > + ret = -EPERM; > + goto exit_unref; > + } > + > ret = kdbus_node_link(&e->node, &bus->node, name); > if (ret < 0) > goto exit_unref; > @@ -265,6 +272,10 @@ int kdbus_cmd_ep_update(struct kdbus_ep *ep, void __user *argp) > .argc = ARRAY_SIZE(argv), > }; > > + ret = security_kdbus_ep_setpolicy(ep->bus); > + if (ret) > + return -EPERM; > + > ret = kdbus_args_parse(&args, argp, &cmd); > if (ret != 0) > return ret; > diff --git a/ipc/kdbus/names.c b/ipc/kdbus/names.c > index d77ee08..dd20bea 100644 > --- a/ipc/kdbus/names.c > +++ b/ipc/kdbus/names.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > #include "bus.h" > #include "connection.h" > @@ -503,6 +504,12 @@ int kdbus_cmd_name_acquire(struct kdbus_conn *conn, void __user *argp) > goto exit; > } > > + ret = security_kdbus_name_acquire(conn, item_name); > + if (ret) { > + ret = -EPERM; > + goto exit; > + } > + > /* > * Do atomic_inc_return here to reserve our slot, then decrement > * it before returning. > @@ -724,6 +731,10 @@ int kdbus_cmd_list(struct kdbus_conn *conn, void __user *argp) > if (ret != 0) > return ret; > > + ret = security_kdbus_name_list(conn->ep->bus); > + if (ret) > + return -EPERM; > + > /* lock order: domain -> bus -> ep -> names -> conn */ > down_read(®->rwlock); > down_read(&conn->ep->bus->conn_rwlock); > diff --git a/ipc/kdbus/queue.c b/ipc/kdbus/queue.c > index 25bb3ad..9872fb4 100644 > --- a/ipc/kdbus/queue.c > +++ b/ipc/kdbus/queue.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > #include "util.h" > #include "domain.h" > @@ -514,12 +515,17 @@ int kdbus_queue_entry_install(struct kdbus_queue_entry *entry, > > for (i = 0; i < res->fds_count; i++) { > if (install_fds) { > - fds[i] = get_unused_fd_flags(O_CLOEXEC); > - if (fds[i] >= 0) > - fd_install(fds[i], > - get_file(res->fds[i])); > - else > + if (security_file_receive(res->fds[i])) { > + fds[i] = -1; > incomplete_fds = true; > + } else { > + fds[i] = get_unused_fd_flags(O_CLOEXEC); > + if (fds[i] >= 0) > + fd_install(fds[i], > + get_file(res->fds[i])); > + else > + incomplete_fds = true; > + } > } else { > fds[i] = -1; > } > @@ -557,13 +563,17 @@ int kdbus_queue_entry_install(struct kdbus_queue_entry *entry, > m.fd = -1; > > if (install_fds) { > - m.fd = get_unused_fd_flags(O_CLOEXEC); > - if (m.fd < 0) { > - m.fd = -1; > + if (security_file_receive(d->memfd.file)) { > incomplete_fds = true; > } else { > - fd_install(m.fd, > - get_file(d->memfd.file)); > + m.fd = get_unused_fd_flags(O_CLOEXEC); > + if (m.fd < 0) { > + m.fd = -1; > + incomplete_fds = true; > + } else { > + fd_install(m.fd, > + get_file(d->memfd.file)); > + } > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/