Re: [PATCH net-next 1/3] rtnetlink: add AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] rtnl_dump_mcaddr()/rtnl_dump_mcaddr_dev()
  (net/core/rtnetlink.c) resume a truncated dump purely positionally —…
- [Medium] rtnl_fill_mcaddr() (net/core/rtnetlink.c) copies dev->addr_len
  bytes out of ha->addr[MAX_ADDR_LEN] into the IFA_MULTICAST attribute:…
- [Low] Claimed uAPI/YAML divergence: Documentation/netlink/specs/rt-
  addr.yaml documents op `getmulticast` as 'Get / dump IPv4/IPv6…
- [Low] Claimed spec/implementation mismatch: rt-addr.yaml lists
  `cacheinfo` in the getmulticast reply attribute set (mcaddr-attrs) and…

--

rtnetlink: add AF_PACKET multicast dumps

This adds a PF_PACKET RTM_GETMULTICAST dump handler that walks dev->mc for
every device in the netns and reports each netdev_hw_addr in the existing
ifaddrmsg format with IFA_MULTICAST and IFA_MC_USERS.  The dump runs under
RCU plus netif_addr_lock_bh() and is registered with
RTNL_FLAG_DUMP_UNLOCKED.

A few questions below.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 81c5a6104dea1..5e83232c15048 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4566,6 +4566,136 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len ? : ret;
>  }
>  
> +static int rtnl_fill_mcaddr(struct sk_buff *skb, const struct net_device *dev,
> +			    const struct netdev_hw_addr *ha, u32 portid,
> +			    u32 seq, unsigned int flags)
> +{

[ ... ]

> +	if (nla_put(skb, IFA_MULTICAST, dev->addr_len, ha->addr) ||
> +	    nla_put_u32(skb, IFA_MC_USERS, ha->refcount)) {

[Severity: Medium]

Can this nla_put() copy uninitialized slab bytes into IFA_MULTICAST when
dev->addr_len grew after the entry was created?

netdev_hw_addr entries are allocated with plain kmalloc() and only
addr_len bytes are initialized, and the length used at creation time is
not recorded anywhere:

net/core/dev_addr_lists.c:__hw_addr_create() {
	alloc_size = sizeof(*ha);
	if (alloc_size < L1_CACHE_BYTES)
		alloc_size = L1_CACHE_BYTES;
	ha = kmalloc(alloc_size, GFP_ATOMIC);
	if (!ha)
		return NULL;
	memcpy(ha->addr, addr, addr_len);
	...
}

__dev_mc_add() passes dev->addr_len as it is at add time:

net/core/dev_addr_lists.c:__dev_mc_add() {
	err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
			       NETDEV_HW_ADDR_T_MULTICAST, global, false,
			       0, false);
}

while rtnl_fill_mcaddr() emits the current dev->addr_len.  One sequence
that produces a mismatch:

  1. create a tap device (ARPHRD_ETHER, addr_len 6)
  2. SIOCADDMULTI via dev_ifsioc() -> dev_mc_add_global(), which only
     requires netif_device_present(), so the device may be down; 6 bytes
     of ha->addr are initialized
  3. TUNSETLINK with ARPHRD_TUNNEL6 in drivers/net/tun.c:

	tun->dev->type = (int) arg;
	tun->dev->addr_len = tun_get_addr_len(tun->dev->type);

     which raises addr_len to 16 and does not purge dev->mc
  4. run the new RTM_GETMULTICAST/AF_PACKET dump

Would step 4 then emit 16 bytes out of a 6-byte-initialized ha->addr,
handing 10 bytes of never-written slab memory to user space?  The read
stays inside ha->addr[MAX_ADDR_LEN], so this is a disclosure of
uninitialized memory rather than an out-of-bounds access.

The same bytes are already visible through /proc/net/dev_mcast, which
prints "%*phN" with dev->addr_len, so the underlying unzeroed tail is
older than this patch, but this adds a second path to it.  Would it make
sense to either bound the emitted length or zero the address tail in
__hw_addr_create()?

> +		nlmsg_cancel(skb, nlh);
> +		return -EMSGSIZE;
> +	}
> +
> +	nlmsg_end(skb, nlh);
> +	return 0;
> +}
> +
> +static int rtnl_dump_mcaddr_dev(struct net_device *dev, struct sk_buff *skb,
> +				struct netlink_callback *cb, int *s_addr_idx,
> +				unsigned int flags)
> +{
> +	struct netdev_hw_addr *ha;
> +	int addr_idx = 0;
> +	int err = 0;
> +
> +	netif_addr_lock_bh(dev);
> +	netdev_for_each_mc_addr(ha, dev) {
> +		if (addr_idx < *s_addr_idx) {
> +			addr_idx++;
> +			continue;
> +		}
> +		err = rtnl_fill_mcaddr(skb, dev, ha, NETLINK_CB(cb->skb).portid,
> +				       cb->nlh->nlmsg_seq, flags);
> +		if (err < 0)
> +			break;
> +		addr_idx++;
> +	}
> +	netif_addr_unlock_bh(dev);
> +
> +	*s_addr_idx = err < 0 ? addr_idx : 0;

[Severity: Medium]

Should this dump also maintain a generation counter and call
nl_dump_check_consistent() so interrupted dumps get NLM_F_DUMP_INTR?

Resume state here is purely positional (ctx->ifindex plus ctx->addr_idx)
and neither RTNL nor netif_addr_lock is held between two invocations of
the callback.  netlink_dump() turns the -EMSGSIZE into a continued dump:

net/netlink/af_netlink.c:netlink_dump() {
	if (nlk->dump_done_errno == -EMSGSIZE && skb->len)
		nlk->dump_done_errno = skb->len;
}

so entries added to or removed from dev->mc in that window shift the
index and entries can be silently repeated or skipped.

cb->seq is never assigned in rtnl_dump_mcaddr(), so the check in
nl_dump_check_consistent():

	if (cb->prev_seq && cb->seq != cb->prev_seq)
		nlh->nlmsg_flags |= NLM_F_DUMP_INTR;

can never fire and user space has no way to notice the loss.  The AF_INET
sibling of the same message type does both halves: inet_dump_addr() sets

	cb->seq = inet_base_seq(tgt_net);

and in_dev_dump_ifmcaddr() calls nl_dump_check_consistent(cb,
nlmsg_hdr(skb)) for every emitted message.

> +
> +	return err;
> +}

