[PATCH net 0/3] sja1105 driver fixes

[PATCH net 0/3] sja1105 driver fixes

[Vladimir Oltean]

Hello,

This is a collection of 3 fixes for the sja1105 DSA driver:
- 1/3: "ethtool -S" shows a bogus counter with no name, and doesn't show
  a valid counter because of it (either "n_not_reach" or "n_rx_bcast").
- 2/3: RX timestamping filters other than L2 PTP event messages don't work,
  but are not rejected, either.
- 3/3: there is a KASAN out-of-bounds warning in sja1105_table_delete_entry()

Vladimir Oltean (3):
  net: dsa: sja1105: fix displaced ethtool statistics counters
  net: dsa: sja1105: reject other RX filters than
    HWTSTAMP_FILTER_PTP_V2_L2_EVENT
  net: dsa: sja1105: fix kasan out-of-bounds warning in
    sja1105_table_delete_entry()

 drivers/net/dsa/sja1105/sja1105_ethtool.c     |  9 ++++++++-
 drivers/net/dsa/sja1105/sja1105_ptp.c         | 20 ++++++++++++++-----
 .../net/dsa/sja1105/sja1105_static_config.c   |  6 ++++--
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH net 1/3] net: dsa: sja1105: fix displaced ethtool statistics counters

[Vladimir Oltean]

Port counters with no name (aka
sja1105_port_counters[__SJA1105_COUNTER_UNUSED]) are skipped when
reporting sja1105_get_sset_count(), but are not skipped during
sja1105_get_strings() and sja1105_get_ethtool_stats().

As a consequence, the first reported counter has an empty name and a
bogus value (reads from area 0, aka MAC, from offset 0, bits start:end
0:0). Also, the last counter (N_NOT_REACH on E/T, N_RX_BCAST on P/Q/R/S)
gets pushed out of the statistics counters that get shown.

Skip __SJA1105_COUNTER_UNUSED consistently, so that the bogus counter
with an empty name disappears, and in its place appears a valid counter.

