Re: [PATCH v2 2/2] net: atlantic: AQC113: Limit multicast filters to available slots

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch swaps two constant bounds for values derived from the
  firmware-reported priv->l2_filter_count, and nothing validates that…
- [High] net: atlantic: off-by-one L2 filter index in
  hw_atl2_hw_packet_filter_set()
- [Low] The commit message leaves out the concrete impact and part of the
  code change: - It only says the firmware count (29) is "unfortunately…

--

net: atlantic: AQC113: Limit multicast filters to available slots

This commit adds a hw_get_num_multicast_addresses() callback to struct
aq_hw_ops. It lets AQC113 report fewer usable L2 filter slots than
AQ_HW_MULTICAST_ADDRESS_MAX, based on the l2_filter_count that the firmware
reports. aq_nic_set_multicast_list() uses this value to decide when to fall
back, and hw_atl2_hw_multicast_list_set() now bounds the count by the same
firmware value.

> On Apple silicon systems with AQC113 the firmware returns
[ ... ]
> unfortunately less than AQ_HW_MULTICAST_ADDRESS_MAX (32).
[ ... ]
> Use this potentially smaller
> number in aq_nic_set_multicast_list() to decide when to switch to
> promiscuous mode.

[Severity: Low]
This isn't a bug, but could the commit message describe the impact and the
full change more precisely?

Before this fix, with a base index of 9 and more than 28 addresses,
hw_atl2_hw_multicast_list_set() writes L2 unicast filter indices 38..41
(0x5240..0x525C). That is past the end of the filter table, and it overlaps
HW_ATL_RPFL2MC_ENF_ADR(0..3) at 0x5250..0x525C. The message does not say
this.

The message also does not mention that the guard in
hw_atl2_hw_multicast_list_set() changes from
(HW_ATL2_MAC_MAX - HW_ATL2_MAC_MIN) to (priv->l2_filter_count - 1).

Also, for multicast the fallback in aq_nic_set_multicast_list() is
IFF_ALLMULTI, not promiscuous mode.

[ ... ]

> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> index 8d7a489aa5de4..d0dc72bd098f4 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> @@ -810,7 +810,7 @@ static int hw_atl2_hw_multicast_list_set(struct aq_hw_s *self,
>  	struct aq_nic_cfg_s *cfg = self->aq_nic_cfg;
>  	int err = 0;
>  
> -	if (count > (HW_ATL2_MAC_MAX - HW_ATL2_MAC_MIN)) {
> +	if (count > (priv->l2_filter_count - 1)) {
>  		err = -EBADRQC;
>  		goto err_exit;
>  	}

[Severity: High]
This guard assumes hw_atl2_hw_packet_filter_set() uses the same slot
layout as this function. It doesn't look like it does.

hw_atl2_hw_multicast_list_set() sets base_location to
priv->l2_filters_base_index + 1. It writes ar_mac[0..count - 1] into
slots base_location + 0 through base_location + count - 1.

aq_nic_set_multicast_list() then calls aq_nic_set_packet_filter(),
which ends up in hw_atl2_hw_packet_filter_set(). That function computes
the same base_location, but its loop starts at i = HW_ATL2_MAC_MIN (1).
It enables base_location + i when i <= cfg->mc_list_count, so the +1
offset is applied a second time.

Take the values from the commit message (base index 9, count 29) and
mc_list_count == n:

  - slot 10 holds ar_mac[0], but hw_atl2_hw_packet_filter_set() never
    touches it. It stays enabled when the list shrinks to zero or
    IFF_MULTICAST is cleared.
  - slot 10 + n gets enabled even though no address was written to it
    on this pass, so frames for whatever stale address sits there are
    accepted.
  - the last iteration, i = 28, writes the enable bit of filter 38.
    That is HW_ATL2_MAC_MAX, past the end of the L2 unicast filter
    table.

The double offset happens on every AQC113, whatever the slot counts.
num_l2_filters is also only clamped to HW_ATL2_MAC_MAX, not to
HW_ATL2_MAC_MAX - base_location. So any base index that is large
enough relative to l2_filter_count walks off the end of the table.
This path also runs from aq_nic_start(), so every open hits it, not
only rx mode changes.

This patch is meant to keep the address list within the slots the
firmware reports. Should hw_atl2_hw_packet_filter_set() start at
i = 0, stop before l2_filter_count - 1 (clamped so base_location + i
stays below HW_ATL2_MAC_MAX), and test i < cfg->mc_list_count, to
match hw_atl2_hw_multicast_list_set()? The loop appears to come from
patch 1/2 of this series, so the fix probably belongs there.

[ ... ]

> @@ -1635,6 +1635,13 @@ static int hw_atl2_hw_vlan_ctrl(struct aq_hw_s *self, bool enable)
>  	return aq_hw_err_from_flags(self);
>  }
>  
> +static int hw_atl2_get_num_multicast_addresses(struct aq_hw_s *self)
> +{
> +	struct hw_atl2_priv *priv = self->priv;
> +
> +	return min_t(int, priv->l2_filter_count - 1, AQ_HW_MULTICAST_ADDRESS_MAX);
> +}

[Severity: High]
What happens here if the firmware reports l2_filter_count == 0?

priv->l2_filter_count is unsigned int, so priv->l2_filter_count - 1 wraps
to UINT_MAX, and min_t(int, ...) then returns -1.

hw_atl2_utils_get_filter_caps() stores the firmware value without any
check. The neighbouring art_count and l2_filters_base_index fields do get
sanitized:

drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c:hw_atl2_utils_get_filter_caps() {
    ...
	priv->art_count = filter_caps.rslv_tbl_count * 8;
	if (priv->art_count == 0)
		priv->art_count = HW_ATL2_ART_TOTAL_ENTRIES;
	priv->l2_filters_base_index = filter_caps.l2_filters_base_index;
	if (priv->l2_filters_base_index >= HW_ATL2_MAC_MAX)
		priv->l2_filters_base_index = HW_ATL2_MAC_UC;
	priv->l2_filter_count = filter_caps.l2_filter_count;
    ...
}

In aq_nic_set_multicast_list(), the unicast check is signed, so it sets
IFF_PROMISC and leaves i at 0. But i is unsigned int, so in the multicast
check the -1 is converted to UINT_MAX:

	if (i + netdev_mc_count(ndev) > num_multicast_addresses) {
		packet_filter |= IFF_ALLMULTI;
	} else {
		netdev_for_each_mc_addr(ha, ndev) {
			ether_addr_copy(self->mc_list.ar[i++],
					ha->addr);

Doesn't this mean the IFF_ALLMULTI fallback is never taken? Once more than
32 multicast addresses are joined, would the loop then overflow
self->mc_list.ar[] into the following fields of struct aq_nic_s?

The later "i <= num_multicast_addresses" check is also unsigned, so
hw_atl2_hw_multicast_list_set() gets called with count > 32. Its new guard,
"count > (priv->l2_filter_count - 1)", also compares against UINT_MAX and
rejects nothing. It would then read ar_mac[] out of bounds and program
filters at base_location + i beyond HW_ATL2_MAC_MAX.

There is also a separate case: a large l2_filter_count with a nonzero base
index. The value is clamped only to AQ_HW_MULTICAST_ADDRESS_MAX, never to
HW_ATL2_MAC_MAX - (l2_filters_base_index + 1). The guard in
hw_atl2_hw_multicast_list_set() used the hardware constant before, and now
it trusts the firmware count.

With a base of 9 and a count of 34, wouldn't slots 10..41 get programmed?
Filters 38..41 seem to land at 0x5240..0x525C, which overlaps
HW_ATL_RPFL2MC_ENF_ADR(0..3).

Would it make sense to validate l2_filter_count in
hw_atl2_utils_get_filter_caps()? That would mean handling zero and clamping
base + count to HW_ATL2_MAC_MAX, like the other fields there.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-net-atlantic-aq113-apple-l2-filter-slots-v2-0-0b988d031c78%40jannau.net