Re: [PATCH v3 net-next 10/14] net: enetc: refactor SI VLAN promiscuous mode configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The loop iterating over total_vfs uses BIT(si_id) which can trigger a shift-out-of-bounds (UB) on 32-bit systems or silently fail on 64-bit systems if the PCI device exposes a large number of VFs.
--

commit 646431737a5f49e08ba2d9ff1794e7e7489db129
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: refactor SI VLAN promiscuous mode configuration

This commit removes the enetc_set_vlan_promisc() functions and replaces
them with a unified enetc_set_si_vlan_promisc() function. It also removes
the vlan_promisc_simap field from struct enetc_pf to simplify the logic.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> index 66bfda60da9cb..16da732dc5de8 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> @@ -185,9 +185,8 @@ enum enetc_bdr_type {TX, RX};
>  #define  PSIPMMR_SI_MAC_MP(n)	BIT((n) + 16)
>  
>  #define ENETC_PSIPVMR		0x001c
> -#define ENETC_VLAN_PROMISC_MAP_ALL	0x7
> -#define ENETC_PSIPVMR_SET_VP(simap)	((simap) & 0x7)
> -#define ENETC_PSIPVMR_SET_VUTA(simap)	(((simap) & 0x7) << 16)
> +#define  PSIPVMR_SI_VLAN_P(n)	BIT(n) /* n = SI index */

[Severity: High]
Can this result in a shift-out-of-bounds if n >= 32?

> +
>  #define ENETC_PSIPMAR0(n)	(0x0100 + (n) * 0x8) /* n = SI index */
>  #define ENETC_PSIPMAR1(n)	(0x0104 + (n) * 0x8)

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index db2a800a7aaf0..afc02ed62c770 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -42,24 +42,20 @@ static void enetc_pf_destroy_pcs(struct phylink_pcs *pcs)

[ ... ]

> -static void enetc_set_vlan_promisc(struct enetc_hw *hw, char si_map)
> +static void enetc_set_si_vlan_promisc(struct enetc_si *si, int si_id,
> +				      bool promisc)
>  {
> -	u32 val = enetc_port_rd(hw, ENETC_PSIPVMR);
> +	struct enetc_hw *hw = &si->hw;
> +	u32 val;
>  
> -	val &= ~ENETC_PSIPVMR_SET_VP(ENETC_VLAN_PROMISC_MAP_ALL);
> -	enetc_port_wr(hw, ENETC_PSIPVMR, ENETC_PSIPVMR_SET_VP(si_map) | val);
> -}
> +	val = enetc_port_rd(hw, ENETC_PSIPVMR);
>  
> -static void enetc_enable_si_vlan_promisc(struct enetc_pf *pf, int si_idx)
> -{
> -	pf->vlan_promisc_simap |= BIT(si_idx);
> -	enetc_set_vlan_promisc(&pf->si->hw, pf->vlan_promisc_simap);
> -}
> +	if (promisc)
> +		val |= PSIPVMR_SI_VLAN_P(si_id);
> +	else
> +		val &= ~PSIPVMR_SI_VLAN_P(si_id);

[Severity: High]
If si_id >= 32, will this silently fail or corrupt the VLAN promiscuous state
for SI 0?

Since val is a 32-bit integer, even on 64-bit architectures where BIT() won't
overflow, the result gets truncated when applied to val here. On 32-bit
architectures, the shift wraps (e.g. 1 << 32 becomes 1 << 0), which would
inadvertently modify the state for the PF instead of the target VF.

>  
> -static void enetc_disable_si_vlan_promisc(struct enetc_pf *pf, int si_idx)
> -{
> -	pf->vlan_promisc_simap &= ~BIT(si_idx);
> -	enetc_set_vlan_promisc(&pf->si->hw, pf->vlan_promisc_simap);
> +	enetc_port_wr(hw, ENETC_PSIPVMR, val);
>  }

[ ... ]

> @@ -441,10 +437,9 @@ static void enetc_configure_port(struct enetc_pf *pf)
>  
>  	/* split up RFS entries */
>  	enetc_port_assign_rfs_entries(pf->si);
> -
>  	/* enforce VLAN promisc mode for all SIs */
> -	pf->vlan_promisc_simap = ENETC_VLAN_PROMISC_MAP_ALL;
> -	enetc_set_vlan_promisc(hw, pf->vlan_promisc_simap);
> +	for (int i = 0; i < pf->total_vfs + 1; i++)
> +		enetc_set_si_vlan_promisc(pf->si, i, true);

[Severity: High]
Does the driver validate or bound pf->total_vfs anywhere before this loop?

If the hardware or hypervisor advertises a TotalVFs value >= 32, this loop
will pass an unbounded index to enetc_set_si_vlan_promisc() and trigger the
shift issues above. The previous implementation safely bounded this by using
a hardcoded 0x7 mask.

>  
>  	enetc_port_wr(hw, ENETC_PSIPMMR, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703101328.550714-1-wei.fang@oss.nxp.com?part=10