| Message ID | 20230704001136.2301645-6-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/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | warning | CHECK: No space is necessary after a cast |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
* Anjali Kulkarni <anjali.k.kulkarni@oracle.com> [691231 23:00]: > There were a couple of reasons for not allowing non-root users access > initially - one is there was some point no proper receive buffer > management in place for netlink multicast. But that should be long > fixed. See link below for more context. > > Second is that some of the messages may contain data that is root only. But > this should be handled with a finer granularity, which is being done at the > protocol layer. The only problematic protocols are nf_queue and the > firewall netlink. Hence, this restriction for non-root access was relaxed > for NETLINK_ROUTE initially: > https://lore.kernel.org/all/20020612013101.A22399@wotan.suse.de/ > > This restriction has also been removed for following protocols: > NETLINK_KOBJECT_UEVENT, NETLINK_AUDIT, NETLINK_SOCK_DIAG, > NETLINK_GENERIC, NETLINK_SELINUX. > > Since process connector messages are not sensitive (process fork, exit > notifications etc.), and anyone can read /proc data, we can allow non-root > access here. However, since process event notification is not the only > consumer of NETLINK_CONNECTOR, we can make this change even more > fine grained than the protocol level, by checking for multicast group > within the protocol. > > Allow non-root access for NETLINK_CONNECTOR via NL_CFG_F_NONROOT_RECV > but add new bind function cn_bind(), which allows non-root access only > for CN_IDX_PROC multicast group. > > Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> > --- > drivers/connector/cn_proc.c | 7 ------- > drivers/connector/connector.c | 19 +++++++++++++++++++ > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c > index dfc84d44f804..bb1fa5d66cf9 100644 > --- a/drivers/connector/cn_proc.c > +++ b/drivers/connector/cn_proc.c > @@ -410,12 +410,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, > !task_is_in_init_pid_ns(current)) > return; > > - /* Can only change if privileged. */ > - if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { > - err = EPERM; > - goto out; > - } > - > if (msg->len == sizeof(*pinput)) { > pinput = (struct proc_input *)msg->data; > mc_op = pinput->mcast_op; > @@ -467,7 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, > break; > } > > -out: This label is still in use from your changes in patch 3. > cn_proc_ack(err, msg->seq, msg->ack); > } > > diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c > index d1179df2b0ba..7f7b94f616a6 100644 > --- a/drivers/connector/connector.c > +++ b/drivers/connector/connector.c > @@ -166,6 +166,23 @@ static int cn_call_callback(struct sk_buff *skb) > return err; > } > > +/* > + * Allow non-root access for NETLINK_CONNECTOR family having CN_IDX_PROC > + * multicast group. > + */ > +static int cn_bind(struct net *net, int group) > +{ > + unsigned long groups = (unsigned long) group; > + > + if (ns_capable(net->user_ns, CAP_NET_ADMIN)) > + return 0; > + > + if (test_bit(CN_IDX_PROC - 1, &groups)) > + return 0; > + > + return -EPERM; > +} > + > static void cn_release(struct sock *sk, unsigned long *groups) > { > if (groups && test_bit(CN_IDX_PROC - 1, groups)) { > @@ -261,6 +278,8 @@ static int cn_init(void) > struct netlink_kernel_cfg cfg = { > .groups = CN_NETLINK_USERS + 0xf, > .input = cn_rx_skb, > + .flags = NL_CFG_F_NONROOT_RECV, > + .bind = cn_bind, > .release = cn_release, > }; > > -- > 2.41.0 >
> On Jul 6, 2023, at 1:16 PM, Liam Howlett <liam.howlett@oracle.com> wrote: > > * Anjali Kulkarni <anjali.k.kulkarni@oracle.com> [691231 23:00]: >> There were a couple of reasons for not allowing non-root users access >> initially - one is there was some point no proper receive buffer >> management in place for netlink multicast. But that should be long >> fixed. See link below for more context. >> >> Second is that some of the messages may contain data that is root only. But >> this should be handled with a finer granularity, which is being done at the >> protocol layer. The only problematic protocols are nf_queue and the >> firewall netlink. Hence, this restriction for non-root access was relaxed >> for NETLINK_ROUTE initially: >> https://lore.kernel.org/all/20020612013101.A22399@wotan.suse.de/ >> >> This restriction has also been removed for following protocols: >> NETLINK_KOBJECT_UEVENT, NETLINK_AUDIT, NETLINK_SOCK_DIAG, >> NETLINK_GENERIC, NETLINK_SELINUX. >> >> Since process connector messages are not sensitive (process fork, exit >> notifications etc.), and anyone can read /proc data, we can allow non-root >> access here. However, since process event notification is not the only >> consumer of NETLINK_CONNECTOR, we can make this change even more >> fine grained than the protocol level, by checking for multicast group >> within the protocol. >> >> Allow non-root access for NETLINK_CONNECTOR via NL_CFG_F_NONROOT_RECV >> but add new bind function cn_bind(), which allows non-root access only >> for CN_IDX_PROC multicast group. >> >> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> >> --- >> drivers/connector/cn_proc.c | 7 ------- >> drivers/connector/connector.c | 19 +++++++++++++++++++ >> 2 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c >> index dfc84d44f804..bb1fa5d66cf9 100644 >> --- a/drivers/connector/cn_proc.c >> +++ b/drivers/connector/cn_proc.c >> @@ -410,12 +410,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, >> !task_is_in_init_pid_ns(current)) >> return; >> >> - /* Can only change if privileged. */ >> - if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { >> - err = EPERM; >> - goto out; >> - } >> - >> if (msg->len == sizeof(*pinput)) { >> pinput = (struct proc_input *)msg->data; >> mc_op = pinput->mcast_op; >> @@ -467,7 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, >> break; >> } >> >> -out: > > This label is still in use from your changes in patch 3. Yes, will send updated patch with this fixed. > >> cn_proc_ack(err, msg->seq, msg->ack); >> } >> >> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c >> index d1179df2b0ba..7f7b94f616a6 100644 >> --- a/drivers/connector/connector.c >> +++ b/drivers/connector/connector.c >> @@ -166,6 +166,23 @@ static int cn_call_callback(struct sk_buff *skb) >> return err; >> } >> >> +/* >> + * Allow non-root access for NETLINK_CONNECTOR family having CN_IDX_PROC >> + * multicast group. >> + */ >> +static int cn_bind(struct net *net, int group) >> +{ >> + unsigned long groups = (unsigned long) group; >> + >> + if (ns_capable(net->user_ns, CAP_NET_ADMIN)) >> + return 0; >> + >> + if (test_bit(CN_IDX_PROC - 1, &groups)) >> + return 0; >> + >> + return -EPERM; >> +} >> + >> static void cn_release(struct sock *sk, unsigned long *groups) >> { >> if (groups && test_bit(CN_IDX_PROC - 1, groups)) { >> @@ -261,6 +278,8 @@ static int cn_init(void) >> struct netlink_kernel_cfg cfg = { >> .groups = CN_NETLINK_USERS + 0xf, >> .input = cn_rx_skb, >> + .flags = NL_CFG_F_NONROOT_RECV, >> + .bind = cn_bind, >> .release = cn_release, >> }; >> >> -- >> 2.41.0
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index dfc84d44f804..bb1fa5d66cf9 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -410,12 +410,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, !task_is_in_init_pid_ns(current)) return; - /* Can only change if privileged. */ - if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { - err = EPERM; - goto out; - } - if (msg->len == sizeof(*pinput)) { pinput = (struct proc_input *)msg->data; mc_op = pinput->mcast_op; @@ -467,7 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, break; } -out: cn_proc_ack(err, msg->seq, msg->ack); } diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index d1179df2b0ba..7f7b94f616a6 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -166,6 +166,23 @@ static int cn_call_callback(struct sk_buff *skb) return err; } +/* + * Allow non-root access for NETLINK_CONNECTOR family having CN_IDX_PROC + * multicast group. + */ +static int cn_bind(struct net *net, int group) +{ + unsigned long groups = (unsigned long) group; + + if (ns_capable(net->user_ns, CAP_NET_ADMIN)) + return 0; + + if (test_bit(CN_IDX_PROC - 1, &groups)) + return 0; + + return -EPERM; +} + static void cn_release(struct sock *sk, unsigned long *groups) { if (groups && test_bit(CN_IDX_PROC - 1, groups)) { @@ -261,6 +278,8 @@ static int cn_init(void) struct netlink_kernel_cfg cfg = { .groups = CN_NETLINK_USERS + 0xf, .input = cn_rx_skb, + .flags = NL_CFG_F_NONROOT_RECV, + .bind = cn_bind, .release = cn_release, };
There were a couple of reasons for not allowing non-root users access initially - one is there was some point no proper receive buffer management in place for netlink multicast. But that should be long fixed. See link below for more context. Second is that some of the messages may contain data that is root only. But this should be handled with a finer granularity, which is being done at the protocol layer. The only problematic protocols are nf_queue and the firewall netlink. Hence, this restriction for non-root access was relaxed for NETLINK_ROUTE initially: https://lore.kernel.org/all/20020612013101.A22399@wotan.suse.de/ This restriction has also been removed for following protocols: NETLINK_KOBJECT_UEVENT, NETLINK_AUDIT, NETLINK_SOCK_DIAG, NETLINK_GENERIC, NETLINK_SELINUX. Since process connector messages are not sensitive (process fork, exit notifications etc.), and anyone can read /proc data, we can allow non-root access here. However, since process event notification is not the only consumer of NETLINK_CONNECTOR, we can make this change even more fine grained than the protocol level, by checking for multicast group within the protocol. Allow non-root access for NETLINK_CONNECTOR via NL_CFG_F_NONROOT_RECV but add new bind function cn_bind(), which allows non-root access only for CN_IDX_PROC multicast group. Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> --- drivers/connector/cn_proc.c | 7 ------- drivers/connector/connector.c | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 7 deletions(-)