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

[PATCH net v2] 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>
---
v2: only disallow eth and ib to attach to the same device. This avoids
breaking udp-eth or udp-ib combination on the same device as found by sashiko.

v1: https://lore.kernel.org/netdev/20260923091057.375981-1-tung.quang.nguyen@est.tech/

 net/tipc/bearer.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 05dcd2f9e887..2c0ceee4aa6b 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,21 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 			goto rejected;
 		}
 
+		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
+		    !strcmp(b_names.media_name, "udp"))
+			goto priority;
+
+		/* Not allow eth and ib to attach to the same device */
+		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;
+		}
+
+priority:
 		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 v2] 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: 0 · Low: 1

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: two bearers can still be attached to one
  net_device, so the dev_refcnt leak the commit claims to fix can still…
- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:300-301)
  detects UDP in two different ways.

--

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

This commit adds a check to tipc_enable_bearer(). The check rejects a new
eth or ib bearer when an existing bearer of a different media type uses
the same interface name. The goal is to stop a dev_refcnt leak where a
second bearer takes another dev_hold() in tipc_enable_l2_media() and
replaces the first one. UDP bearers are exempt from the check.

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..2c0ceee4aa6b9 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c

[ ... ]

> @@ -296,6 +297,21 @@ static int tipc_enable_bearer(struct net *net, const char *name,
>  			goto rejected;
>  		}
>  
> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
> +		    !strcmp(b_names.media_name, "udp"))
> +			goto priority;

[Severity: Low]
This isn't a bug, but the new bearer's media is already known at this
point, because earlier in tipc_enable_bearer() it was looked up with:

	m = tipc_media_find(b_names.media_name);

Could this use m->type_id == TIPC_MEDIA_TYPE_UDP instead of comparing
against the "udp" string? That would match the check on the existing
bearer and the other UDP checks in bearer.c.

For the same reason, the media difference test below could be written as
b->media != m.

> +
> +		/* Not allow eth and ib to attach to the same device */
> +		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]
Does this string comparison catch every case where two bearers end up on
the same net_device?

tipc_enable_l2_media() finds the device with dev_get_by_name(). That
lookup also matches altnames, because netdev_name_node_alt_create() adds
them to the same per-netns name hash. Consider this sequence:

  enable bearer "eth:dummy0"
  ip link property add dev dummy0 altname foo
  enable bearer "ib:foo"   (or "eth:foo")

The if_name strings differ, so this check passes. The earlier "already
enabled" check on the full name passes too. This check never looks at the
same-media case ("eth:dummy0" plus "eth:foo"), and that case needs only
the eth media.

The second attach then takes another device reference and overwrites
dev->tipc_ptr without checking it:

net/tipc/bearer.c:tipc_enable_l2_media() {
    ...
	dev = dev_get_by_name(net, dev_name);
    ...
	rcu_assign_pointer(dev->tipc_ptr, b);
	return 0;
}

On NETDEV_UNREGISTER, tipc_l2_device_event() disables only the bearer
found in dev->tipc_ptr. tipc_disable_l2_media() then clears it:

	RCU_INIT_POINTER(dev->tipc_ptr, NULL);

As a result, the NETDEV_UNREGISTER that netdev_wait_allrefs_any()
rebroadcasts gets NOTIFY_DONE. The other bearer's device reference is
never dropped.

Would ip link del dummy0 then loop forever in netdev_wait_allrefs_any()?
Disabling the second bearer first gives the same result, because the
first bearer is then invisible to the notifier.

An altname can also be added after the first bearer already exists. Any
name-based check at enable time would miss that case.

TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM, and adding an altname needs
only CAP_NET_ADMIN in the netns's user namespace. That makes this look
reachable from an unprivileged user namespace.

Would it be more robust to replace this string check with an occupancy
check in tipc_enable_l2_media(), right after dev_get_by_name()? For
example, when rtnl_dereference(dev->tipc_ptr) is non-NULL, call dev_put()
and return -EBUSY. UDP bearers never set dev->tipc_ptr, so the udp
special case above would no longer be needed either.

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

[ ... ]

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

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

[Tung Quang Nguyen]

