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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] An unprivileged local user can still receive gtp multicast
  notifications (PDP add/delete and echo responses) by joining the…

--

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

This patch marks the gtp generic netlink multicast group with
GENL_MCAST_CAP_NET_ADMIN in drivers/net/gtp.c and with uns-admin-perm in
the gtp netlink spec. The goal is to stop unprivileged users from
receiving the PDP context add/delete and echo response notifications,
which carry the same data as a GETPDP reply.

[ ... ]

> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> index 86c8c88b702cf..1dd32e979cf59 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: High]
Can an unprivileged user still get these notifications by joining the
group ID before the gtp family is registered?

The flag is only checked in genl_bind(), and only when a registered
family owns the requested group. If no family owns it, the loop matches
nothing and ret stays 0:

net/netlink/genetlink.c:genl_bind() {
    ...
	int ret = 0;
    ...
	idr_for_each_entry(&genl_fam_idr, family, id) {
    ...
		i = group - family->mcgrp_offset;
		if (i < 0 || i >= family->n_mcgrps)
			continue;
    ...
	}

	up_read(&cb_lock);
	return ret;
}

The genl socket is NL_CFG_F_NONROOT_RECV. For NETLINK_ADD_MEMBERSHIP,
netlink_setsockopt() only checks the ID against nlk->ngroups. So an
unprivileged NETLINK_GENERIC socket can join any free ID that fits in
the current bitmap.

When gtp is loaded later, the call chain is
genl_register_family()->genl_validate_assign_mc_groups()->
genl_allocate_reserve_groups(), which gives gtp the first free ID. After
that, only __netlink_change_ngroups() runs to resize the table. Sockets
already subscribed to that ID are not cleared or rechecked.

do_one_broadcast() only looks at the socket's group bitmap and netns.
So the earlier subscriber would receive what gtp_tunnel_notify() and the
echo response path send through genlmsg_multicast_netns(...,
GTP_GENL_MCGRP, ...).

The same user can also trigger the module load. CTRL_CMD_GETFAMILY does
not need GENL_ADMIN_PERM. ctrl_getfamily() calls request_module() for
"net-pf-16-proto-16-family-gtp", and gtp.c declares
MODULE_ALIAS_GENL_FAMILY("gtp").

The commit message says:

    This fix blocks the unprivileged users.

That claim fails when gtp is built as a module and is not yet loaded
when the user subscribes.

Could this also be closed in the genetlink core? One option is for
genl_validate_assign_mc_groups() to clear existing members of newly
allocated IDs with __netlink_clear_multicast_users(). Another is for
genl_bind() to reject IDs that no family owns. Either change would go on
top of this patch, not replace it.

The same gap seems to affect other modular groups that use the
GENL_MCAST_CAP_* flags, such as the psample one.

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