| Message ID | 20230329182543.1161480-7-anjali.k.kulkarni@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Process connector bug fixes & enhancements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Wed, 29 Mar 2023 11:25:42 -0700 Anjali Kulkarni wrote: > A new field perm_groups is added in netlink_sock to store the protocol's > multicast group access permissions. This is to allow for a more fine > grained access control than just at the protocol level. These > permissions can be supplied by the protocol via the netlink_kernel_cfg. > A new function netlink_multicast_allowed() is added, which checks if > the protocol's multicast group has non-root access before allowing bind. Is there a reason this is better than implementing .bind in the connector family and filtering there?
> On Mar 30, 2023, at 11:39 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 29 Mar 2023 11:25:42 -0700 Anjali Kulkarni wrote: >> A new field perm_groups is added in netlink_sock to store the protocol's >> multicast group access permissions. This is to allow for a more fine >> grained access control than just at the protocol level. These >> permissions can be supplied by the protocol via the netlink_kernel_cfg. >> A new function netlink_multicast_allowed() is added, which checks if >> the protocol's multicast group has non-root access before allowing bind. > > Is there a reason this is better than implementing .bind > in the connector family and filtering there? Are you suggesting adding something like a new struct proto_ops for the connector family? I have not looked into that, though that would seem like a lot of work, and also I have not seen any infra structure to call into protocol specific bind from netlink bind?
On Fri, 31 Mar 2023 17:00:27 +0000 Anjali Kulkarni wrote: > > Is there a reason this is better than implementing .bind > > in the connector family and filtering there? > > Are you suggesting adding something like a new struct proto_ops for > the connector family? I have not looked into that, though that would > seem like a lot of work, and also I have not seen any infra structure > to call into protocol specific bind from netlink bind? Where you're adding a release callback in patch 2 - there's a bind callback already three lines above. What am I missing?
> On Mar 31, 2023, at 10:24 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 31 Mar 2023 17:00:27 +0000 Anjali Kulkarni wrote: >>> Is there a reason this is better than implementing .bind >>> in the connector family and filtering there? >> >> Are you suggesting adding something like a new struct proto_ops for >> the connector family? I have not looked into that, though that would >> seem like a lot of work, and also I have not seen any infra structure >> to call into protocol specific bind from netlink bind? > > Where you're adding a release callback in patch 2 - there's a bind > callback already three lines above. What am I missing? Ah yes, that one is actually meant to be used for adding(bind) and deleting(unbind) multicast group memberships. So it is also called from setsockopt() - so I think just checking for root access permission changes the semantics of what it is meant to be used for? Besides we would need to change some of that ordering there (check for permissions & netlink_bind call) and changing it for all users of netlink might not be a good idea…? Anjali
On Fri, 31 Mar 2023 17:48:18 +0000 Anjali Kulkarni wrote: > > On Mar 31, 2023, at 10:24 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 31 Mar 2023 17:00:27 +0000 Anjali Kulkarni wrote: > >> Are you suggesting adding something like a new struct proto_ops for > >> the connector family? I have not looked into that, though that would > >> seem like a lot of work, and also I have not seen any infra structure > >> to call into protocol specific bind from netlink bind? > > > > Where you're adding a release callback in patch 2 - there's a bind > > callback already three lines above. What am I missing? > Ah yes, that one is actually meant to be used for adding(bind) and > deleting(unbind) multicast group memberships. So it is also called > from setsockopt() - so I think just checking for root access > permission changes the semantics of what it is meant to be used for? > Besides we would need to change some of that ordering there (check > for permissions & netlink_bind call) and changing it for all users of > netlink might not be a good idea…? AFAICT genetlink uses that callback in the way I'm suggesting already (see genl_bind()) so if you can spot a bug or a problem - we need to fix it :S
> On Mar 31, 2023, at 11:13 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 31 Mar 2023 17:48:18 +0000 Anjali Kulkarni wrote: >>> On Mar 31, 2023, at 10:24 AM, Jakub Kicinski <kuba@kernel.org> wrote: >>> On Fri, 31 Mar 2023 17:00:27 +0000 Anjali Kulkarni wrote: >>>> Are you suggesting adding something like a new struct proto_ops for >>>> the connector family? I have not looked into that, though that would >>>> seem like a lot of work, and also I have not seen any infra structure >>>> to call into protocol specific bind from netlink bind? >>> >>> Where you're adding a release callback in patch 2 - there's a bind >>> callback already three lines above. What am I missing? >> Ah yes, that one is actually meant to be used for adding(bind) and >> deleting(unbind) multicast group memberships. So it is also called >> from setsockopt() - so I think just checking for root access >> permission changes the semantics of what it is meant to be used for? >> Besides we would need to change some of that ordering there (check >> for permissions & netlink_bind call) and changing it for all users of >> netlink might not be a good idea…? > > AFAICT genetlink uses that callback in the way I'm suggesting already > (see genl_bind()) so if you can spot a bug or a problem - we need to > fix it :S Ok, I will take a look and make the change. Anjali
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 05a316aa93b4..253cbcd7a290 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -46,6 +46,7 @@ void netlink_table_ungrab(void); struct netlink_kernel_cfg { unsigned int groups; unsigned int flags; + long unsigned perm_groups; void (*input)(struct sk_buff *skb); struct mutex *cb_mutex; int (*bind)(struct net *net, int group); diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index dc7880055705..f31173d28dd0 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -679,6 +679,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol, void (*unbind)(struct net *net, int group); void (*release)(struct sock *sock, unsigned long *groups); int err = 0; + unsigned long perm_groups; sock->state = SS_UNCONNECTED; @@ -706,6 +707,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol, bind = nl_table[protocol].bind; unbind = nl_table[protocol].unbind; release = nl_table[protocol].release; + perm_groups = nl_table[protocol].perm_groups; netlink_unlock_table(); if (err < 0) @@ -722,6 +724,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol, nlk->netlink_bind = bind; nlk->netlink_unbind = unbind; nlk->netlink_release = release; + nlk->perm_groups = perm_groups; out: return err; @@ -938,6 +941,20 @@ bool netlink_net_capable(const struct sk_buff *skb, int cap) } EXPORT_SYMBOL(netlink_net_capable); +static inline bool netlink_multicast_allowed(unsigned long perm_groups, + unsigned long groups) +{ + int group; + + for (group = 0; group < BITS_PER_TYPE(u32); group++) { + if (test_bit(group, &groups)) { + if (!test_bit(group, &perm_groups)) + return false; + } + } + return true; +} + static inline int netlink_allowed(const struct socket *sock, unsigned int flag) { return (nl_table[sock->sk->sk_protocol].flags & flag) || @@ -1023,8 +1040,11 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, /* Only superuser is allowed to listen multicasts */ if (groups) { - if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) - return -EPERM; + if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) { + if (!netlink_multicast_allowed(nlk->perm_groups, + groups)) + return -EPERM; + } err = netlink_realloc_groups(sk); if (err) return err; @@ -2124,6 +2144,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module, nl_table[unit].unbind = cfg->unbind; nl_table[unit].release = cfg->release; nl_table[unit].flags = cfg->flags; + nl_table[unit].perm_groups = cfg->perm_groups; if (cfg->compare) nl_table[unit].compare = cfg->compare; } diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h index 054335a34804..b7880254c716 100644 --- a/net/netlink/af_netlink.h +++ b/net/netlink/af_netlink.h @@ -29,6 +29,7 @@ struct netlink_sock { u32 flags; u32 subscriptions; u32 ngroups; + unsigned long perm_groups; unsigned long *groups; unsigned long state; size_t max_recvmsg_len; @@ -62,6 +63,7 @@ struct netlink_table { struct listeners __rcu *listeners; unsigned int flags; unsigned int groups; + unsigned long perm_groups; struct mutex *cb_mutex; struct module *module; int (*bind)(struct net *net, int group);
A new field perm_groups is added in netlink_sock to store the protocol's multicast group access permissions. This is to allow for a more fine grained access control than just at the protocol level. These permissions can be supplied by the protocol via the netlink_kernel_cfg. A new function netlink_multicast_allowed() is added, which checks if the protocol's multicast group has non-root access before allowing bind. Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> --- include/linux/netlink.h | 1 + net/netlink/af_netlink.c | 25 +++++++++++++++++++++++-- net/netlink/af_netlink.h | 2 ++ 3 files changed, 26 insertions(+), 2 deletions(-)