>Subject: Re: [PATCH net v2] tipc: reject attaching bearer of different media to
>the same occupied device
>
>Thank you for your contribution! Sashiko AI review found 2 potential
>issue(s) to consider.
>
>Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
>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: two bearers can still be attached to one
>  net_device, so the dev_refcnt leak the commit claims to fix can still…
>- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:300-301)
>  detects UDP in two different ways.
>
>--
>
>tipc: reject attaching bearer of different media to the same occupied device
>
>This commit adds a check to tipc_enable_bearer(). The check rejects a new eth
>or ib bearer when an existing bearer of a different media type uses the same
>interface name. The goal is to stop a dev_refcnt leak where a second bearer
>takes another dev_hold() in tipc_enable_l2_media() and replaces the first one.
>UDP bearers are exempt from the check.
>
>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..2c0ceee4aa6b9 100644
>> --- a/net/tipc/bearer.c
>> +++ b/net/tipc/bearer.c
>
>[ ... ]
>
>> @@ -296,6 +297,21 @@ static int tipc_enable_bearer(struct net *net, const
>char *name,
>>  			goto rejected;
>>  		}
>>
>> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
>> +		    !strcmp(b_names.media_name, "udp"))
>> +			goto priority;
>
>[Severity: Low]
>This isn't a bug, but the new bearer's media is already known at this point,
>because earlier in tipc_enable_bearer() it was looked up with:
>
>	m = tipc_media_find(b_names.media_name);
>
>Could this use m->type_id == TIPC_MEDIA_TYPE_UDP instead of comparing
>against the "udp" string? That would match the check on the existing bearer
>and the other UDP checks in bearer.c.
>
>For the same reason, the media difference test below could be written as
>b->media != m.

Yes. I will use this comparison  in v3.

>
>> +
>> +		/* Not allow eth and ib to attach to the same device */
>> +		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]
>Does this string comparison catch every case where two bearers end up on the
>same net_device?
>
>tipc_enable_l2_media() finds the device with dev_get_by_name(). That lookup
>also matches altnames, because netdev_name_node_alt_create() adds them
>to the same per-netns name hash. Consider this sequence:
>
>  enable bearer "eth:dummy0"
>  ip link property add dev dummy0 altname foo
>  enable bearer "ib:foo"   (or "eth:foo")
>
>The if_name strings differ, so this check passes. The earlier "already enabled"
>check on the full name passes too. This check never looks at the same-media
>case ("eth:dummy0" plus "eth:foo"), and that case needs only the eth media.
>

I will address this issue in v3.

pw-bot: cr

>The second attach then takes another device reference and overwrites
>dev->tipc_ptr without checking it:
>
>net/tipc/bearer.c:tipc_enable_l2_media() {
>    ...
>	dev = dev_get_by_name(net, dev_name);
>    ...
>	rcu_assign_pointer(dev->tipc_ptr, b);
>	return 0;
>}
>
>On NETDEV_UNREGISTER, tipc_l2_device_event() disables only the bearer
>found in dev->tipc_ptr. tipc_disable_l2_media() then clears it:
>
>	RCU_INIT_POINTER(dev->tipc_ptr, NULL);
>
>As a result, the NETDEV_UNREGISTER that netdev_wait_allrefs_any()
>rebroadcasts gets NOTIFY_DONE. The other bearer's device reference is never
>dropped.
>
>Would ip link del dummy0 then loop forever in netdev_wait_allrefs_any()?
>Disabling the second bearer first gives the same result, because the first bearer
>is then invisible to the notifier.
>
>An altname can also be added after the first bearer already exists. Any name-
>based check at enable time would miss that case.
>
>TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM, and adding an
>altname needs only CAP_NET_ADMIN in the netns's user namespace. That
>makes this look reachable from an unprivileged user namespace.
>
>Would it be more robust to replace this string check with an occupancy check
>in tipc_enable_l2_media(), right after dev_get_by_name()? For example, when
>rtnl_dereference(dev->tipc_ptr) is non-NULL, call dev_put() and return -EBUSY.
>UDP bearers never set dev->tipc_ptr, so the udp special case above would no
>longer be needed either.
>
>> +			errstr = "same device for different media";
>> +			NL_SET_ERR_MSG(extack,
>> +				       "Same device for different media");
>> +			goto rejected;
>> +		}
>
>[ ... ]
>
>--
>Sashiko AI review · https://netdev-
>ai.bots.linux.dev/sashiko/#/patchset/20260924162016.1258485-1-
>tung.quang.nguyen%40est.tech