Fixes: 039b167d68a3 ("net: dsa: sja1105: don't use burst SPI reads for port statistics")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_ethtool.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_ethtool.c b/drivers/net/dsa/sja1105/sja1105_ethtool.c
index 2ea64b1d026d..84d7d3f66bd0 100644
--- a/drivers/net/dsa/sja1105/sja1105_ethtool.c
+++ b/drivers/net/dsa/sja1105/sja1105_ethtool.c
@@ -571,6 +571,9 @@ void sja1105_get_ethtool_stats(struct dsa_switch *ds, int port, u64 *data)
 		max_ctr = __MAX_SJA1105PQRS_PORT_COUNTER;
 
 	for (i = 0; i < max_ctr; i++) {
+		if (!strlen(sja1105_port_counters[i].name))
+			continue;
+
 		rc = sja1105_port_counter_read(priv, port, i, &data[k++]);
 		if (rc) {
 			dev_err(ds->dev,
@@ -596,8 +599,12 @@ void sja1105_get_strings(struct dsa_switch *ds, int port,
 	else
 		max_ctr = __MAX_SJA1105PQRS_PORT_COUNTER;
 
-	for (i = 0; i < max_ctr; i++)
+	for (i = 0; i < max_ctr; i++) {
+		if (!strlen(sja1105_port_counters[i].name))
+			continue;
+
 		ethtool_puts(&data, sja1105_port_counters[i].name);
+	}
 }
 
 int sja1105_get_sset_count(struct dsa_switch *ds, int port, int sset)
-- 
2.34.1

Re: [PATCH net 1/3] net: dsa: sja1105: fix displaced ethtool statistics counters

[Simon Horman]

On Tue, Mar 18, 2025 at 01:57:14PM +0200, Vladimir Oltean wrote:
> Port counters with no name (aka
> sja1105_port_counters[__SJA1105_COUNTER_UNUSED]) are skipped when
> reporting sja1105_get_sset_count(), but are not skipped during
> sja1105_get_strings() and sja1105_get_ethtool_stats().
> 
> As a consequence, the first reported counter has an empty name and a
> bogus value (reads from area 0, aka MAC, from offset 0, bits start:end
> 0:0). Also, the last counter (N_NOT_REACH on E/T, N_RX_BCAST on P/Q/R/S)
> gets pushed out of the statistics counters that get shown.
> 
> Skip __SJA1105_COUNTER_UNUSED consistently, so that the bogus counter
> with an empty name disappears, and in its place appears a valid counter.
> 
> Fixes: 039b167d68a3 ("net: dsa: sja1105: don't use burst SPI reads for port statistics")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 2/3] net: dsa: sja1105: reject other RX filters than HWTSTAMP_FILTER_PTP_V2_L2_EVENT

[Vladimir Oltean]

This is all that we can support timestamping, so we shouldn't accept
anything else. Also see sja1105_hwtstamp_get().

To avoid erroring out in an inconsistent state, operate on copies of
priv->hwts_rx_en and priv->hwts_tx_en, and write them back when nothing
else can fail anymore.

Fixes: a602afd200f5 ("net: dsa: sja1105: Expose PTP timestamping ioctls to userspace")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_ptp.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index a1f4ca6ad888..08b45fdd1d24 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -61,17 +61,21 @@ enum sja1105_ptp_clk_mode {
 int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 {
 	struct sja1105_private *priv = ds->priv;
+	unsigned long hwts_tx_en, hwts_rx_en;
 	struct hwtstamp_config config;
 
 	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
 		return -EFAULT;
 
+	hwts_tx_en = priv->hwts_tx_en;
+	hwts_rx_en = priv->hwts_rx_en;
+
 	switch (config.tx_type) {
 	case HWTSTAMP_TX_OFF:
-		priv->hwts_tx_en &= ~BIT(port);
+		hwts_tx_en &= ~BIT(port);
 		break;
 	case HWTSTAMP_TX_ON:
-		priv->hwts_tx_en |= BIT(port);
+		hwts_tx_en |= BIT(port);
 		break;
 	default:
 		return -ERANGE;
@@ -79,15 +83,21 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 
 	switch (config.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		priv->hwts_rx_en &= ~BIT(port);
+		hwts_rx_en &= ~BIT(port);
 		break;
-	default:
-		priv->hwts_rx_en |= BIT(port);
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+		hwts_rx_en |= BIT(port);
 		break;
+	default:
+		return -ERANGE;
 	}
 
 	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
 		return -EFAULT;
+
+	priv->hwts_tx_en = hwts_tx_en;
+	priv->hwts_rx_en = hwts_rx_en;
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH net 2/3] net: dsa: sja1105: reject other RX filters than HWTSTAMP_FILTER_PTP_V2_L2_EVENT

[Simon Horman]

On Tue, Mar 18, 2025 at 01:57:15PM +0200, Vladimir Oltean wrote:
> This is all that we can support timestamping, so we shouldn't accept
> anything else. Also see sja1105_hwtstamp_get().
> 
> To avoid erroring out in an inconsistent state, operate on copies of
> priv->hwts_rx_en and priv->hwts_tx_en, and write them back when nothing
> else can fail anymore.
> 
> Fixes: a602afd200f5 ("net: dsa: sja1105: Expose PTP timestamping ioctls to userspace")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 3/3] net: dsa: sja1105: fix kasan out-of-bounds warning in sja1105_table_delete_entry()

[Vladimir Oltean]

There are actually 2 problems:
- deleting the last element doesn't require the memmove of elements
  [i + 1, end) over it. Actually, element i+1 is out of bounds.
- The memmove itself should move size - i - 1 elements, because the last
  element is out of bounds.

The out-of-bounds element still remains out of bounds after being
accessed, so the problem is only that we touch it, not that it becomes
in active use. But I suppose it can lead to issues if the out-of-bounds
element is part of an unmapped page.

Fixes: 6666cebc5e30 ("net: dsa: sja1105: Add support for VLAN operations")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_static_config.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.c b/drivers/net/dsa/sja1105/sja1105_static_config.c
index 3d790f8c6f4d..ffece8a400a6 100644
--- a/drivers/net/dsa/sja1105/sja1105_static_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_static_config.c
@@ -1917,8 +1917,10 @@ int sja1105_table_delete_entry(struct sja1105_table *table, int i)
 	if (i > table->entry_count)
 		return -ERANGE;
 
-	memmove(entries + i * entry_size, entries + (i + 1) * entry_size,
-		(table->entry_count - i) * entry_size);
+	if (i + 1 < table->entry_count) {
+		memmove(entries + i * entry_size, entries + (i + 1) * entry_size,
+			(table->entry_count - i - 1) * entry_size);
+	}
 
 	table->entry_count--;
 
-- 
2.34.1

Re: [PATCH net 3/3] net: dsa: sja1105: fix kasan out-of-bounds warning in sja1105_table_delete_entry()

[Simon Horman]

On Tue, Mar 18, 2025 at 01:57:16PM +0200, Vladimir Oltean wrote:
> There are actually 2 problems:
> - deleting the last element doesn't require the memmove of elements
>   [i + 1, end) over it. Actually, element i+1 is out of bounds.
> - The memmove itself should move size - i - 1 elements, because the last
>   element is out of bounds.
> 
> The out-of-bounds element still remains out of bounds after being
> accessed, so the problem is only that we touch it, not that it becomes
> in active use. But I suppose it can lead to issues if the out-of-bounds
> element is part of an unmapped page.
> 
> Fixes: 6666cebc5e30 ("net: dsa: sja1105: Add support for VLAN operations")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 0/3] sja1105 driver fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 18 Mar 2025 13:57:13 +0200 you wrote:
> Hello,
> 
> This is a collection of 3 fixes for the sja1105 DSA driver:
> - 1/3: "ethtool -S" shows a bogus counter with no name, and doesn't show
>   a valid counter because of it (either "n_not_reach" or "n_rx_bcast").
> - 2/3: RX timestamping filters other than L2 PTP event messages don't work,
>   but are not rejected, either.
> - 3/3: there is a KASAN out-of-bounds warning in sja1105_table_delete_entry()
> 
> [...]

Here is the summary with links:
  - [net,1/3] net: dsa: sja1105: fix displaced ethtool statistics counters
    https://git.kernel.org/netdev/net/c/00eb88752f48
  - [net,2/3] net: dsa: sja1105: reject other RX filters than HWTSTAMP_FILTER_PTP_V2_L2_EVENT
    https://git.kernel.org/netdev/net/c/b6a177b55971
  - [net,3/3] net: dsa: sja1105: fix kasan out-of-bounds warning in sja1105_table_delete_entry()
    https://git.kernel.org/netdev/net/c/5f2b28b79d2d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html