| Message ID | 20250326161251.7233-1-v.shevtsov@mt-integration.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: felix: check felix_cpu_port_for_conduit() for failure | expand |
On Wed, Mar 26, 2025 at 09:12:45PM +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. > > 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> > --- > 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 -EINVAL; > + } If i'm reading the code correctly you mean ocelot_bond_get_id() returns -ENOENT? If so, you should return the ENOENT, not replace it by EINVAL. Andrew
On Wed, 26 Mar 2025 19:22:07 +0100, Andrew Lunn wrote: > If i'm reading the code correctly you mean ocelot_bond_get_id() > returns -ENOENT? > > If so, you should return the ENOENT, not replace it by EINVAL. > > Andrew Or maybe it's better to just return negative cpu value instead? This variable will have the correct -ENOENT value in case of failure.
On Wed, Mar 26, 2025 at 11:29:29PM +0500, Vitaliy Shevtsov wrote: > On Wed, 26 Mar 2025 19:22:07 +0100, Andrew Lunn wrote: > > > If i'm reading the code correctly you mean ocelot_bond_get_id() > > returns -ENOENT? > > > > If so, you should return the ENOENT, not replace it by EINVAL. > > > > Andrew > > Or maybe it's better to just return negative cpu value instead? Yes, that is what i meant. The typical pattern is: int lag = ocelot_bond_get_id(ocelot, bond); if (lag < 0) return lag Andrew
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 -EINVAL; + } + 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 -EINVAL; + } + ocelot_port_unassign_dsa_8021q_cpu(ocelot, port); ocelot_port_assign_dsa_8021q_cpu(ocelot, port, cpu);
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> --- drivers/net/dsa/ocelot/felix.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)