Re: [PATCH net-next] net: bridge: bound the port VLAN lists in the IFLA_AF_SPEC nest

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] No Fixes: tag on a patch that fixes user-visible malformed
  netlink output.

Pre-existing issues:
- [Medium] The u16 nla_len wrap of the IFLA_AF_SPEC nest remains
  reachable for the bridge device: br_fill_ifinfo() still emits…

--

net: bridge: bound the port VLAN lists in the IFLA_AF_SPEC nest

The patch stops br_fill_ifinfo() from putting more than 65535 bytes into
the single IFLA_AF_SPEC nest by asking br_af_spec_has_room() for room
before each VLAN, VLAN tunnel or MST entry, and clamps the size estimate
in br_get_link_af_size_filtered() to U16_MAX. Documentation/networking/bridge.rst
gets a section describing the per-entry costs and pointing at RTM_GETVLAN.

Two questions below, one about the tag block and one about the remaining
paths into the same nest.

> net: bridge: bound the port VLAN lists in the IFLA_AF_SPEC nest
>
> br_fill_ifinfo() opens one IFLA_AF_SPEC nest, fills it with the VLAN
> information of a bridge port and closes it with nla_nest_end(), which
> stores the accumulated length into nla_len. That field is a u16, so for
> a nest larger than 65535 bytes the stored length wraps modulo 65536.
> The encoding has never been able to describe more than that; nothing
> regressed.

[Severity: Medium]
Should this carry a Fixes: tag anyway?

The rest of the changelog describes user visible breakage in shipped
kernels: a 115048 byte RTM_NEWLINK with an nla_len of 49088 broadcast to
every RTNLGRP_LINK listener in the netns, 2341 attributes after the nest
misread as IFLA_IFNAME, a 147776 byte RTM_GETLINK reply whose attribute
walk derails 16672 bytes in, and a CONFIG_DEBUG_NET splat from
nla_nest_end(). Without a Fixes: line stable selection has no anchor.

The sibling fix cited a few paragraphs later, bdd39576bf50a ("net: bridge:
prevent too big nested attributes in br_fill_linkxstats()"), does carry
one:

    Fixes: a60c090361ea ("bridge: netlink: export per-vlan stats")

For the two lists that can fill the nest on their own the candidates would
be efa5356b0d97 ("bridge: per vlan dst_metadata netlink support") for
br_fill_vlan_tunnel_info() and IFLA_BRIDGE_VLAN_TUNNEL_INFO, and the commit
that added br_mst_fill_info() and IFLA_BRIDGE_MST_ENTRY.

> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Artem Lytkin <iprintercanon@gmail.com>

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index b2cd4e39326d0..643ae06f7df8c 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c

[ ... ]

> @@ -117,11 +128,19 @@ static size_t br_get_link_af_size_filtered(const struct net_device *dev,
>  		vinfo_sz += br_get_vlan_tunnel_info_size(vg);
>  
>  	/* Each VLAN is returned in bridge_vlan_info along with flags */
> -	vinfo_sz += num_vlan_infos * nla_total_size(sizeof(struct bridge_vlan_info));
> +	vinfo_sz += num_vlan_infos * BR_VLAN_INFO_SIZE;
>  
>  	if (p && vg && (filter_mask & RTEXT_FILTER_MST))
>  		vinfo_sz += br_mst_info_size(vg);
>  
> +	/* These three lists share the IFLA_AF_SPEC nest, whose length is a
> +	 * u16, and br_fill_ifinfo() stops once it is full. Do not size the
> +	 * skb for entries that will not be emitted. The CFM size below is
> +	 * only added for the bridge device, which carries no tunnel or MST
> +	 * entries and so stays far below the clamp.
> +	 */
> +	vinfo_sz = min_t(size_t, vinfo_sz, U16_MAX);
> +
>  	if (!(filter_mask & RTEXT_FILTER_CFM_STATUS))
>  		return vinfo_sz;
>  

[Severity: Medium]
This is a pre-existing issue and not introduced by the patch, but can the
same nla_len wrap still be reached for the bridge device through the CFM
status list?

The changelog says so explicitly ("CFM peer status can grow past the limit
on its own; that is a separate change"), so this is more a question about
whether the new helper could cover it in the same series than a complaint
about the scope.

br_cfm_status_fill_info() walks br->mep_list and each mep->peer_mep_list
appending roughly 64 to 76 bytes per entry with no aggregate cap:

net/bridge/br_cfm_netlink.c:br_cfm_status_fill_info() {
	hlist_for_each_entry_rcu(mep, &br->mep_list, head) {
		tb = nla_nest_start(skb, IFLA_BRIDGE_CFM_MEP_STATUS_INFO);
	...
}

br_cfm_mep_create() and br_cfm_cc_peer_mep_add() only reject duplicate
instance and mepid values under RTNL, so on the order of 860 MEP and peer
MEP entries push skb_tail_pointer(skb) - af past 65535, and the
nla_nest_end(skb, af) at the 'done:' label in br_fill_ifinfo() stores a
length truncated modulo 65536, which is the same failure this patch closes
for the port lists. Neither the IFLA_BRIDGE_CFM nest nor
br_mrp_fill_info() consults br_af_spec_has_room().

Also, the clamp above runs before the CFM terms are added, so it does not
constrain that path at all. Was that ordering intentional? The per-peer
accounting that follows the clamp also omits the
IFLA_BRIDGE_CFM_CC_PEER_STATUS_INFO nest header for each peer, which can
undersize the skb and make br_fill_ifinfo() return -EMSGSIZE.

> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index d337b1cfb980d..55dfa44269c90 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h

[ ... ]

> @@ -2161,6 +2168,18 @@ static inline int br_cfm_peer_mep_count(struct net_bridge *br, u32 *count)
>  #endif
>  
>  /* br_netlink.c */
> +/* The IFLA_AF_SPEC nest that br_fill_ifinfo() builds is a single netlink
> + * attribute, so everything put inside it has to fit in the u16 nla_len.
> + * The entries have fixed sizes, so ask for room before adding one instead
> + * of closing the nest with a length that wrapped.
> + */
> +static inline bool br_af_spec_has_room(const struct sk_buff *skb,
> +				       const struct nlattr *af, size_t size)
> +{
> +	return skb_tail_pointer(skb) - (const unsigned char *)af + size <=
> +	       U16_MAX;
> +}
> +

[ ... ]

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