Re: [PATCH net] gtp: Require 'CAP_NET_ADMIN' when joining the gtp multicast group

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, the multicast group and GETPDP carry the
  same PDP-context data but are guarded by different checks.

--

gtp: Require 'CAP_NET_ADMIN' when joining the gtp multicast group

This commit marks the gtp generic netlink "gtp" multicast group with
GENL_MCAST_CAP_NET_ADMIN, so joining it requires CAP_NET_ADMIN in the user
namespace owning the network namespace. The goal is to stop unprivileged
users from reading PDP context notifications that GETPDP restricts to
CAP_NET_ADMIN holders.

> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> index 69fe5717846b5..e5ff1621ed620 100644
> --- a/drivers/net/gtp.c
> +++ b/drivers/net/gtp.c
> @@ -118,7 +118,8 @@ enum gtp_multicast_groups {
>  };
>  
>  static const struct genl_multicast_group gtp_genl_mcgrps[] = {
> -	[GTP_GENL_MCGRP] = { .name = GTP_GENL_MCGRP_NAME },
> +	[GTP_GENL_MCGRP] = { .name = GTP_GENL_MCGRP_NAME,
> +			     .flags = GENL_MCAST_CAP_NET_ADMIN, },

[Severity: Medium]
Does this make the multicast group check the same as the one on GETPDP?
The commit message says:

  GETPDP requires CAP_NET_ADMIN, but generic netlink lets any user join a
  multicast group by default

All of gtp_genl_ops (NEWPDP, DELPDP, GETPDP doit/dumpit, ECHOREQ) use
GENL_ADMIN_PERM. genl_family_rcv_msg() checks it like this:

net/netlink/genetlink.c:
	if ((op.flags & GENL_ADMIN_PERM) &&
	    !netlink_capable(skb, CAP_NET_ADMIN))
		return -EPERM;

netlink_capable() ends up in netlink_ns_capable(skb, &init_user_ns, cap),
so it needs CAP_NET_ADMIN in the initial user namespace.

genl_bind() checks the new flag against the netns owner instead:

net/netlink/genetlink.c:genl_bind() {
    ...
		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
			ret = -EPERM;
    ...
}

Take a gtp device in a netns owned by a non-init user namespace, where an
init-ns daemon installs PDP contexts through GTPA_NET_NS_FD (gtp_find_dev()
calls get_net_ns_by_fd()). gtp_tunnel_notify() multicasts to
dev_net(pctx->dev):

	ret = genlmsg_multicast_netns(&gtp_genl_family, dev_net(pctx->dev), msg,
				      0, GTP_GENL_MCGRP, GFP_ATOMIC);

Root inside that container would get -EPERM from GETPDP and the dump. It
could still join the "gtp" group and receive every NEWPDP, DELPDP and echo
response notification. These carry the MS and peer addresses, the TEIDs,
and the TID and flow label.

When the UDP socket lives in the host netns, container root has no other
way to see the TEIDs and peer addresses.

Genetlink has no init_user_ns scoped GENL_MCAST_* flag. Making the two
checks match would need a core change, or moving the ops to
GENL_UNS_ADMIN_PERM if userns scope is the intended policy.
net/ipv6/ioam6.c already pairs GENL_ADMIN_PERM ops with a
GENL_MCAST_CAP_NET_ADMIN group in the same way.

This patch does block fully unprivileged users. The commit message also
says "in the user namespace owning the network namespace".

Could the commit message say that the multicast check is still weaker than
the GETPDP check?

>  };
>  
>  static void pdp_context_delete(struct pdp_ctx *pctx);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021065750.2069795-1-anilkaushikwireless%40gmail.com