Messages in this thread | | | Date | Thu, 1 Jun 2023 14:20:55 -0400 | From | "Liam R. Howlett" <> | Subject | Re: [PATCH v5 6/6] connector/cn_proc: Allow non-root users access |
| |
* 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 | 14 ++++++++++++++ > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c > index 35bec1fd7ee0..046a8c1d8577 100644 > --- a/drivers/connector/cn_proc.c > +++ b/drivers/connector/cn_proc.c > @@ -408,12 +408,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; > @@ -460,7 +454,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..193d3056de64 100644 > --- a/drivers/connector/connector.c > +++ b/drivers/connector/connector.c > @@ -166,6 +166,18 @@ static int cn_call_callback(struct sk_buff *skb) > return err; > } >
Should there be a comment here about non-root access?
> +static int cn_bind(struct net *net, int group) > +{ > + unsigned long groups = 0; > + groups = (unsigned long) group;
I don't understand why you have this cast on a second line or why you zero groups before the assignment? It would all fit on one line.
> + > + if (ns_capable(net->user_ns, CAP_NET_ADMIN)) > + return 0;
New line here please
> + if (test_bit(CN_IDX_PROC - 1, &groups)) > + return 0;
New line here please
> + return -EPERM; > +} > + > static void cn_release(struct sock *sk, unsigned long *groups) > { > if (groups && test_bit(CN_IDX_PROC - 1, groups)) { > @@ -261,6 +273,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.40.0 >
| |