linux-kernel - Re: [PATCH] net: dsa: felix: check felix_cpu_port_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dc85eb72-cdec-43a1-8ad7-6cd7db9c6b25@lunn.ch>
Date: Wed, 26 Mar 2025 19:22:07 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Vitaliy Shevtsov <v.shevtsov@...integration.ru>
Cc: Vladimir Oltean <vladimir.oltean@....com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org
Subject: Re: [PATCH] net: dsa: felix: check felix_cpu_port_for_conduit() for
 failure

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@...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