[ ... ]

> +static int rtnl_dump_mcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{

[ ... ]

> +	for_each_netdev_dump(net, dev, ctx->ifindex) {
> +		err = rtnl_dump_mcaddr_dev(dev, skb, cb, &ctx->addr_idx,
> +					   flags);
> +		if (err < 0)
> +			break;
> +	}

[Severity: Medium]

Related to the above, can a stale ctx->addr_idx be applied to the wrong
device here?

On -EMSGSIZE, rtnl_dump_mcaddr_dev() keeps the offset in ctx->addr_idx and
the break skips the ifindex++ of for_each_netdev_dump(), so ctx->ifindex
still names the interrupted device.  If that device is unregistered before
the dump resumes:

include/linux/netdevice.h:
	#define for_each_netdev_dump(net, d, ifindex)				\
		for (; (d = xa_find(&(net)->dev_by_index, &ifindex,		\
				    ULONG_MAX, XA_PRESENT)); ifindex++)

xa_find() with XA_PRESENT returns the next device that does exist, and the
non-zero ctx->addr_idx is passed unchanged into rtnl_dump_mcaddr_dev(), so

		if (addr_idx < *s_addr_idx) {
			addr_idx++;
			continue;
		}

would skip the first ctx->addr_idx multicast entries of an unrelated
device.  Should ctx->addr_idx be reset when the resumed ifindex does not
match the recorded one?

> +out:
> +	rcu_read_unlock();
> +	return err;
> +}
> +

[ ... ]

> @@ -7251,6 +7381,8 @@ static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst =
>  	{.msgtype = RTM_SETSTATS, .doit = rtnl_stats_set},
>  	{.msgtype = RTM_NEWLINKPROP, .doit = rtnl_newlinkprop},
>  	{.msgtype = RTM_DELLINKPROP, .doit = rtnl_dellinkprop},
> +	{.protocol = PF_PACKET, .msgtype = RTM_GETMULTICAST,
> +	 .dumpit = rtnl_dump_mcaddr, .flags = RTNL_FLAG_DUMP_UNLOCKED},

[Severity: Low]

This isn't a bug, but as of this commit alone the getmulticast op in
Documentation/netlink/specs/rt-addr.yaml still reads "Get / dump
IPv4/IPv6 multicast addresses." while the same op now also serves
AF_PACKET requests with link-layer address semantics.

The follow-up patch in the series, "netlink: specs: rt-addr: document
AF_PACKET multicast dumps", rewrites that doc to cover ifa-family
selecting AF_INET, AF_INET6 or AF_PACKET and the permanent-flag meaning,
so the spec and the implementation do agree at the end of the series.
Only noting it in case the two patches are ever applied separately.

>  	{.protocol = PF_BRIDGE, .msgtype = RTM_GETLINK,
>  	 .dumpit = rtnl_bridge_getlink},
>  	{.protocol = PF_BRIDGE, .msgtype = RTM_DELLINK,

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