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

[PATCH net v3] 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>
---
v3: Address alternative device name issue found by sashiko
v2: https://lore.kernel.org/netdev/20260924162016.1258485-1-tung.quang.nguyen@est.tech/
v1: https://lore.kernel.org/netdev/20260923091057.375981-1-tung.quang.nguyen@est.tech/

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

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 05dcd2f9e887..f480fffca843 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -285,17 +285,42 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	bearer_id = MAX_BEARERS;
 	i = MAX_BEARERS;
 	while (i-- != 0) {
+		struct net_device *dev1, *dev2;
+		char *if_name;
+
 		b = rtnl_dereference(tn->bearer_list[i]);
 		if (!b) {
 			bearer_id = i;
 			continue;
 		}
 		if (!strcmp(name, b->name)) {
+duplicate:
 			errstr = "already enabled";
 			NL_SET_ERR_MSG(extack, "Already enabled");
 			goto rejected;
 		}
 
+		/* udp media can be used together with any media */
+		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
+		    m->type_id == TIPC_MEDIA_TYPE_UDP)
+			goto priority;
+
+		if_name = strchr((const char *)b->name, ':') + 1;
+		dev1 = __dev_get_by_name(net, if_name);
+		dev2 = __dev_get_by_name(net, b_names.if_name);
+		if (dev1 == dev2) {
+			/* Not allow eth and ib to attach to the same device */
+			if (b->media != m) {
+				errstr = "same dev for different media";
+				NL_SET_ERR_MSG(extack,
+					       "Same dev for different media");
+				goto rejected;
+			} else {
+				goto duplicate;
+			}
+		}
+
+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 v3] tipc: reject attaching bearer of different media to the same occupied device

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

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

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: the new occupied-device check in
  tipc_enable_bearer() (net/tipc/bearer.c:308-311) can be bypassed.
- [Low] The commit message does not match the code.
- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:309-311)
  compares two possibly-NULL results of __dev_get_by_name() without a…

--

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

This patch adds a check to tipc_enable_bearer(). It resolves the interface
names of the existing and the requested bearer to net_devices, and rejects
the new bearer when both resolve to the same device. The goal is to stop
the dev_refcnt leak when, for example, "eth:eth0" and "ib:eth0" are both
enabled.

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..f480fffca843e 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -285,17 +285,42 @@ static int tipc_enable_bearer(struct net *net, const char *name,

[ ... ]

> +		/* udp media can be used together with any media */
> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
> +		    m->type_id == TIPC_MEDIA_TYPE_UDP)
> +			goto priority;
> +
> +		if_name = strchr((const char *)b->name, ':') + 1;
> +		dev1 = __dev_get_by_name(net, if_name);
> +		dev2 = __dev_get_by_name(net, b_names.if_name);

[Severity: High]
Can this check be bypassed when the existing bearer's saved name no longer
resolves to the device the bearer is bound to?

dev1 comes from looking up b->name again. It does not use the device the
bearer actually holds (rtnl_dereference(b->media_ptr) for L2 media).
tipc_enable_l2_media() uses dev_get_by_name(), so a bearer can be enabled
by an altname. That altname can later be deleted while the bearer stays
enabled.

Deleting an altname goes through:

rtnl_linkprop()
  rtnl_alt_ifname()
    netdev_name_node_alt_destroy()
  netdev_state_change()

This sends only NETDEV_CHANGE, and only when the device is up. On
NETDEV_CHANGE, tipc_l2_device_event() only sets the bearer up or resets
it. Only NETDEV_UNREGISTER, NETDEV_CHANGENAME and a bad NETDEV_CHANGEMTU
disable it.

For example:

  ip link property add dev eth0 altname foo
  tipc bearer enable media eth dev foo
      (b1->name = "eth:foo", holds eth0, eth0->tipc_ptr = b1)
  ip link property del dev eth0 altname foo
      (b1 stays enabled)
  tipc bearer enable media ib dev eth0

In the new loop strcmp() does not match. dev1 =
__dev_get_by_name("foo") is NULL and dev2 is eth0, so the check is
skipped. tipc_enable_l2_media() then does:

	dev = dev_get_by_name(net, dev_name);
	...
	rcu_assign_pointer(dev->tipc_ptr, b);

This takes a second reference on eth0 and overwrites eth0->tipc_ptr with
b2. Isn't this the same dev_refcnt leak the commit message describes?

On NETDEV_UNREGISTER only b2 is disabled, because it is the one found
through dev->tipc_ptr. b1's reference and packet_type stay in place, so
netdev_wait_allrefs() waits until b1 is disabled by hand.

If b1 is disabled first instead, that clears dev->tipc_ptr while it points
to b2, and b2 is left orphaned in the same way.

TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM. CAP_NET_ADMIN in a user
namespace is enough to do this with dummy or veth devices.

The name-based lookup can also reject requests wrongly. Suppose the altname
"foo" is moved to eth1, or eth1 is renamed to "foo". A request for eth1
then gives dev1 == dev2 == eth1 and is rejected, although eth1 has no
bearer.

Would it be more robust to compare against the real binding? One option is
rtnl_dereference(b->media_ptr) for L2 media. Another is a check in
tipc_enable_l2_media() that returns -EBUSY when
rtnl_dereference(dev->tipc_ptr) is already set.

