Re: [PATCH net] net: bcmgenet: restore the hardware filters on open

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

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

- [Medium] The commit message of "net: bcmgenet: restore the hardware
  filters on open" describes only the loss of user-configured rxnfc…

--

net: bcmgenet: restore the hardware filters on open

The patch splits bcmgenet_hfb_init() into a list-initialisation part that
now runs once from bcmgenet_probe(), and a new bcmgenet_hfb_restore() that
clears the hardware filter block and re-programs the configured rxnfc
rules.  bcmgenet_open() and bcmgenet_resume() both call the restore helper.

[ ... ]

[Severity: Medium]
Does this hunk also fix a NULL pointer dereference that the changelog does
not mention?

Before this change, priv->rxnfc_list was only initialised from
bcmgenet_open(), and alloc_etherdev_mqs() hands back zeroed private data,
so between register_netdev() and the first ifup the list head is
{NULL, NULL}.

The ethtool rxnfc getters have no netif_running() gate; bcmgenet_begin()
only enables the clock:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_begin() {
	struct bcmgenet_priv *priv = netdev_priv(dev);

	/* Turn on the clock */
	return clk_prepare_enable(priv->clk);
}

so ETHTOOL_GRXCLSRLCNT reaches bcmgenet_get_rxnfc() ->
bcmgenet_get_num_flows():

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_get_num_flows() {
	struct list_head *pos;
	int res = 0;

	list_for_each(pos, &priv->rxnfc_list)
		res++;
	...
}

with pos = head->next == NULL, the list_is_head() test does not match the
head, and pos->next is then dereferenced.

ETHTOOL_GRXCLSRLCNT, ETHTOOL_GRXCLSRULE and ETHTOOL_GRXCLSRLALL are
exempted from the ns_capable(CAP_NET_ADMIN) check in dev_ethtool():

net/ethtool/ioctl.c:dev_ethtool() {
	case ETHTOOL_GRXCLSRLCNT:
	case ETHTOOL_GRXCLSRULE:
	case ETHTOOL_GRXCLSRLALL:
		break;
	default:
		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
			return -EPERM;
	...
}

so an unprivileged local user running "ethtool -n eth0" on a bcmgenet
interface that was never brought up could hit this.

The changelog only describes the lost rules:

	Every rule the user configured is silently lost

	Initialise the lists once at probe and restore the rules on open, as
	bcmgenet_resume() already does.

Could the changelog also mention this crash, since it affects how the
patch is triaged for stable?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913190052.939955-1-nb%40tipi-net.de