| Message ID | 20240925201106.16134-3-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/2] selinux: streamline selinux_nlmsg_lookup() | expand |
On Thu, Sep 26, 2024 at 6:11 AM Paul Moore <paul@paul-moore.com> wrote: > > Streamline the code in selinux_nlmsg_lookup() to improve the code flow, > readability, and remove the unnecessary local variables. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/nlmsgtab.c | 90 ++++++++++++++++--------------------- > 1 file changed, 39 insertions(+), 51 deletions(-) > > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c > index acc7d74b99d5..f68b4f493de6 100644 > --- a/security/selinux/nlmsgtab.c > +++ b/security/selinux/nlmsgtab.c > @@ -168,34 +168,12 @@ static int nlmsg_perm(u16 nlmsg_type, u32 *perm, const struct nlmsg_perm *tab, s > > int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) > { > - int err = 0; > - > - if (selinux_policycap_netlink_xperm()) { > - switch (sclass) { > - case SECCLASS_NETLINK_ROUTE_SOCKET: > - *perm = NETLINK_ROUTE_SOCKET__NLMSG; > - break; > - case SECCLASS_NETLINK_TCPDIAG_SOCKET: > - *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; > - break; > - case SECCLASS_NETLINK_XFRM_SOCKET: > - *perm = NETLINK_XFRM_SOCKET__NLMSG; > - break; > - case SECCLASS_NETLINK_AUDIT_SOCKET: > - *perm = NETLINK_AUDIT_SOCKET__NLMSG; > - break; > - /* While it is possible to add a similar permission to other > - * netlink classes, note that the extended permission value is > - * matched against the nlmsg_type field. Notably, > - * SECCLASS_NETLINK_GENERIC_SOCKET uses dynamic values for this > - * field, which means that it cannot be added as-is. > - */ > - default: > - err = -ENOENT; > - break; > - } > - return err; > - } > + /* While it is possible to add a similar permission to other netlink > + * classes, note that the extended permission value is matched against > + * the nlmsg_type field. Notably, SECCLASS_NETLINK_GENERIC_SOCKET uses > + * dynamic values for this field, which means that it cannot be added > + * as-is. > + */ > > switch (sclass) { > case SECCLASS_NETLINK_ROUTE_SOCKET: > @@ -205,42 +183,52 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) > * before updating the BUILD_BUG_ON() macro! > */ > BUILD_BUG_ON(RTM_MAX != (RTM_NEWTUNNEL + 3)); > - err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, > - sizeof(nlmsg_route_perms)); > - break; > > + if (selinux_policycap_netlink_xperm()) { > + *perm = NETLINK_ROUTE_SOCKET__NLMSG; > + return 0; > + } > + return nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, > + sizeof(nlmsg_route_perms)); > + break; > case SECCLASS_NETLINK_TCPDIAG_SOCKET: > - err = nlmsg_perm(nlmsg_type, perm, nlmsg_tcpdiag_perms, > - sizeof(nlmsg_tcpdiag_perms)); > + if (selinux_policycap_netlink_xperm()) { > + *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; > + return 0; > + } > + return nlmsg_perm(nlmsg_type, perm, nlmsg_tcpdiag_perms, > + sizeof(nlmsg_tcpdiag_perms)); > break; > - > case SECCLASS_NETLINK_XFRM_SOCKET: > /* If the BUILD_BUG_ON() below fails you must update the > * structures at the top of this file with the new mappings > * before updating the BUILD_BUG_ON() macro! > */ > BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT); > - err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, > - sizeof(nlmsg_xfrm_perms)); > - break; > > - case SECCLASS_NETLINK_AUDIT_SOCKET: > - if ((nlmsg_type >= AUDIT_FIRST_USER_MSG && > - nlmsg_type <= AUDIT_LAST_USER_MSG) || > - (nlmsg_type >= AUDIT_FIRST_USER_MSG2 && > - nlmsg_type <= AUDIT_LAST_USER_MSG2)) { > - *perm = NETLINK_AUDIT_SOCKET__NLMSG_RELAY; > - } else { > - err = nlmsg_perm(nlmsg_type, perm, nlmsg_audit_perms, > - sizeof(nlmsg_audit_perms)); > + if (selinux_policycap_netlink_xperm()) { > + *perm = NETLINK_XFRM_SOCKET__NLMSG; > + return 0; > } > + return nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, > + sizeof(nlmsg_xfrm_perms)); > break; > - > - /* No messaging from userspace, or class unknown/unhandled */ > - default: > - err = -ENOENT; > + case SECCLASS_NETLINK_AUDIT_SOCKET: > + if (selinux_policycap_netlink_xperm()) { > + *perm = NETLINK_XFRM_SOCKET__NLMSG; Should it be NETLINK_AUDIT_SOCKET__NLMSG here?
On Wed, Sep 25, 2024 at 8:48 PM Thiébaud Weksteen <tweek@google.com> wrote: > On Thu, Sep 26, 2024 at 6:11 AM Paul Moore <paul@paul-moore.com> wrote: > > > > Streamline the code in selinux_nlmsg_lookup() to improve the code flow, > > readability, and remove the unnecessary local variables. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/nlmsgtab.c | 90 ++++++++++++++++--------------------- > > 1 file changed, 39 insertions(+), 51 deletions(-) > > > > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c ... > > - /* No messaging from userspace, or class unknown/unhandled */ > > - default: > > - err = -ENOENT; > > + case SECCLASS_NETLINK_AUDIT_SOCKET: > > + if (selinux_policycap_netlink_xperm()) { > > + *perm = NETLINK_XFRM_SOCKET__NLMSG; > > Should it be NETLINK_AUDIT_SOCKET__NLMSG here? Yes it should, thanks!
On Thu, Sep 26, 2024 at 11:45 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Sep 25, 2024 at 8:48 PM Thiébaud Weksteen <tweek@google.com> wrote: > > On Thu, Sep 26, 2024 at 6:11 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > Streamline the code in selinux_nlmsg_lookup() to improve the code flow, > > > readability, and remove the unnecessary local variables. > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > security/selinux/nlmsgtab.c | 90 ++++++++++++++++--------------------- > > > 1 file changed, 39 insertions(+), 51 deletions(-) > > > > > > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c > > ... > > > > Should it be NETLINK_AUDIT_SOCKET__NLMSG here? > > Yes it should, thanks! > With the right constant: Tested-by: Thiébaud Weksteen <tweek@google.com>
On Sep 25, 2024 Paul Moore <paul@paul-moore.com> wrote: > > Streamline the code in selinux_nlmsg_lookup() to improve the code flow, > readability, and remove the unnecessary local variables. > > Tested-by: Thiébaud Weksteen <tweek@google.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/nlmsgtab.c | 90 ++++++++++++++++--------------------- > 1 file changed, 39 insertions(+), 51 deletions(-) Merged into selinux/dev with the correction from Thiébaud. -- paul-moore.com
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index acc7d74b99d5..f68b4f493de6 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -168,34 +168,12 @@ static int nlmsg_perm(u16 nlmsg_type, u32 *perm, const struct nlmsg_perm *tab, s int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) { - int err = 0; - - if (selinux_policycap_netlink_xperm()) { - switch (sclass) { - case SECCLASS_NETLINK_ROUTE_SOCKET: - *perm = NETLINK_ROUTE_SOCKET__NLMSG; - break; - case SECCLASS_NETLINK_TCPDIAG_SOCKET: - *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; - break; - case SECCLASS_NETLINK_XFRM_SOCKET: - *perm = NETLINK_XFRM_SOCKET__NLMSG; - break; - case SECCLASS_NETLINK_AUDIT_SOCKET: - *perm = NETLINK_AUDIT_SOCKET__NLMSG; - break; - /* While it is possible to add a similar permission to other - * netlink classes, note that the extended permission value is - * matched against the nlmsg_type field. Notably, - * SECCLASS_NETLINK_GENERIC_SOCKET uses dynamic values for this - * field, which means that it cannot be added as-is. - */ - default: - err = -ENOENT; - break; - } - return err; - } + /* While it is possible to add a similar permission to other netlink + * classes, note that the extended permission value is matched against + * the nlmsg_type field. Notably, SECCLASS_NETLINK_GENERIC_SOCKET uses + * dynamic values for this field, which means that it cannot be added + * as-is. + */ switch (sclass) { case SECCLASS_NETLINK_ROUTE_SOCKET: @@ -205,42 +183,52 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) * before updating the BUILD_BUG_ON() macro! */ BUILD_BUG_ON(RTM_MAX != (RTM_NEWTUNNEL + 3)); - err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, - sizeof(nlmsg_route_perms)); - break; + if (selinux_policycap_netlink_xperm()) { + *perm = NETLINK_ROUTE_SOCKET__NLMSG; + return 0; + } + return nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, + sizeof(nlmsg_route_perms)); + break; case SECCLASS_NETLINK_TCPDIAG_SOCKET: - err = nlmsg_perm(nlmsg_type, perm, nlmsg_tcpdiag_perms, - sizeof(nlmsg_tcpdiag_perms)); + if (selinux_policycap_netlink_xperm()) { + *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; + return 0; + } + return nlmsg_perm(nlmsg_type, perm, nlmsg_tcpdiag_perms, + sizeof(nlmsg_tcpdiag_perms)); break; - case SECCLASS_NETLINK_XFRM_SOCKET: /* If the BUILD_BUG_ON() below fails you must update the * structures at the top of this file with the new mappings * before updating the BUILD_BUG_ON() macro! */ BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT); - err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, - sizeof(nlmsg_xfrm_perms)); - break; - case SECCLASS_NETLINK_AUDIT_SOCKET: - if ((nlmsg_type >= AUDIT_FIRST_USER_MSG && - nlmsg_type <= AUDIT_LAST_USER_MSG) || - (nlmsg_type >= AUDIT_FIRST_USER_MSG2 && - nlmsg_type <= AUDIT_LAST_USER_MSG2)) { - *perm = NETLINK_AUDIT_SOCKET__NLMSG_RELAY; - } else { - err = nlmsg_perm(nlmsg_type, perm, nlmsg_audit_perms, - sizeof(nlmsg_audit_perms)); + if (selinux_policycap_netlink_xperm()) { + *perm = NETLINK_XFRM_SOCKET__NLMSG; + return 0; } + return nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, + sizeof(nlmsg_xfrm_perms)); break; - - /* No messaging from userspace, or class unknown/unhandled */ - default: - err = -ENOENT; + case SECCLASS_NETLINK_AUDIT_SOCKET: + if (selinux_policycap_netlink_xperm()) { + *perm = NETLINK_XFRM_SOCKET__NLMSG; + return 0; + } else if ((nlmsg_type >= AUDIT_FIRST_USER_MSG && + nlmsg_type <= AUDIT_LAST_USER_MSG) || + (nlmsg_type >= AUDIT_FIRST_USER_MSG2 && + nlmsg_type <= AUDIT_LAST_USER_MSG2)) { + *perm = NETLINK_AUDIT_SOCKET__NLMSG_RELAY; + return 0; + } + return nlmsg_perm(nlmsg_type, perm, nlmsg_audit_perms, + sizeof(nlmsg_audit_perms)); break; } - return err; + /* No messaging from userspace, or class unknown/unhandled */ + return -ENOENT; }
Streamline the code in selinux_nlmsg_lookup() to improve the code flow, readability, and remove the unnecessary local variables. Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/nlmsgtab.c | 90 ++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 51 deletions(-)