Re: [PATCH net-next v3 2/2] net: bridge: fail link info that does not fit in a netlink attribute

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] New error path discards already-consumed CFM status.
- [Low] Missing `Fixes:` tag / stable routing information for a user-
  visible uAPI bug fix.
- [Low] The single `nla_nest_too_large` label emits "AF_SPEC info too
  large, use per-object dumps (e.g. RTM_GETVLAN)" for all three overflow…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it
  undermines the notification behaviour the commit message describes:…
- [Medium] This problem wasn't introduced by this patch, but it sits
  directly in the rewritten function: in `br_fill_ifinfo()` the return…

--

net: bridge: fail link info that does not fit in a netlink attribute

br_fill_ifinfo() now finalizes the IFLA_AF_SPEC, IFLA_BRIDGE_CFM and
IFLA_BRIDGE_MST nests with nla_nest_end_safe() and returns -E2BIG with an
extack message when a nest payload exceeds what the u16 nla_len can
describe.  br_fill_ifinfo() gains an extack argument, passed through from
br_getlink() and NULL from br_info_notify().

A few questions on the change below.

> nla_len is a u16, and a port with enough VLANs, tunnels or MST entries
> makes the IFLA_AF_SPEC nest wrap, so userspace reads garbage. Same for
> the inner MST and CFM nests.

[Severity: Low]
Since the commit message describes pre-existing user-visible breakage
("userspace reads garbage") rather than a cleanup, should this carry a
Fixes: tag so it gets routed to stable?

The overflow does look reachable: __get_vlan_tinfo_size() in
br_netlink_tunnel.c charges 28 bytes per tunnel mapping

	return nla_total_size(0) + nla_total_size(sizeof(u32)) +
	       nla_total_size(sizeof(u16)) + nla_total_size(sizeof(u16));

so roughly 4094 VLANs with non-consecutive tunnel IDs push the AF_SPEC
payload past U16_MAX, and br_info_notify() allocates an skb of exactly
br_nlmsg_size() bytes for it.

Candidate anchors would be the commit that introduced the AF_SPEC nesting
(b6c02ef54913) and the later CFM/MST reporting additions.

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 855a46aec3a8..fbd91b86d426 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c

[ ... ]

> @@ -588,7 +588,8 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>  				goto nla_put_failure;
>  		}
>  
> -		nla_nest_end(skb, cfm_nest);
> +		if (nla_nest_end_safe(skb, cfm_nest) < 0)
> +			goto nla_nest_too_large;
>  	}

[Severity: Medium]
Can this new exit throw away CFM status that has already been consumed?

br_getlink() calls br_fill_ifinfo() with getlink = true, and
br_cfm_status_fill_info() clears the "seen" indications as it serializes
each record:

net/bridge/br_cfm_netlink.c:br_cfm_status_fill_info() {
	...
	if (getlink) {
		/* Clear all 'seen' indications */
		mep->status.opcode_unexp_seen = false;
		mep->status.version_unexp_seen = false;
		mep->status.rx_level_low_seen = false;
	}
	...
		if (getlink) { /* Only clear if this is a GETLINK */
			/* Clear all 'seen' indications */
			peer_mep->cc_status.seen = false;
			peer_mep->cc_status.tlv_seen = false;
			peer_mep->cc_status.seq_unexp_seen = false;
		}
	...
}

Nothing bounds the peer MEP count so that the IFLA_BRIDGE_CFM nest stays
below U16_MAX (each peer status record is around 76 bytes, so roughly 860
peers overflow it).  The nest check then jumps to nla_nest_too_large, which
cancels the whole message and returns -E2BIG, after the flags were already
cleared.

A retry returns -E2BIG again, so that generation of defect and
unexpected-opcode information is gone.  Before this patch the malformed
message was at least delivered.  The existing nla_put_failure path has the
same clear-then-cancel shape; would validating the nest size before
consuming the state be preferable to adding another cancel-after-clear
exit?

> @@ -608,20 +609,27 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>  		if (err)
>  			goto nla_put_failure;
>  
> -		nla_nest_end(skb, mst_nest);
> +		if (nla_nest_end_safe(skb, mst_nest) < 0)
> +			goto nla_nest_too_large;
>  	}
>  
>  done:
>  	if (af) {
> -		if (nlmsg_get_pos(skb) - (void *)af > nla_attr_size(0))
> -			nla_nest_end(skb, af);
> -		else
> +		if (nla_nest_end_safe(skb, af) < 0)
> +			goto nla_nest_too_large;
> +		if (!nla_len(af))
>  			nla_nest_cancel(skb, af);
>  	}
>  
>  	nlmsg_end(skb, nlh);
>  	return 0;
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but it lands right in the
rewritten function: can the done: block finalize a truncated AF_SPEC nest
and report success?

