On 07/08/2015 09:37 AM, Stephen Smalley wrote: > On 07/08/2015 06: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 <k.lewandowsk@xxxxxxxxxxx> >> Signed-off-by: Paul Osmialowski <p.osmialowsk@xxxxxxxxxxx> >> --- >> 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/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 <linux/slab.h> >> #include <linux/syscalls.h> >> #include <linux/uio.h> >> +#include <linux/security.h> >> >> #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); >> + if (ret) { >> + ret = -EPERM; >> + goto exit_unref; >> + } > > This seems convoluted and expensive. If you always want the label of > the current task here, then why not just have security_kdbus_connect() > internally extract the label of the current task? > >> @@ -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; >> + } > > Where does kdbus apply its uid-based or other restrictions on > connections? Why do we need to insert separate hooks into each of these > functions? Is there no central chokepoint already for permission > checking that we can hook? For example, why wouldn't you insert a single hook into kdbus_conn_policy_talk() where they perform their DAC checking? You would need to restructure it slightly to ensure that the security hook is only called if it passes the DAC (privileged || uid_eq) check so that we do not trigger MAC denials when DAC wouldn't have allowed it anyway. Also, kdbus_conn_policy_talk() takes a separate conn_creds argument - that should be passed through to the hook as well. -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