> +		if (dev1 == dev2) {

[Severity: Low]
What happens here when both lookups fail? __dev_get_by_name() returns NULL
for a name that does not resolve, and nothing checks for NULL before the
comparison.

Suppose an existing eth/ib bearer's saved altname has been deleted, so
dev1 is NULL as described above. If the request is for an interface that
does not exist, dev2 is also NULL. NULL == NULL then matches, and the
request is rejected with -EINVAL and "Already enabled" or "Same dev for
different media".

Without this check the request would reach tipc_enable_l2_media(), which
returns -ENODEV:

	dev = dev_get_by_name(net, dev_name);
	if (!dev)
		return -ENODEV;

Should a NULL dev1 or dev2 count as no match here?

> +			/* Not allow eth and ib to attach to the same device */
> +			if (b->media != m) {
> +				errstr = "same dev for different media";
> +				NL_SET_ERR_MSG(extack,
> +					       "Same dev for different media");
> +				goto rejected;
> +			} else {
> +				goto duplicate;
> +			}

[Severity: Low]
This isn't a bug, but the commit message only describes the different
media case:

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

The else branch also rejects a second bearer of the same media on the same
device when it is named by an altname. An example is "eth:eth0" plus
"eth:foo", where foo is an altname of eth0. This case fails with -EINVAL
and "Already enabled". Before this patch that configuration was accepted
and leaked dev_refcnt in the same way.

The only mention of this is the v3 changelog below the "---" line, which is
dropped when the patch is applied. Could the commit message describe this
case too?

> +		}
> +
> +priority:

[ ... ]

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

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

[Tung Quang Nguyen]

>Subject: Re: [PATCH net v3] tipc: reject attaching bearer of different media to
>the same occupied device
>
>Thank you for your contribution! Sashiko AI review found 3 potential
>issue(s) to consider.
>
>Critical: 0 · High: 1 · Medium: 0 · Low: 2
>
>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: the new occupied-device check in
>  tipc_enable_bearer() (net/tipc/bearer.c:308-311) can be bypassed.
>- [Low] The commit message does not match the code.
>- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:309-311)
>  compares two possibly-NULL results of __dev_get_by_name() without a…
>
>--
>
>tipc: reject attaching bearer of different media to the same occupied device
>
>This patch adds a check to tipc_enable_bearer(). It resolves the interface
>names of the existing and the requested bearer to net_devices, and rejects the
>new bearer when both resolve to the same device. The goal is to stop the
>dev_refcnt leak when, for example, "eth:eth0" and "ib:eth0" are both enabled.
>
>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..f480fffca843e 100644
>> --- a/net/tipc/bearer.c
>> +++ b/net/tipc/bearer.c
>> @@ -285,17 +285,42 @@ static int tipc_enable_bearer(struct net *net,
>> const char *name,
>
>[ ... ]
>
>> +		/* udp media can be used together with any media */
>> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
>> +		    m->type_id == TIPC_MEDIA_TYPE_UDP)
>> +			goto priority;
>> +
>> +		if_name = strchr((const char *)b->name, ':') + 1;
>> +		dev1 = __dev_get_by_name(net, if_name);
>> +		dev2 = __dev_get_by_name(net, b_names.if_name);
>
>[Severity: High]
>Can this check be bypassed when the existing bearer's saved name no longer
>resolves to the device the bearer is bound to?
>
>dev1 comes from looking up b->name again. It does not use the device the
>bearer actually holds (rtnl_dereference(b->media_ptr) for L2 media).
>tipc_enable_l2_media() uses dev_get_by_name(), so a bearer can be enabled
>by an altname. That altname can later be deleted while the bearer stays
>enabled.
>
>Deleting an altname goes through:
>
>rtnl_linkprop()
>  rtnl_alt_ifname()
>    netdev_name_node_alt_destroy()
>  netdev_state_change()
>
>This sends only NETDEV_CHANGE, and only when the device is up. On
>NETDEV_CHANGE, tipc_l2_device_event() only sets the bearer up or resets it.
>Only NETDEV_UNREGISTER, NETDEV_CHANGENAME and a bad
>NETDEV_CHANGEMTU disable it.
>
>For example:
>
>  ip link property add dev eth0 altname foo
>  tipc bearer enable media eth dev foo
>      (b1->name = "eth:foo", holds eth0, eth0->tipc_ptr = b1)
>  ip link property del dev eth0 altname foo
>      (b1 stays enabled)
>  tipc bearer enable media ib dev eth0
>
>In the new loop strcmp() does not match. dev1 =
>__dev_get_by_name("foo") is NULL and dev2 is eth0, so the check is skipped.
>tipc_enable_l2_media() then does:
>
>	dev = dev_get_by_name(net, dev_name);
>	...
>	rcu_assign_pointer(dev->tipc_ptr, b);
>
>This takes a second reference on eth0 and overwrites eth0->tipc_ptr with b2.
>Isn't this the same dev_refcnt leak the commit message describes?
>
>On NETDEV_UNREGISTER only b2 is disabled, because it is the one found
>through dev->tipc_ptr. b1's reference and packet_type stay in place, so
>netdev_wait_allrefs() waits until b1 is disabled by hand.
>
>If b1 is disabled first instead, that clears dev->tipc_ptr while it points to b2, and
>b2 is left orphaned in the same way.
>
>TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM. CAP_NET_ADMIN in a
>user namespace is enough to do this with dummy or veth devices.
>
>The name-based lookup can also reject requests wrongly. Suppose the
>altname "foo" is moved to eth1, or eth1 is renamed to "foo". A request for
>eth1 then gives dev1 == dev2 == eth1 and is rejected, although eth1 has no
>bearer.
>
>Would it be more robust to compare against the real binding? One option is
>rtnl_dereference(b->media_ptr) for L2 media

OK. I will use this check in v4.

--
pw-bot: cr