Earlier in br_fill_ifinfo() the VLAN fill result is clobbered:

	if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
		err = br_fill_ifvlaninfo_compressed(skb, vg);
	else
		err = br_fill_ifvlaninfo(skb, vg);

	if (port && test_bit(BR_VLAN_TUNNEL_BIT, &port->flags))
		err = br_fill_vlan_tunnel_info(skb, vg);
	rcu_read_unlock();
	if (err)
		goto nla_put_failure;

br_fill_vlan_tunnel_info() in br_netlink_tunnel.c returns 0 without writing
anything when no VLAN in the group has a tunnel_dst:

	if (!v->tinfo.tunnel_dst)
		continue;
	...
	return 0;

vlan_tunnel is a port flag that can be set without any per-VLAN tunnel
mapping, so an -EMSGSIZE from the VLAN fill is overwritten with 0, the
goto nla_put_failure is skipped, and control reaches done: which ends the
nest and returns 0.  In a dump rtnl_bridge_getlink() then does

	idx++;

and moves past the device, so the missing VLANs are never emitted.

> +nla_nest_too_large:
> +	NL_SET_ERR_MSG_MOD(extack,
> +			   "AF_SPEC info too large, use per-object dumps (e.g. RTM_GETVLAN)");
> +	nlmsg_cancel(skb, nlh);
> +	return -E2BIG;
> +

[Severity: Low]
All three overflow sites share this label, so an oversized
IFLA_BRIDGE_CFM nest also tells userspace to retry with RTM_GETVLAN.

The bridge only registers PF_BRIDGE handlers for the VLAN and MDB commands:

net/bridge/br_vlan.c:
	{THIS_MODULE, PF_BRIDGE, RTM_GETVLAN, NULL, br_vlan_rtm_dump, 0},

There is no RTM_GET* command that dumps CFM MEP config or status; those
attributes are only produced by br_cfm_config_fill_info() and
br_cfm_status_fill_info(), reachable solely from br_fill_ifinfo(), and
include/uapi/linux/rtnetlink.h has no CFM command.  The same applies to the
per-port IFLA_BRIDGE_MST nest, for which RTM_GETVLAN returns only the
indirect per-VLAN state.

Would a per-site message be better here, so the advice matches what the
kernel can actually return?

>  nla_put_failure:
>  	nlmsg_cancel(skb, nlh);
>  	return -EMSGSIZE;
> @@ -654,7 +662,8 @@ void br_info_notify(int event, const struct net_bridge *br,
>  	if (skb == NULL)
>  		goto errout;
>  
> -	err = br_fill_ifinfo(skb, port, 0, 0, event, 0, filter, dev, false);
> +	err = br_fill_ifinfo(skb, port, 0, 0, event, 0, filter, dev, false,
> +			     NULL);
>  	if (err < 0) {
>  		/* -EMSGSIZE implies BUG in br_nlmsg_size() */
>  		WARN_ON(err == -EMSGSIZE);

[Severity: Medium]
This is a pre-existing issue, but it affects the notification behaviour
described in the changelog: on the CFM status notification path the skb can
be too small, so this WARN_ON fires before the new -E2BIG check is reached.

br_cfm_status_fill_info() opens one nest per record:

net/bridge/br_cfm_netlink.c:br_cfm_status_fill_info() {
	...
	tb = nla_nest_start(skb, IFLA_BRIDGE_CFM_MEP_STATUS_INFO);
	if (!tb)
		goto nla_info_failure;
	...
}

and likewise one IFLA_BRIDGE_CFM_CC_PEER_STATUS_INFO nest per peer MEP, but
br_get_link_af_size_filtered() only charges the outer nest plus the child
attributes:

net/bridge/br_netlink.c:br_get_link_af_size_filtered() {
	...
	vinfo_sz += nla_total_size(0);	/* IFLA_BRIDGE_CFM */
	/* For each status struct the MEP instance (u32) is added */
	vinfo_sz += num_cfm_mep_infos *
		    (nla_total_size(sizeof(u32)) ...
	...
}

No nla_total_size(0) is added for the enclosing per-record nests, so the
estimate is 4 bytes short per record.  br_cfm_notify() reaches this path:

net/bridge/br_cfm.c:br_cfm_notify() {
	u32 filter = RTEXT_FILTER_CFM_STATUS;

	br_info_notify(event, port->br, NULL, filter);
}

The deficit grows with the record count while the slack from kmalloc bucket
rounding does not, so for some MEP/peer counts nla_nest_start() fails with
-EMSGSIZE, the "implies BUG in br_nlmsg_size()" WARN_ON triggers and the
notification is dropped instead of taking the new -E2BIG path.

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