[PATCH v2] net: dsa: felix: check felix_cpu_port_for_conduit() for failure

[PATCH v2] net: dsa: felix: check felix_cpu_port_for_conduit() for failure

[Vitaliy Shevtsov]

felix_cpu_port_for_conduit() can return a negative value in case of failure
and then it will be used as a port index causing buffer underflow. This can
happen if a bonding interface in 802.1Q mode has no ports. This is unlikely
to happen because the underlying driver handles IFF_TEAM, IFF_MASTER,
IFF_BONDING bits and ports populating correctly, it is still better to
check this for correctness if somehow it fails.

Check if cpu_port is non-negative before using it as an index.
Errors from change_conduit() are already handled and no additional changes
are required.

Found by Linux Verification Center (linuxtesting.org) with Svace.

Signed-off-by: Vitaliy Shevtsov <v.shevtsov@mt-integration.ru>
---
v2: return the real cpu value instead of -EINVAL as per Andrew Lunn observation.

 drivers/net/dsa/ocelot/felix.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 0a4e682a55ef..1495f8e21f90 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -523,6 +523,7 @@ static int felix_tag_npi_change_conduit(struct dsa_switch *ds, int port,
 {
 	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
 	struct ocelot *ocelot = ds->priv;
+	int cpu;
 
 	if (netif_is_lag_master(conduit)) {
 		NL_SET_ERR_MSG_MOD(extack,
@@ -546,7 +547,12 @@ static int felix_tag_npi_change_conduit(struct dsa_switch *ds, int port,
 	}
 
 	felix_npi_port_deinit(ocelot, ocelot->npi);
-	felix_npi_port_init(ocelot, felix_cpu_port_for_conduit(ds, conduit));
+	cpu = felix_cpu_port_for_conduit(ds, conduit);
+	if (cpu < 0) {
+		dev_err(ds->dev, "Cpu port for conduit not found\n");
+		return cpu;
+	}
+	felix_npi_port_init(ocelot, cpu);
 
 	return 0;
 }
@@ -658,6 +664,11 @@ static int felix_tag_8021q_change_conduit(struct dsa_switch *ds, int port,
 	int cpu = felix_cpu_port_for_conduit(ds, conduit);
 	struct ocelot *ocelot = ds->priv;
 
+	if (cpu < 0) {
+		dev_err(ds->dev, "Cpu port for conduit not found\n");
+		return cpu;
+	}
+
 	ocelot_port_unassign_dsa_8021q_cpu(ocelot, port);
 	ocelot_port_assign_dsa_8021q_cpu(ocelot, port, cpu);
 
-- 
2.48.1

Re: [PATCH v2] net: dsa: felix: check felix_cpu_port_for_conduit() for failure

[Vladimir Oltean]

Hello Vitaliy,

On Wed, Mar 26, 2025 at 11:34:57PM +0500, Vitaliy Shevtsov wrote:
> felix_cpu_port_for_conduit() can return a negative value in case of failure
> and then it will be used as a port index causing buffer underflow. This can
> happen if a bonding interface in 802.1Q mode has no ports. This is unlikely
> to happen because the underlying driver handles IFF_TEAM, IFF_MASTER,
> IFF_BONDING bits and ports populating correctly, it is still better to
> check this for correctness if somehow it fails.

If the bonding interface has no ports, it is not a DSA conduit.

See the logic in dsa_conduit_changeupper() which, starting from "dev"
which is known to be a DSA conduit, it looks at info->upper_dev which is
a LAG device, and calls dsa_conduit_lag_join() when it is linking with
it. Thus, the LAG device (info->upper_dev) has at least one port: dev.

Also see this comment and walk through the dsa_conduit_lag_leave() path:

		/* If the LAG DSA conduit has no ports left, migrate back all
		 * user ports to the first physical CPU port
		 */

Given the justification provided thus far, I don't see a reason to merge
this patch. The "somehow it fails" needs to be a bit more clear.

Re: [PATCH v2] net: dsa: felix: check felix_cpu_port_for_conduit() for failure

[Vitaliy Shevtsov]

On Wed, 26 Mar 2025 21:22:59 +0200, Vladimir Oltean wrote:

> Hello Vitaliy,
> 
> If the bonding interface has no ports, it is not a DSA conduit.
> 
> See the logic in dsa_conduit_changeupper() which, starting from "dev"
> which is known to be a DSA conduit, it looks at info->upper_dev which is
> a LAG device, and calls dsa_conduit_lag_join() when it is linking with
> it. Thus, the LAG device (info->upper_dev) has at least one port: dev.
> 
> Also see this comment and walk through the dsa_conduit_lag_leave() path:
> 
> 		/* If the LAG DSA conduit has no ports left, migrate back all
> 		 * user ports to the first physical CPU port
> 		 */
> 
> Given the justification provided thus far, I don't see a reason to merge
> this patch. The "somehow it fails" needs to be a bit more clear.

Hello, Vladimir.

Okay then. Pretty clear, thanks.

-- 
Vitaliy Shevtsov <v.shevtsov@mt-integration.ru>