Re: [PATCH net-next v6 4/4] selftests: net: test AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

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

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

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] dev_mc_fill_addr() (net/core/dev_addr_lists.c:1210) emits
  IFA_MULTICAST with `nla_put(skb, IFA_MULTICAST, dev->addr_len,…
- [Medium] dump_mcaddr_l2_check()
  (tools/testing/selftests/net/rtnetlink.py:113) issues the new…

--

selftests: net: test AF_PACKET multicast dumps

Adds dump_mcaddr_l2_check() to tools/testing/selftests/net/rtnetlink.py,
which dumps the link-layer multicast addresses of dummy devices over
AF_PACKET RTM_GETMULTICAST and checks the ifa-index filter, the global
flag, mc-users and target-netnsid handling.

> diff --git a/tools/testing/selftests/net/rtnetlink.py b/tools/testing/selftests/net/rtnetlink.py
> index dc8c77db48974..5534ade056a5c 100755
> --- a/tools/testing/selftests/net/rtnetlink.py
> +++ b/tools/testing/selftests/net/rtnetlink.py

[ ... ]

> @@ -105,6 +110,69 @@ def dump_mcaddr6_check() -> None:
>                  s2.close()
>  
>  
> +def dump_mcaddr_l2_check() -> None:
> +    """
> +    Verify link-layer multicast addresses in an AF_PACKET RTM_GETMULTICAST
> +    dump: the ifa-index filter, mc-users, the global flag and
> +    target-netnsid.
> +    """
> +
> +    with NetNS() as ns, NetNSEnter(str(ns)):
> +        for ifname in ("dummy1", "dummy2"):
> +            ip(f"link add name {ifname} type dummy")
> +            ip(f"link set {ifname} up")
> +        dev_idx = socket.if_nametoindex("dummy1")
> +        ip(f"maddr add {ETH_TEST_MULTICAST_STR} dev dummy1")
> +
> +        rtnl = RtnlAddrFamily()
> +        defer(rtnl.close)
> +        addresses = rtnl.getmulticast(
> +            {"ifa-family": socket.AF_PACKET, "ifa-index": dev_idx},
> +            dump=True)

[Severity: Medium]
Should this dump be guarded so that an older kernel produces a skip
instead of a failure?

On a kernel without the PF_PACKET RTM_GETMULTICAST registration,
rtnetlink_rcv_msg() looks up the handler for the requested family, then
retries with PF_UNSPEC:

net/core/rtnetlink.c:rtnetlink_rcv_msg() {
	...
	link = rtnl_get_link(family, type);
	if (!link || !link->dumpit) {
		family = PF_UNSPEC;
		link = rtnl_get_link(family, type);
		if (!link || !link->dumpit)
			goto err_unlock;
	...
}

There is no PF_UNSPEC RTM_GETMULTICAST dumpit (only PF_INET in
net/ipv4/devinet.c and PF_INET6 in net/ipv6/addrconf.c, plus the
PF_PACKET entry added by this series), so err stays at its -EOPNOTSUPP
initialisation and an error ack comes back. ynl then turns that into an
exception:

tools/net/ynl/pyynl/lib/ynl.py:YnlFamily._op() {
	...
                if nl_msg.error:
                    raise NlError(nl_msg)
	...
}

which ksft_run() reports as "not ok" plus a traceback.

The two neighbouring tests in this same file take the other route:
_users_for() returns None when mc-users is absent, and
dump_mcaddr_check()/dump_mcaddr6_check() convert that into
raise KsftSkipEx("kernel does not expose IFA_MC_USERS"). Would wrapping
the first getmulticast() call and raising KsftSkipEx on NlError keep this
new case consistent with the rest of the file when the selftest tree is
newer than the kernel under test?

> +
> +        # dummy2 has entries as well, only dummy1 may be listed
> +        ksft_eq({addr['ifa-index'] for addr in addresses}, {dev_idx},
> +                "AF_PACKET multicast dump ignored ifa-index filter")
> +
> +        entries = {addr['multicast']: addr for addr in addresses}

[Severity: High]
The IFA_MULTICAST payload consumed here is emitted by dev_mc_fill_addr()
in net/core/dev_addr_lists.c, added by this series, using the device's
current addr_len for every entry of dev->mc:

net/core/dev_addr_lists.c:dev_mc_fill_addr() {
	...
	    nla_put(skb, IFA_MULTICAST, dev->addr_len, ha->addr) ||
	...
}

Each entry was created with an unzeroed allocation where only addr_len
bytes were written:

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

ha->addr is MAX_ADDR_LEN (32) bytes, so bytes addr_len..31 hold stale
slab contents. Can this dump then copy uninitialised heap bytes to user
space if dev->addr_len grows after the entry was inserted?

The sequence I end up with is:

  1. create a tap device (ARPHRD_ETHER, addr_len 6; tun_net_mclist is
     provided as .ndo_set_rx_mode, which SIOCADDMULTI requires per
     net/core/dev_ioctl.c)
  2. ip maddr add 01:00:5e:01:01:01 dev tap0, so __hw_addr_create()
     copies 6 bytes
  3. with the device down, TUNSETLINK to ARPHRD_TUNNEL6:

drivers/net/tun.c:__tun_chr_ioctl() {
	...
			tun->dev->type = (int) arg;
			tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
	...
}

     addr_len becomes sizeof(struct in6_addr), and dev->mc is not
     flushed; the NETDEV_PRE_TYPE_CHANGE handlers only drop the
     protocol-joined mappings via ip_mc_unmap()/ipv6_mc_unmap(), not
     global_use entries
  4. an AF_PACKET RTM_GETMULTICAST dump (RTNL_KIND_GET, no capability
     required) now returns 16 bytes for that entry, 10 of which were
     never initialised

The pre-existing dev->mc dump path does not reach this because it is
gated on the device type:

net/core/rtnetlink.c:ndo_dflt_fdb_dump() {
	...
	if (dev->type != ARPHRD_ETHER)
		return -EINVAL;
	...
}

The new dump has no such restriction. Would bounding the copy by what was
actually initialised - recording or clamping a per-entry length, or
zeroing ha->addr in __hw_addr_create() - be preferable to trusting
dev->addr_len here?

For the record, the bonding path is not a trigger: bond_enslave() calls
dev_uc_flush()/dev_mc_flush() immediately before bond_setup_by_slave()
changes addr_len, so only the tun/tap TUNSETLINK path above applies.

> +
> +        # Bringing an Ethernet device up joins 224.0.0.1, which maps
> +        # to 01:00:5e:00:00:01 in the device multicast list.
> +        all_hosts = entries.get(ETH_ALL_HOSTS_MULTICAST)
> +        ksft_not_none(all_hosts,
> +                      "dummy1 does not have the all-hosts link-layer address")
> +        if all_hosts is not None:
> +            ksft_not_in('global', all_hosts['flags'],
> +                        "protocol entry is global")
> +
> +        static = entries.get(ETH_TEST_MULTICAST)
> +        ksft_not_none(static, "dummy1 does not have the SIOCADDMULTI address")
> +        if static is not None:
> +            ksft_eq(static['mc-users'], 1,
> +                    "unexpected mc-users for the SIOCADDMULTI address")
> +            ksft_in('global', static['flags'],
> +                    "SIOCADDMULTI entry is not global")

[ ... ]

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