| Message ID | 1466711578-64398-6-git-send-email-danielj@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jun 23, 2016 at 10:52:51PM +0300, Dan Jurgens wrote: > From: Daniel Jurgens <danielj@mellanox.com> > > Add a type and access vector for PKeys. Implement the qp_pkey_access > and mad_agent_pkey_access hooks to check that the caller has > permission to access the PKey on the given subnet prefix. Add an Extra space before " Add" > interface to get the PKey SID. Walk the PKey ocontexts to find an entry > for the given subnet prefix and pkey. > > Signed-off-by: Daniel Jurgens <danielj@mellanox.com> > Reviewed-by: Eli Cohen <eli@mellanox.com> > --- > include/linux/lsm_audit.h | 7 ++++ > security/selinux/hooks.c | 41 ++++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 ++ > security/selinux/include/initial_sid_to_string.h | 1 + > security/selinux/include/security.h | 2 ++ > security/selinux/ss/services.c | 41 ++++++++++++++++++++++++ > 6 files changed, 94 insertions(+) > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > index ffb9c9d..8ff7eae 100644 > --- a/include/linux/lsm_audit.h > +++ b/include/linux/lsm_audit.h > @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { > u16 cmd; > }; > > +struct lsm_pkey_audit { > + u64 subnet_prefix; > + u16 pkey; > +}; > + > /* Auxiliary data to use in generating the audit record. */ > struct common_audit_data { > char type; > @@ -59,6 +64,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_INODE 9 > #define LSM_AUDIT_DATA_DENTRY 10 > #define LSM_AUDIT_DATA_IOCTL_OP 11 > +#define LSM_AUDIT_DATA_PKEY 12 > union { > struct path path; > struct dentry *dentry; > @@ -75,6 +81,7 @@ struct common_audit_data { > #endif > char *kmod_name; > struct lsm_ioctlop_audit *op; > + struct lsm_pkey_audit *pkey; > } u; > /* this union contains LSM specific data */ > union { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4f13ea4..5a40b10 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6018,6 +6018,44 @@ static void selinux_unregister_ib_flush_callback(void) > mutex_unlock(&ib_flush_mutex); > } > > +static int selinux_pkey_access(u64 subnet_prefix, u16 pkey_val, void *security) > +{ > + struct common_audit_data ad; > + int err; > + u32 sid = 0; > + struct ib_security_struct *sec = security; > + struct lsm_pkey_audit pkey; > + > + err = security_pkey_sid(subnet_prefix, pkey_val, &sid); > + > + if (err) > + goto out; > + > + ad.type = LSM_AUDIT_DATA_PKEY; > + pkey.subnet_prefix = subnet_prefix; > + pkey.pkey = pkey_val; > + ad.u.pkey = &pkey; > + err = avc_has_perm(sec->sid, sid, > + SECCLASS_INFINIBAND_PKEY, > + INFINIBAND_PKEY__ACCESS, &ad); > +out: > + return err; > +} > + > +static int selinux_ib_qp_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_qp_security *qp_sec) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + qp_sec->q_security); > +} > + > +static int selinux_ib_mad_agent_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_mad_agent *mad_agent) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + mad_agent->m_security); > +} > + > static int selinux_ib_qp_alloc_security(struct ib_qp_security *qp_sec) > { > struct ib_security_struct *sec; > @@ -6248,6 +6286,9 @@ static struct security_hook_list selinux_hooks[] = { > selinux_register_ib_flush_callback), > LSM_HOOK_INIT(unregister_ib_flush_callback, > selinux_unregister_ib_flush_callback), > + LSM_HOOK_INIT(ib_qp_pkey_access, selinux_ib_qp_pkey_access), > + LSM_HOOK_INIT(ib_mad_agent_pkey_access, > + selinux_ib_mad_agent_pkey_access), > LSM_HOOK_INIT(ib_qp_alloc_security, > selinux_ib_qp_alloc_security), > LSM_HOOK_INIT(ib_qp_free_security, > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 1f1f4b2..d42dd4d 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -165,5 +165,7 @@ struct security_class_mapping secclass_map[] = { > { COMMON_CAP_PERMS, NULL } }, > { "cap2_userns", > { COMMON_CAP2_PERMS, NULL } }, > + { "infiniband_pkey", > + { "access", NULL } }, > { NULL } > }; > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index a59b64e..8f2eefc 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -29,5 +29,6 @@ static const char *initial_sid_to_string[] = > "policy", > "scmp_packet", > "devnull", > + "pkey", > }; > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index a7e6ed2..8f1a66e 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -180,6 +180,8 @@ int security_get_user_sids(u32 callsid, char *username, > > int security_port_sid(u8 protocol, u16 port, u32 *out_sid); > > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid); > + > int security_netif_sid(char *name, u32 *if_sid); > > int security_node_sid(u16 domain, void *addr, u32 addrlen, > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 89df646..49701a5 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2229,6 +2229,47 @@ out: > } > > /** > + * security_pkey_sid - Obtain the SID for a pkey. > + * @subnet_prefix: Subnet Prefix > + * @pkey_num: pkey number > + * @out_sid: security identifier > + */ > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid) > +{ > + struct ocontext *c; > + int rc = 0; > + > + read_lock(&policy_rwlock); > + > + c = policydb.ocontexts[OCON_PKEY]; > + while (c) { > + if (c->u.pkey.low_pkey <= pkey_num && > + c->u.pkey.high_pkey >= pkey_num && > + c->u.pkey.subnet_prefix == subnet_prefix) > + break; > + > + c = c->next; > + } > + > + if (c) { > + if (!c->sid[0]) { > + rc = sidtab_context_to_sid(&sidtab, > + &c->context[0], > + &c->sid[0]); > + if (rc) > + goto out; > + } > + *out_sid = c->sid[0]; > + } else { > + *out_sid = SECINITSID_PKEY; > + } Curly brackets are not needed > + > +out: > + read_unlock(&policy_rwlock); > + return rc; > +} > + > +/** > * security_netif_sid - Obtain the SID for a network interface. > * @name: interface name > * @if_sid: interface SID > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/30/2016 10:24 AM, Yuval Shaia wrote: > On Thu, Jun 23, 2016 at 10:52:51PM +0300, Dan Jurgens wrote: >> + if (c) { >> + if (!c->sid[0]) { >> + rc = sidtab_context_to_sid(&sidtab, >> + &c->context[0], >> + &c->sid[0]); >> + if (rc) >> + goto out; >> + } >> + *out_sid = c->sid[0]; >> + } else { >> + *out_sid = SECINITSID_PKEY; >> + } > Curly brackets are not needed > According to the coding style guide if either branch requires brackets both should use them: This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches: if (condition) { do_this(); do_that(); } else { otherwise(); }
On Thu, Jun 23, 2016 at 3:52 PM, Dan Jurgens <danielj@mellanox.com> wrote: > From: Daniel Jurgens <danielj@mellanox.com> > > Add a type and access vector for PKeys. Implement the qp_pkey_access > and mad_agent_pkey_access hooks to check that the caller has > permission to access the PKey on the given subnet prefix. Add an > interface to get the PKey SID. Walk the PKey ocontexts to find an entry > for the given subnet prefix and pkey. > > Signed-off-by: Daniel Jurgens <danielj@mellanox.com> > Reviewed-by: Eli Cohen <eli@mellanox.com> > --- > include/linux/lsm_audit.h | 7 ++++ > security/selinux/hooks.c | 41 ++++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 ++ > security/selinux/include/initial_sid_to_string.h | 1 + > security/selinux/include/security.h | 2 ++ > security/selinux/ss/services.c | 41 ++++++++++++++++++++++++ > 6 files changed, 94 insertions(+) > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > index ffb9c9d..8ff7eae 100644 > --- a/include/linux/lsm_audit.h > +++ b/include/linux/lsm_audit.h > @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { > u16 cmd; > }; > > +struct lsm_pkey_audit { > + u64 subnet_prefix; > + u16 pkey; > +}; > + > /* Auxiliary data to use in generating the audit record. */ > struct common_audit_data { > char type; > @@ -59,6 +64,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_INODE 9 > #define LSM_AUDIT_DATA_DENTRY 10 > #define LSM_AUDIT_DATA_IOCTL_OP 11 > +#define LSM_AUDIT_DATA_PKEY 12 > union { > struct path path; > struct dentry *dentry; > @@ -75,6 +81,7 @@ struct common_audit_data { > #endif > char *kmod_name; > struct lsm_ioctlop_audit *op; > + struct lsm_pkey_audit *pkey; > } u; > /* this union contains LSM specific data */ > union { Please correct me if I'm wrong, but I don't recall seeing any code in the patchset which actually logs the extra IB information in the audit record, did I miss it? > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4f13ea4..5a40b10 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6018,6 +6018,44 @@ static void selinux_unregister_ib_flush_callback(void) > mutex_unlock(&ib_flush_mutex); > } > > +static int selinux_pkey_access(u64 subnet_prefix, u16 pkey_val, void *security) > +{ > + struct common_audit_data ad; > + int err; > + u32 sid = 0; > + struct ib_security_struct *sec = security; > + struct lsm_pkey_audit pkey; > + > + err = security_pkey_sid(subnet_prefix, pkey_val, &sid); > + > + if (err) > + goto out; Pet peeve of mine, just do the return here directly, there is no need to jump to out. > + ad.type = LSM_AUDIT_DATA_PKEY; > + pkey.subnet_prefix = subnet_prefix; > + pkey.pkey = pkey_val; > + ad.u.pkey = &pkey; > + err = avc_has_perm(sec->sid, sid, > + SECCLASS_INFINIBAND_PKEY, > + INFINIBAND_PKEY__ACCESS, &ad); Similar, just return avc_has_perm() directly. > +out: > + return err; > +} > + > +static int selinux_ib_qp_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_qp_security *qp_sec) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + qp_sec->q_security); > +} > + > +static int selinux_ib_mad_agent_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_mad_agent *mad_agent) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + mad_agent->m_security); > +} > + > static int selinux_ib_qp_alloc_security(struct ib_qp_security *qp_sec) > { > struct ib_security_struct *sec; > @@ -6248,6 +6286,9 @@ static struct security_hook_list selinux_hooks[] = { > selinux_register_ib_flush_callback), > LSM_HOOK_INIT(unregister_ib_flush_callback, > selinux_unregister_ib_flush_callback), > + LSM_HOOK_INIT(ib_qp_pkey_access, selinux_ib_qp_pkey_access), > + LSM_HOOK_INIT(ib_mad_agent_pkey_access, > + selinux_ib_mad_agent_pkey_access), > LSM_HOOK_INIT(ib_qp_alloc_security, > selinux_ib_qp_alloc_security), > LSM_HOOK_INIT(ib_qp_free_security, > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 1f1f4b2..d42dd4d 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -165,5 +165,7 @@ struct security_class_mapping secclass_map[] = { > { COMMON_CAP_PERMS, NULL } }, > { "cap2_userns", > { COMMON_CAP2_PERMS, NULL } }, > + { "infiniband_pkey", > + { "access", NULL } }, > { NULL } > }; > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index a59b64e..8f2eefc 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -29,5 +29,6 @@ static const char *initial_sid_to_string[] = > "policy", > "scmp_packet", > "devnull", > + "pkey", > }; > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index a7e6ed2..8f1a66e 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -180,6 +180,8 @@ int security_get_user_sids(u32 callsid, char *username, > > int security_port_sid(u8 protocol, u16 port, u32 *out_sid); > > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid); > + > int security_netif_sid(char *name, u32 *if_sid); > > int security_node_sid(u16 domain, void *addr, u32 addrlen, > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 89df646..49701a5 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2229,6 +2229,47 @@ out: > } > > /** > + * security_pkey_sid - Obtain the SID for a pkey. > + * @subnet_prefix: Subnet Prefix > + * @pkey_num: pkey number > + * @out_sid: security identifier > + */ > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid) > +{ > + struct ocontext *c; > + int rc = 0; > + > + read_lock(&policy_rwlock); > + > + c = policydb.ocontexts[OCON_PKEY]; > + while (c) { > + if (c->u.pkey.low_pkey <= pkey_num && > + c->u.pkey.high_pkey >= pkey_num && > + c->u.pkey.subnet_prefix == subnet_prefix) > + break; > + > + c = c->next; > + } > + > + if (c) { > + if (!c->sid[0]) { > + rc = sidtab_context_to_sid(&sidtab, > + &c->context[0], > + &c->sid[0]); > + if (rc) > + goto out; > + } > + *out_sid = c->sid[0]; > + } else { > + *out_sid = SECINITSID_PKEY; > + } > + > +out: > + read_unlock(&policy_rwlock); > + return rc; > +} I wondered about this earlier in the patchset when we were discussing the policy format, and I'm still wondering; perhaps you can help me understand IB a bit better ... From what I gather, the partition key is the IB security boundary, not the subnet, is that true? If so, why are we including the subnet with the partition key value/label? I understand the low/high pkey range as a way of simplifying the policy, but I don't quite understand the point of tying the subnet to the partition key label. Would you ever want to have multiple labels for a single partition key, or should it be a single label for the partition key regardless of the subnet?
On 7/1/2016 11:29 AM, Paul Moore wrote: > On Thu, Jun 23, 2016 at 3:52 PM, Dan Jurgens <danielj@mellanox.com> wrote: >> From: Daniel Jurgens <danielj@mellanox.com> >> >> Add a type and access vector for PKeys. Implement the qp_pkey_access >> and mad_agent_pkey_access hooks to check that the caller has >> permission to access the PKey on the given subnet prefix. Add an >> interface to get the PKey SID. Walk the PKey ocontexts to find an entry >> for the given subnet prefix and pkey. >> >> Signed-off-by: Daniel Jurgens <danielj@mellanox.com> >> Reviewed-by: Eli Cohen <eli@mellanox.com> >> --- >> include/linux/lsm_audit.h | 7 ++++ >> security/selinux/hooks.c | 41 ++++++++++++++++++++++++ >> security/selinux/include/classmap.h | 2 ++ >> security/selinux/include/initial_sid_to_string.h | 1 + >> security/selinux/include/security.h | 2 ++ >> security/selinux/ss/services.c | 41 ++++++++++++++++++++++++ >> 6 files changed, 94 insertions(+) >> >> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h >> index ffb9c9d..8ff7eae 100644 >> --- a/include/linux/lsm_audit.h >> +++ b/include/linux/lsm_audit.h >> @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { >> u16 cmd; >> }; >> >> +struct lsm_pkey_audit { >> + u64 subnet_prefix; >> + u16 pkey; >> +}; >> + >> /* Auxiliary data to use in generating the audit record. */ >> struct common_audit_data { >> char type; >> @@ -59,6 +64,7 @@ struct common_audit_data { >> #define LSM_AUDIT_DATA_INODE 9 >> #define LSM_AUDIT_DATA_DENTRY 10 >> #define LSM_AUDIT_DATA_IOCTL_OP 11 >> +#define LSM_AUDIT_DATA_PKEY 12 >> union { >> struct path path; >> struct dentry *dentry; >> @@ -75,6 +81,7 @@ struct common_audit_data { >> #endif >> char *kmod_name; >> struct lsm_ioctlop_audit *op; >> + struct lsm_pkey_audit *pkey; >> } u; >> /* this union contains LSM specific data */ >> union { > Please correct me if I'm wrong, but I don't recall seeing any code in > the patchset which actually logs the extra IB information in the audit > record, did I miss it? I didn't make any changes to the audit logging. The messages look like this. [245259.895597] audit: type=1400 audit(1467392186.710:631): avc: denied { access } for pid=27519 comm="ib_write_bw" scontext=root:sysadm_r:sysadm_t:s0 tcontext=system_u:object_r:staff_allowed_pkey_t:s0 tclass=infiniband_pkey permissive=1 > I wondered about this earlier in the patchset when we were discussing > the policy format, and I'm still wondering; perhaps you can help me > understand IB a bit better ... > > From what I gather, the partition key is the IB security boundary, not > the subnet, is that true? If so, why are we including the subnet with > the partition key value/label? I understand the low/high pkey range > as a way of simplifying the policy, but I don't quite understand the > point of tying the subnet to the partition key label. Would you ever > want to have multiple labels for a single partition key, or should it > be a single label for the partition key regardless of the subnet? > Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets.
On Fri, Jul 1, 2016 at 2:21 PM, Daniel Jurgens <danielj@mellanox.com> wrote: > On 7/1/2016 11:29 AM, Paul Moore wrote: >> On Thu, Jun 23, 2016 at 3:52 PM, Dan Jurgens <danielj@mellanox.com> wrote: >>> From: Daniel Jurgens <danielj@mellanox.com> >>> >>> Add a type and access vector for PKeys. Implement the qp_pkey_access >>> and mad_agent_pkey_access hooks to check that the caller has >>> permission to access the PKey on the given subnet prefix. Add an >>> interface to get the PKey SID. Walk the PKey ocontexts to find an entry >>> for the given subnet prefix and pkey. >>> >>> Signed-off-by: Daniel Jurgens <danielj@mellanox.com> >>> Reviewed-by: Eli Cohen <eli@mellanox.com> >>> --- >>> include/linux/lsm_audit.h | 7 ++++ >>> security/selinux/hooks.c | 41 ++++++++++++++++++++++++ >>> security/selinux/include/classmap.h | 2 ++ >>> security/selinux/include/initial_sid_to_string.h | 1 + >>> security/selinux/include/security.h | 2 ++ >>> security/selinux/ss/services.c | 41 ++++++++++++++++++++++++ >>> 6 files changed, 94 insertions(+) >>> >>> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h >>> index ffb9c9d..8ff7eae 100644 >>> --- a/include/linux/lsm_audit.h >>> +++ b/include/linux/lsm_audit.h >>> @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { >>> u16 cmd; >>> }; >>> >>> +struct lsm_pkey_audit { >>> + u64 subnet_prefix; >>> + u16 pkey; >>> +}; >>> + >>> /* Auxiliary data to use in generating the audit record. */ >>> struct common_audit_data { >>> char type; >>> @@ -59,6 +64,7 @@ struct common_audit_data { >>> #define LSM_AUDIT_DATA_INODE 9 >>> #define LSM_AUDIT_DATA_DENTRY 10 >>> #define LSM_AUDIT_DATA_IOCTL_OP 11 >>> +#define LSM_AUDIT_DATA_PKEY 12 >>> union { >>> struct path path; >>> struct dentry *dentry; >>> @@ -75,6 +81,7 @@ struct common_audit_data { >>> #endif >>> char *kmod_name; >>> struct lsm_ioctlop_audit *op; >>> + struct lsm_pkey_audit *pkey; >>> } u; >>> /* this union contains LSM specific data */ >>> union { >> Please correct me if I'm wrong, but I don't recall seeing any code in >> the patchset which actually logs the extra IB information in the audit >> record, did I miss it? > > I didn't make any changes to the audit logging. The messages look like this. > [245259.895597] audit: type=1400 audit(1467392186.710:631): avc: denied { access } for pid=27519 comm="ib_write_bw" scontext=root:sysadm_r:sysadm_t:s0 tcontext=system_u:object_r:staff_allowed_pkey_t:s0 tclass=infiniband_pkey permissive=1 So why are you adding fields to the common_audit_data structure? If you don't think the IB specific data is worth recording in the audit log, don't record it in the structure. If you do believe it is important to record these IB details, please write the necessary code. >> I wondered about this earlier in the patchset when we were discussing >> the policy format, and I'm still wondering; perhaps you can help me >> understand IB a bit better ... >> >> From what I gather, the partition key is the IB security boundary, not >> the subnet, is that true? If so, why are we including the subnet with >> the partition key value/label? I understand the low/high pkey range >> as a way of simplifying the policy, but I don't quite understand the >> point of tying the subnet to the partition key label. Would you ever >> want to have multiple labels for a single partition key, or should it >> be a single label for the partition key regardless of the subnet? >> > Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets. Perhaps a better explanation of partitions and subnets are in order, especially for those of like me who are new to IB.
On 7/1/2016 1:59 PM, Paul Moore wrote: > On Fri, Jul 1, 2016 at 2:21 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >> On 7/1/2016 11:29 AM, Paul Moore wrote: >>> I wondered about this earlier in the patchset when we were discussing >>> the policy format, and I'm still wondering; perhaps you can help me >>> understand IB a bit better ... >>> >>> From what I gather, the partition key is the IB security boundary, not >>> the subnet, is that true? If so, why are we including the subnet with >>> the partition key value/label? I understand the low/high pkey range >>> as a way of simplifying the policy, but I don't quite understand the >>> point of tying the subnet to the partition key label. Would you ever >>> want to have multiple labels for a single partition key, or should it >>> be a single label for the partition key regardless of the subnet? >>> >> Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets. > Perhaps a better explanation of partitions and subnets are in order, > especially for those of like me who are new to IB. > A subnet is a set of ports managed by a common subnet manager, which sets up the partition configuration. A partition is a virtual fabric, similar to an VLAN. If there are multiple IB ports each could be connected to a different subnet. By including the subnet prefix in the label the subnets can use the same PKey values and policy can restrict access appropriately. Without that mechanism if one of the subnets had a partition with PKey 1 the other partition couldn't reuse that PKey if a different security policy is desired for that subnet.
On Fri, Jul 1, 2016 at 3:16 PM, Daniel Jurgens <danielj@mellanox.com> wrote: > On 7/1/2016 1:59 PM, Paul Moore wrote: >> On Fri, Jul 1, 2016 at 2:21 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >>> On 7/1/2016 11:29 AM, Paul Moore wrote: >>>> I wondered about this earlier in the patchset when we were discussing >>>> the policy format, and I'm still wondering; perhaps you can help me >>>> understand IB a bit better ... >>>> >>>> From what I gather, the partition key is the IB security boundary, not >>>> the subnet, is that true? If so, why are we including the subnet with >>>> the partition key value/label? I understand the low/high pkey range >>>> as a way of simplifying the policy, but I don't quite understand the >>>> point of tying the subnet to the partition key label. Would you ever >>>> want to have multiple labels for a single partition key, or should it >>>> be a single label for the partition key regardless of the subnet? >>>> >>> Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets. >> Perhaps a better explanation of partitions and subnets are in order, >> especially for those of like me who are new to IB. >> > > A subnet is a set of ports managed by a common subnet manager, which sets up the partition configuration. So there can be multiple partitions inside a subnet and not multiple subnets inside a partition? > A partition is a virtual fabric, similar to an VLAN. Yeah, I've read that in multiple places and I think that is what I find confusing as it doesn't seem to mesh with my understanding of what you are intending. > If there are multiple IB ports each could be connected to a different subnet. Ports are just end points, I get that. That's important, but it isn't helping me understand the relationship between subnets and partitions, that is where I'm struggling at the moment. > By including the subnet prefix in the label the subnets can use the same PKey values and policy can restrict access appropriately. This doesn't make any sense to me right now. > Without that mechanism if one of the subnets had a partition with PKey 1 the other partition couldn't reuse that PKey if a different security policy is desired for that subnet. <blank stare>
On 7/1/2016 2:26 PM, Paul Moore wrote: > On Fri, Jul 1, 2016 at 3:16 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >> On 7/1/2016 1:59 PM, Paul Moore wrote: >>> On Fri, Jul 1, 2016 at 2:21 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >>>> On 7/1/2016 11:29 AM, Paul Moore wrote: >>>>> I wondered about this earlier in the patchset when we were discussing >>>>> the policy format, and I'm still wondering; perhaps you can help me >>>>> understand IB a bit better ... >>>>> >>>>> From what I gather, the partition key is the IB security boundary, not >>>>> the subnet, is that true? If so, why are we including the subnet with >>>>> the partition key value/label? I understand the low/high pkey range >>>>> as a way of simplifying the policy, but I don't quite understand the >>>>> point of tying the subnet to the partition key label. Would you ever >>>>> want to have multiple labels for a single partition key, or should it >>>>> be a single label for the partition key regardless of the subnet? >>>>> >>>> Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets. >>> Perhaps a better explanation of partitions and subnets are in order, >>> especially for those of like me who are new to IB. >>> >> A subnet is a set of ports managed by a common subnet manager, which sets up the partition configuration. > So there can be multiple partitions inside a subnet and not multiple > subnets inside a partition? Yes, a each subnet can have many partitions. The partitions are contained within that subnet, a different subnet can have a partition that uses same PKey value, but that's a different partition. So if we have 2 subnets, fe80:: and fe81:: they can each have a partition that uses PKey X but it doesn't mean nodes with access to that partition on 0xfe80 can reach nodes on 0xfe81 on that partition. > >> A partition is a virtual fabric, similar to an VLAN. > Yeah, I've read that in multiple places and I think that is what I > find confusing as it doesn't seem to mesh with my understanding of > what you are intending. > >> If there are multiple IB ports each could be connected to a different subnet. > Ports are just end points, I get that. That's important, but it isn't > helping me understand the relationship between subnets and partitions, > that is where I'm struggling at the moment. Subnets have one or more partitions. Partitions belong to one subnet. >> By including the subnet prefix in the label the subnets can use the same PKey values and policy can restrict access appropriately. > This doesn't make any sense to me right now. > >> Without that mechanism if one of the subnets had a partition with PKey 1 the other partition couldn't reuse that PKey if a different security policy is desired for that subnet. > <blank stare> Going back to the example above assume fe80:: has a partition using PKey 0x8001 and we grant a user access. Without the subnet prefix in the label we are granting that user the ability to use pkey 0x8001 on any partition available. So subnet fe81:: can't use PKey 0x8001 if it doesn't want to grant that same user access. By labeling with the subnet prefix we can grant access to PKey 0x8001 on subnet fe80:: only, leaving the subnet manager on fe81:: the ability to use that same PKey but not grant access to said user.
On Fri, Jul 1, 2016 at 3:57 PM, Daniel Jurgens <danielj@mellanox.com> wrote: > On 7/1/2016 2:26 PM, Paul Moore wrote: >> On Fri, Jul 1, 2016 at 3:16 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >>> On 7/1/2016 1:59 PM, Paul Moore wrote: >>>> On Fri, Jul 1, 2016 at 2:21 PM, Daniel Jurgens <danielj@mellanox.com> wrote: >>>>> On 7/1/2016 11:29 AM, Paul Moore wrote: >>>>>> I wondered about this earlier in the patchset when we were discussing >>>>>> the policy format, and I'm still wondering; perhaps you can help me >>>>>> understand IB a bit better ... >>>>>> >>>>>> From what I gather, the partition key is the IB security boundary, not >>>>>> the subnet, is that true? If so, why are we including the subnet with >>>>>> the partition key value/label? I understand the low/high pkey range >>>>>> as a way of simplifying the policy, but I don't quite understand the >>>>>> point of tying the subnet to the partition key label. Would you ever >>>>>> want to have multiple labels for a single partition key, or should it >>>>>> be a single label for the partition key regardless of the subnet? >>>>>> >>>>> Each subnet can have a different partition configuration and a node can be on multiple subnets. By specifying the subnet prefix along with the pkey value the user has flexibility to have different policy for different subnets, instead of a global PKey space that would require coordinating the partition configuration across all subnets. >>>> Perhaps a better explanation of partitions and subnets are in order, >>>> especially for those of like me who are new to IB. >>>> >>> A subnet is a set of ports managed by a common subnet manager, which sets up the partition configuration. >> So there can be multiple partitions inside a subnet and not multiple >> subnets inside a partition? > > Yes, a each subnet can have many partitions. The partitions are contained within that subnet, a different subnet can have a partition that uses same PKey value, but that's a different partition. So if we have 2 subnets, fe80:: and fe81:: they can each have a partition that uses PKey X but it doesn't mean nodes with access to that partition on 0xfe80 can reach nodes on 0xfe81 on that partition. Thanks, that clears things up. Originally I thought it was the other way around which we causing a lot of confusion on my part.
On 06/23/2016 03:52 PM, Dan Jurgens wrote: > From: Daniel Jurgens <danielj@mellanox.com> > > Add a type and access vector for PKeys. Implement the qp_pkey_access > and mad_agent_pkey_access hooks to check that the caller has > permission to access the PKey on the given subnet prefix. Add an > interface to get the PKey SID. Walk the PKey ocontexts to find an entry > for the given subnet prefix and pkey. > > Signed-off-by: Daniel Jurgens <danielj@mellanox.com> > Reviewed-by: Eli Cohen <eli@mellanox.com> > --- > include/linux/lsm_audit.h | 7 ++++ > security/selinux/hooks.c | 41 ++++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 ++ > security/selinux/include/initial_sid_to_string.h | 1 + > security/selinux/include/security.h | 2 ++ > security/selinux/ss/services.c | 41 ++++++++++++++++++++++++ > 6 files changed, 94 insertions(+) > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > index ffb9c9d..8ff7eae 100644 > --- a/include/linux/lsm_audit.h > +++ b/include/linux/lsm_audit.h > @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { > u16 cmd; > }; > > +struct lsm_pkey_audit { > + u64 subnet_prefix; > + u16 pkey; > +}; > + > /* Auxiliary data to use in generating the audit record. */ > struct common_audit_data { > char type; > @@ -59,6 +64,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_INODE 9 > #define LSM_AUDIT_DATA_DENTRY 10 > #define LSM_AUDIT_DATA_IOCTL_OP 11 > +#define LSM_AUDIT_DATA_PKEY 12 > union { > struct path path; > struct dentry *dentry; > @@ -75,6 +81,7 @@ struct common_audit_data { > #endif > char *kmod_name; > struct lsm_ioctlop_audit *op; > + struct lsm_pkey_audit *pkey; > } u; > /* this union contains LSM specific data */ > union { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4f13ea4..5a40b10 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6018,6 +6018,44 @@ static void selinux_unregister_ib_flush_callback(void) > mutex_unlock(&ib_flush_mutex); > } > > +static int selinux_pkey_access(u64 subnet_prefix, u16 pkey_val, void *security) > +{ > + struct common_audit_data ad; > + int err; > + u32 sid = 0; > + struct ib_security_struct *sec = security; > + struct lsm_pkey_audit pkey; > + > + err = security_pkey_sid(subnet_prefix, pkey_val, &sid); > + > + if (err) > + goto out; > + > + ad.type = LSM_AUDIT_DATA_PKEY; > + pkey.subnet_prefix = subnet_prefix; > + pkey.pkey = pkey_val; > + ad.u.pkey = &pkey; > + err = avc_has_perm(sec->sid, sid, > + SECCLASS_INFINIBAND_PKEY, > + INFINIBAND_PKEY__ACCESS, &ad); > +out: > + return err; > +} > + > +static int selinux_ib_qp_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_qp_security *qp_sec) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + qp_sec->q_security); > +} > + > +static int selinux_ib_mad_agent_pkey_access(u64 subnet_prefix, u16 pkey_val, > + struct ib_mad_agent *mad_agent) > +{ > + return selinux_pkey_access(subnet_prefix, pkey_val, > + mad_agent->m_security); > +} > + > static int selinux_ib_qp_alloc_security(struct ib_qp_security *qp_sec) > { > struct ib_security_struct *sec; > @@ -6248,6 +6286,9 @@ static struct security_hook_list selinux_hooks[] = { > selinux_register_ib_flush_callback), > LSM_HOOK_INIT(unregister_ib_flush_callback, > selinux_unregister_ib_flush_callback), > + LSM_HOOK_INIT(ib_qp_pkey_access, selinux_ib_qp_pkey_access), > + LSM_HOOK_INIT(ib_mad_agent_pkey_access, > + selinux_ib_mad_agent_pkey_access), > LSM_HOOK_INIT(ib_qp_alloc_security, > selinux_ib_qp_alloc_security), > LSM_HOOK_INIT(ib_qp_free_security, > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 1f1f4b2..d42dd4d 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -165,5 +165,7 @@ struct security_class_mapping secclass_map[] = { > { COMMON_CAP_PERMS, NULL } }, > { "cap2_userns", > { COMMON_CAP2_PERMS, NULL } }, > + { "infiniband_pkey", > + { "access", NULL } }, > { NULL } > }; > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index a59b64e..8f2eefc 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -29,5 +29,6 @@ static const char *initial_sid_to_string[] = > "policy", > "scmp_packet", > "devnull", > + "pkey", I don't think we can/should add any more initial SIDs until we have dynamic discovery support for them. At present, we'll have problems with old kernel with new policy and with new kernel with old policy when the number of initial SIDs changes. > }; > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index a7e6ed2..8f1a66e 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -180,6 +180,8 @@ int security_get_user_sids(u32 callsid, char *username, > > int security_port_sid(u8 protocol, u16 port, u32 *out_sid); > > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid); > + > int security_netif_sid(char *name, u32 *if_sid); > > int security_node_sid(u16 domain, void *addr, u32 addrlen, > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 89df646..49701a5 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2229,6 +2229,47 @@ out: > } > > /** > + * security_pkey_sid - Obtain the SID for a pkey. > + * @subnet_prefix: Subnet Prefix > + * @pkey_num: pkey number > + * @out_sid: security identifier > + */ > +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid) > +{ > + struct ocontext *c; > + int rc = 0; > + > + read_lock(&policy_rwlock); > + > + c = policydb.ocontexts[OCON_PKEY]; > + while (c) { > + if (c->u.pkey.low_pkey <= pkey_num && > + c->u.pkey.high_pkey >= pkey_num && > + c->u.pkey.subnet_prefix == subnet_prefix) > + break; > + > + c = c->next; > + } > + > + if (c) { > + if (!c->sid[0]) { > + rc = sidtab_context_to_sid(&sidtab, > + &c->context[0], > + &c->sid[0]); > + if (rc) > + goto out; > + } > + *out_sid = c->sid[0]; > + } else { > + *out_sid = SECINITSID_PKEY; Could we just use SECINITSID_UNLABELED as the default? > + } > + > +out: > + read_unlock(&policy_rwlock); > + return rc; > +} > + > +/** > * security_netif_sid - Obtain the SID for a network interface. > * @name: interface name > * @if_sid: interface SID >
On 7/11/2016 9:45 AM, Stephen Smalley wrote: > On 06/23/2016 03:52 PM, Dan Jurgens wrote: >> diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h >> index a59b64e..8f2eefc 100644 >> --- a/security/selinux/include/initial_sid_to_string.h >> +++ b/security/selinux/include/initial_sid_to_string.h >> @@ -29,5 +29,6 @@ static const char *initial_sid_to_string[] = >> "policy", >> "scmp_packet", >> "devnull", >> + "pkey", > I don't think we can/should add any more initial SIDs until we have > dynamic discovery support for them. At present, we'll have problems > with old kernel with new policy and with new kernel with old policy when > the number of initial SIDs changes. > >> + if (c) { >> + if (!c->sid[0]) { >> + rc = sidtab_context_to_sid(&sidtab, >> + &c->context[0], >> + &c->sid[0]); >> + if (rc) >> + goto out; >> + } >> + *out_sid = c->sid[0]; >> + } else { >> + *out_sid = SECINITSID_PKEY; > Could we just use SECINITSID_UNLABELED as the default? > I don't see why that would be a problem. I'll take the same comment for the "[PATCH 06/12] selinux: Add IB End Port SMP access vector".
On Mon, Jul 11, 2016 at 10:46 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > I don't think we can/should add any more initial SIDs until we have > dynamic discovery support for them. At present, we'll have problems > with old kernel with new policy and with new kernel with old policy when > the number of initial SIDs changes. Seems like a reasonable policy to me.
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h index ffb9c9d..8ff7eae 100644 --- a/include/linux/lsm_audit.h +++ b/include/linux/lsm_audit.h @@ -45,6 +45,11 @@ struct lsm_ioctlop_audit { u16 cmd; }; +struct lsm_pkey_audit { + u64 subnet_prefix; + u16 pkey; +}; + /* Auxiliary data to use in generating the audit record. */ struct common_audit_data { char type; @@ -59,6 +64,7 @@ struct common_audit_data { #define LSM_AUDIT_DATA_INODE 9 #define LSM_AUDIT_DATA_DENTRY 10 #define LSM_AUDIT_DATA_IOCTL_OP 11 +#define LSM_AUDIT_DATA_PKEY 12 union { struct path path; struct dentry *dentry; @@ -75,6 +81,7 @@ struct common_audit_data { #endif char *kmod_name; struct lsm_ioctlop_audit *op; + struct lsm_pkey_audit *pkey; } u; /* this union contains LSM specific data */ union { diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4f13ea4..5a40b10 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6018,6 +6018,44 @@ static void selinux_unregister_ib_flush_callback(void) mutex_unlock(&ib_flush_mutex); } +static int selinux_pkey_access(u64 subnet_prefix, u16 pkey_val, void *security) +{ + struct common_audit_data ad; + int err; + u32 sid = 0; + struct ib_security_struct *sec = security; + struct lsm_pkey_audit pkey; + + err = security_pkey_sid(subnet_prefix, pkey_val, &sid); + + if (err) + goto out; + + ad.type = LSM_AUDIT_DATA_PKEY; + pkey.subnet_prefix = subnet_prefix; + pkey.pkey = pkey_val; + ad.u.pkey = &pkey; + err = avc_has_perm(sec->sid, sid, + SECCLASS_INFINIBAND_PKEY, + INFINIBAND_PKEY__ACCESS, &ad); +out: + return err; +} + +static int selinux_ib_qp_pkey_access(u64 subnet_prefix, u16 pkey_val, + struct ib_qp_security *qp_sec) +{ + return selinux_pkey_access(subnet_prefix, pkey_val, + qp_sec->q_security); +} + +static int selinux_ib_mad_agent_pkey_access(u64 subnet_prefix, u16 pkey_val, + struct ib_mad_agent *mad_agent) +{ + return selinux_pkey_access(subnet_prefix, pkey_val, + mad_agent->m_security); +} + static int selinux_ib_qp_alloc_security(struct ib_qp_security *qp_sec) { struct ib_security_struct *sec; @@ -6248,6 +6286,9 @@ static struct security_hook_list selinux_hooks[] = { selinux_register_ib_flush_callback), LSM_HOOK_INIT(unregister_ib_flush_callback, selinux_unregister_ib_flush_callback), + LSM_HOOK_INIT(ib_qp_pkey_access, selinux_ib_qp_pkey_access), + LSM_HOOK_INIT(ib_mad_agent_pkey_access, + selinux_ib_mad_agent_pkey_access), LSM_HOOK_INIT(ib_qp_alloc_security, selinux_ib_qp_alloc_security), LSM_HOOK_INIT(ib_qp_free_security, diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 1f1f4b2..d42dd4d 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -165,5 +165,7 @@ struct security_class_mapping secclass_map[] = { { COMMON_CAP_PERMS, NULL } }, { "cap2_userns", { COMMON_CAP2_PERMS, NULL } }, + { "infiniband_pkey", + { "access", NULL } }, { NULL } }; diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index a59b64e..8f2eefc 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -29,5 +29,6 @@ static const char *initial_sid_to_string[] = "policy", "scmp_packet", "devnull", + "pkey", }; diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index a7e6ed2..8f1a66e 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -180,6 +180,8 @@ int security_get_user_sids(u32 callsid, char *username, int security_port_sid(u8 protocol, u16 port, u32 *out_sid); +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid); + int security_netif_sid(char *name, u32 *if_sid); int security_node_sid(u16 domain, void *addr, u32 addrlen, diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 89df646..49701a5 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2229,6 +2229,47 @@ out: } /** + * security_pkey_sid - Obtain the SID for a pkey. + * @subnet_prefix: Subnet Prefix + * @pkey_num: pkey number + * @out_sid: security identifier + */ +int security_pkey_sid(u64 subnet_prefix, u16 pkey_num, u32 *out_sid) +{ + struct ocontext *c; + int rc = 0; + + read_lock(&policy_rwlock); + + c = policydb.ocontexts[OCON_PKEY]; + while (c) { + if (c->u.pkey.low_pkey <= pkey_num && + c->u.pkey.high_pkey >= pkey_num && + c->u.pkey.subnet_prefix == subnet_prefix) + break; + + c = c->next; + } + + if (c) { + if (!c->sid[0]) { + rc = sidtab_context_to_sid(&sidtab, + &c->context[0], + &c->sid[0]); + if (rc) + goto out; + } + *out_sid = c->sid[0]; + } else { + *out_sid = SECINITSID_PKEY; + } + +out: + read_unlock(&policy_rwlock); + return rc; +} + +/** * security_netif_sid - Obtain the SID for a network interface. * @name: interface name * @if_sid: interface SID