Re: [PATCH] Prevent crash when adding interface under a lag

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hello,

On Sat, Dec 20, 2025 at 05:32:15PM +0000, Jerry Wu wrote:
> Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
> had fixed CVE-2024-26723 which is caused by NULL pointer dereference.
> ocelot_set_aggr_pgids in drivers/net/ethernet/mscc/ocelot.c contains a similar logic.
> This patch fix it in the same way as the aforementioned patch did.
> 
> Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
> ---

Please follow the Documentation/process/maintainer-netdev.rst process
and send the patch to the net tree and use ./scripts/get_maintainer.pl
to form a more comprehensive CC list, which at the very least contains
netdev@vger.kernel.org.

We need a Fixes: tag; I believe the correct one should be:
Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
Looking at that commit, we can clearly see that when iterating the first
time to form the "visited" mask, we do skip the unprobed ports properly:

		if (!ocelot_port || !ocelot_port->bond)
			continue;

but then there is another iteration over the ports right next to it, and
we failed to check this condition again.

I would add a small mention that the Ocelot library has two front-ends,
which treat unused ports differently.

The drivers/net/dsa/ocelot/felix_vsc9959.c frontend uses the DSA
framework, which registers ports for all switch ports, with the special
DSA_PORT_TYPE_UNUSED value. I tested the patch on this frontend, so the
bug doesn't exist there.

Then there is the drivers/net/ethernet/mscc/ocelot_vsc7514.c frontend,
which indeed leaves unused ports as NULL pointers. The problem only
exists there.

>  drivers/net/ethernet/mscc/ocelot.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 08bee56aea35..cb1c19c38c2c 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -2307,19 +2307,24 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
> 
>         /* Now, set PGIDs for each active LAG */
>         for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -               struct net_device *bond = ocelot->ports[lag]->bond;
> +               struct ocelot_port *port = ocelot->ports[lag];

Please name the variable ocelot_port, that is the convention in this driver.
If you name it "port", you are shadowing the "int port" definition from
the beginning of the function.

>                 int num_active_ports = 0;
> +               struct net_device *bond;
>                 unsigned long bond_mask;
>                 u8 aggr_idx[16];
> 
> -               if (!bond || (visited & BIT(lag)))
> +               if (!port || !port->bond || (visited & BIT(lag)))
>                         continue;
> 
> +               bond = port->bond;
>                 bond_mask = ocelot_get_bond_mask(ocelot, bond);

I think you can remove the "bond" local variable and replace this with
ocelot_port->bond directly. This makes the check rather similar with the
one from the previous

> 
>                 for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
>                         struct ocelot_port *ocelot_port = ocelot->ports[port];
> 
> +                       if (!port)
> +                               continue;
> +

This hunk is
(a) unnecessary. The bond_mask is formed by ocelot_get_bond_mask(), and
    this doesn't set ports which don't exist in the mask. So there is no
    reason to add this check.
(b) incorrect. You are testing that the "int port" is zero, not that the
    "ocelot_port" pointer is NULL (the real intention, but unnecessary,
    see a).

>                         // Destination mask
>                         ocelot_write_rix(ocelot, bond_mask,
>                                          ANA_PGID_PGID, port);
> --
> 2.51.0
>