[PATCH net] tipc: reject attaching bearer of different media to the same occupied device

[PATCH net] tipc: reject attaching bearer of different media to the same occupied device

[Tung Nguyen]

Currently, two bearers of different media types can be added to the
same device (e.g., "eth:eth0" and "ib:eth0"). This causes a leak
in device->dev_refcnt as follows:

tipc_enable_l2_media()
 dev_get_by_name()
  dev_hold()

As a result, device->dev_refcnt is incremented twice, but the second
bearer replaces the first one.

Fix this by rejecting a bearer with a different media type when the
device is already occupied.

Fixes: cb30a63384bc ("tipc: refactor function tipc_enable_bearer()")
Reported-by: Jiakai Xu <xujiakai24@mails.ucas.ac.cn>
Closes: http://lore.kernel.org/netdev/20260918013509.1853952-1-xujiakai24@mails.ucas.ac.cn/
Signed-off-by: Tung Nguyen <tung.quang.nguyen@est.tech>
---
 net/tipc/bearer.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 05dcd2f9e887..1fb54a588c8d 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -258,6 +258,7 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	int bearer_id = 0;
 	int res = -EINVAL;
 	char *errstr = "";
+	char *if_name;
 	u32 i;
 
 	if (!bearer_name_validate(name, &b_names)) {
@@ -296,6 +297,15 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 			goto rejected;
 		}
 
+		if_name = strchr((const char *)b->name, ':') + 1;
+		if (!strcmp(if_name, b_names.if_name) &&
+		    strcmp(b->media->name, b_names.media_name)) {
+			errstr = "same device for different media";
+			NL_SET_ERR_MSG(extack,
+				       "Same device for different media");
+			goto rejected;
+		}
+
 		if (b->priority == prio &&
 		    (++with_this_prio > 2)) {
 			pr_warn("Bearer <%s>: already 2 bearers with priority %u\n",
-- 
2.43.0

Re: [PATCH net] tipc: reject attaching bearer of different media to the same occupied device

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete.
- [Medium] The new check runs for every media type, but for UDP media the
  text after ':' is not a device name.

--

tipc: reject attaching bearer of different media to the same occupied device

This patch adds a check to tipc_enable_bearer(). A new bearer is rejected
when an existing bearer has the same interface-name suffix but a different
media type. The goal is to fix a dev_refcnt leak: two L2 bearers such as
"eth:eth0" and "ib:eth0" attach to the same net_device, and the second one
overwrites dev->tipc_ptr.

Closes: http://lore.kernel.org/netdev/20260918013509.1853952-1-xujiakai24@mails.ucas.ac.cn/

> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 05dcd2f9e887a..1fb54a588c8df 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c

[ ... ]

> @@ -296,6 +297,15 @@ static int tipc_enable_bearer(struct net *net, const char *name,
>  			goto rejected;
>  		}
>  
> +		if_name = strchr((const char *)b->name, ':') + 1;
> +		if (!strcmp(if_name, b_names.if_name) &&
> +		    strcmp(b->media->name, b_names.media_name)) {

[Severity: High]
Can a string comparison reliably tell whether two bearers share a device?

tipc_enable_l2_media() does not use the raw suffix as-is. It resolves it
with dev_get_by_name():

net/tipc/bearer.c:tipc_enable_l2_media() {
    char *dev_name = strchr((const char *)b->name, ':') + 1;
    ...
    dev = dev_get_by_name(net, dev_name);
    ...
    rcu_assign_pointer(dev->tipc_ptr, b);
}

The lookup goes through
dev_get_by_name()->dev_get_by_name_rcu()->netdev_name_node_lookup_rcu().
It also matches altnames, because netdev_name_node_alt_create() adds them
to the same per-netns name hash.

Say eth0 has the altname "myalt". Enabling "eth:eth0" and then "ib:myalt"
(or "eth:myalt") passes both the full-name check and this new strcmp().
Both bearers then attach to the same net_device.

Each enable takes a device reference through dev_get_by_name() and calls
dev_add_pack(). The second enable then overwrites dev->tipc_ptr without
checking whether it is already set.

On NETDEV_UNREGISTER, tipc_l2_device_event() only acts on the bearer
stored in dev->tipc_ptr:

    b = rtnl_dereference(dev->tipc_ptr);
    ...
    case NETDEV_UNREGISTER:
    case NETDEV_CHANGENAME:
        bearer_disable(net, b);

tipc_disable_l2_media() then clears dev->tipc_ptr. The first bearer keeps
its device reference and its packet_type, and later NETDEV_UNREGISTER
rebroadcasts cannot find it.

Would unregister_netdevice() then hang waiting for the device to become
free? That is the same dev_refcnt leak the commit message describes.

TIPC_NL_BEARER_ENABLE uses GENL_UNS_ADMIN_PERM, and adding an altname
only needs CAP_NET_ADMIN in the netns. An unprivileged user could
therefore do this from inside their own user and network namespace.

Would it be more robust to check device identity instead of names? For
example, tipc_enable_l2_media() could call dev_put() and return -EBUSY
when rtnl_dereference(dev->tipc_ptr) is already non-NULL. That would
replace the name comparison in tipc_enable_bearer().

> +			errstr = "same device for different media";
> +			NL_SET_ERR_MSG(extack,
> +				       "Same device for different media");
> +			goto rejected;
> +		}

[Severity: Medium]
Does this check also reject valid UDP bearers?

For UDP media, the text after ':' is not a device name, only a label.
tipc_udp_enable() never reads the suffix of b->name, and it never sets
dev->tipc_ptr. It picks its device from TIPC_NLA_UDP_LOCAL through
__ip_dev_find(), or through __dev_get_by_index() and ipv6_dev_find() for
IPv6.

With this patch, enabling "udp:eth0" while "eth:eth0" exists (or the
other way round) fails with -EINVAL and "Same device for different
media". The two bearers don't share an L2 device reference or
dev->tipc_ptr, and this combination worked before the patch.

Would a device-occupancy check inside tipc_enable_l2_media() avoid this
false rejection?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923091057.375981-1-tung.quang.nguyen%40est.tech