[PATCH net v4 0/2] net: stmmac: misc fixes

[PATCH net v4 0/2] net: stmmac: misc fixes

[Konrad Leszczynski]

This series adds two fixes addressing KASAN panic on ethtool usage and
flow stop on TC block setup when interface down.

Patchset has been created as a result of discussion at [1].

v2 can be found at [2].
v3 can be found at [3].

[1] https://lore.kernel.org/netdev/20250826113247.3481273-1-konrad.leszczynski@intel.com/
[2] https://lore.kernel.org/netdev/20250828100237.4076570-1-konrad.leszczynski@intel.com/
[3] https://lore.kernel.org/netdev/20250916092507.216613-1-konrad.leszczynski@intel.com/

v3 -> v4:
- add missing Reviewed-by lines

v2 -> v3:
- replace strcpy with ethtool_puts per suggestion
- removed extension patch for printing enhanced descriptors

v1 -> v2:
- add missing Fixes lines
- add missing SoB lines
- removed all non-fix patches. These will be sent in separate series

Karol Jurczenia (1):
  net: stmmac: check if interface is running before TC block setup

Konrad Leszczynski (1):
  net: stmmac: replace memcpy with ethtool_puts in ethtool

 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH net v4 1/2] net: stmmac: replace memcpy with ethtool_puts in ethtool

[Konrad Leszczynski]

Fix kernel exception by replacing memcpy with ethtool_puts when used with
safety feature strings in ethtool logic.

[  +0.000023] BUG: KASAN: global-out-of-bounds in stmmac_get_strings+0x17d/0x520 [stmmac]
[  +0.000115] Read of size 32 at addr ffffffffc0cfab20 by task ethtool/2571

[  +0.000005] Call Trace:
[  +0.000004]  <TASK>
[  +0.000003]  dump_stack_lvl+0x6c/0x90
[  +0.000016]  print_report+0xce/0x610
[  +0.000011]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
[  +0.000108]  ? kasan_addr_to_slab+0xd/0xa0
[  +0.000008]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
[  +0.000101]  kasan_report+0xd4/0x110
[  +0.000010]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
[  +0.000102]  kasan_check_range+0x3a/0x1c0
[  +0.000010]  __asan_memcpy+0x24/0x70
[  +0.000008]  stmmac_get_strings+0x17d/0x520 [stmmac]

Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Konrad Leszczynski <konrad.leszczynski@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 77758a7299b4..d5a2b7e9b2a9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -752,7 +752,7 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data)
 				if (!stmmac_safety_feat_dump(priv,
 							&priv->sstats, i,
 							NULL, &desc)) {
-					memcpy(p, desc, ETH_GSTRING_LEN);
+					ethtool_puts(&p, desc);
 					p += ETH_GSTRING_LEN;
 				}
 			}
-- 
2.34.1

[PATCH net v4 2/2] net: stmmac: check if interface is running before TC block setup

[Konrad Leszczynski]

From: Karol Jurczenia <karol.jurczenia@intel.com>

If the interface is down before setting a TC block, the queues are already
disabled and setup cannot proceed.

Reviewed-by: Konrad Leszczynski <konrad.leszczynski@intel.com>
Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Karol Jurczenia <karol.jurczenia@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7b16d1207b80..133ffdc91153 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6247,6 +6247,9 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 	struct stmmac_priv *priv = cb_priv;
 	int ret = -EOPNOTSUPP;
 
+	if (!netif_running(priv->dev))
+		return -EINVAL;
+
 	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
 		return ret;
 
-- 
2.34.1

Re: [PATCH net v4 1/2] net: stmmac: replace memcpy with ethtool_puts in ethtool

[Simon Horman]

On Tue, Sep 16, 2025 at 02:09:31PM +0200, Konrad Leszczynski wrote:
> Fix kernel exception by replacing memcpy with ethtool_puts when used with
> safety feature strings in ethtool logic.
> 
> [  +0.000023] BUG: KASAN: global-out-of-bounds in stmmac_get_strings+0x17d/0x520 [stmmac]
> [  +0.000115] Read of size 32 at addr ffffffffc0cfab20 by task ethtool/2571
> 
> [  +0.000005] Call Trace:
> [  +0.000004]  <TASK>
> [  +0.000003]  dump_stack_lvl+0x6c/0x90
> [  +0.000016]  print_report+0xce/0x610
> [  +0.000011]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> [  +0.000108]  ? kasan_addr_to_slab+0xd/0xa0
> [  +0.000008]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> [  +0.000101]  kasan_report+0xd4/0x110
> [  +0.000010]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> [  +0.000102]  kasan_check_range+0x3a/0x1c0
> [  +0.000010]  __asan_memcpy+0x24/0x70
> [  +0.000008]  stmmac_get_strings+0x17d/0x520 [stmmac]
> 
> Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Signed-off-by: Konrad Leszczynski <konrad.leszczynski@intel.com>

As per my comment on v3 (sorry, I missed that you had already posted v4):

I think it would be good to explain why using memcpy() is a problem here.

I looked and it seems to me that one reason is that the strings may be
too long. But are there others?

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 77758a7299b4..d5a2b7e9b2a9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -752,7 +752,7 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>  				if (!stmmac_safety_feat_dump(priv,
>  							&priv->sstats, i,
>  							NULL, &desc)) {
> -					memcpy(p, desc, ETH_GSTRING_LEN);
> +					ethtool_puts(&p, desc);
>  					p += ETH_GSTRING_LEN;

ethtool_puts() increments p, so I think the line above should be removed.

>  				}
>  			}
> -- 
> 2.34.1
> 
> 

Re: [PATCH net v4 1/2] net: stmmac: replace memcpy with ethtool_puts in ethtool

[Andrew Lunn]

On Tue, Sep 16, 2025 at 05:45:30PM +0100, Simon Horman wrote:
> On Tue, Sep 16, 2025 at 02:09:31PM +0200, Konrad Leszczynski wrote:
> > Fix kernel exception by replacing memcpy with ethtool_puts when used with
> > safety feature strings in ethtool logic.
> > 
> > [  +0.000023] BUG: KASAN: global-out-of-bounds in stmmac_get_strings+0x17d/0x520 [stmmac]
> > [  +0.000115] Read of size 32 at addr ffffffffc0cfab20 by task ethtool/2571
> > 
> > [  +0.000005] Call Trace:
> > [  +0.000004]  <TASK>
> > [  +0.000003]  dump_stack_lvl+0x6c/0x90
> > [  +0.000016]  print_report+0xce/0x610
> > [  +0.000011]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> > [  +0.000108]  ? kasan_addr_to_slab+0xd/0xa0
> > [  +0.000008]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> > [  +0.000101]  kasan_report+0xd4/0x110
> > [  +0.000010]  ? stmmac_get_strings+0x17d/0x520 [stmmac]
> > [  +0.000102]  kasan_check_range+0x3a/0x1c0
> > [  +0.000010]  __asan_memcpy+0x24/0x70
> > [  +0.000008]  stmmac_get_strings+0x17d/0x520 [stmmac]
> > 
> > Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
> > Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
> > Signed-off-by: Konrad Leszczynski <konrad.leszczynski@intel.com>
> 
> As per my comment on v3 (sorry, I missed that you had already posted v4):
> 
> I think it would be good to explain why using memcpy() is a problem here.
> 
> I looked and it seems to me that one reason is that the strings may be
> too long.

I actually think the strings are too short. But i could be wrong.

In general, stmmac uses:

struct stmmac_stats {
        char stat_string[ETH_GSTRING_LEN] __nonstring;
        int sizeof_stat;
        int stat_offset;
};

so the string is guaranteed to be long enough. However, these 'safety'
features make use of

struct dwmac5_error_desc {
        bool valid;
        const char *desc;
        const char *detailed_desc;
};

where desc can be any length, ironically making
dwmac5_safety_feat_dump() unsafe. This is why i asked that this be
changed to be the same stmmac_stats, so [ETH_GSTRING_LEN]
__nonstring. But that seems to of fallen on deaf ears.

	Andrew

Re: [PATCH net v4 1/2] net: stmmac: replace memcpy with ethtool_puts in ethtool

[Jakub Kicinski]

On Tue, 16 Sep 2025 19:12:22 +0200 Andrew Lunn wrote:
> where desc can be any length, ironically making
> dwmac5_safety_feat_dump() unsafe. This is why i asked that this be
> changed to be the same stmmac_stats, so [ETH_GSTRING_LEN]
> __nonstring. But that seems to of fallen on deaf ears.

Yes, very strange.

Tony, Przemek, I suspect the folks here are from "different part of
Intel" but I think they need some guidance..

Re: [PATCH net v4 2/2] net: stmmac: check if interface is running before TC block setup

[Jakub Kicinski]

On Tue, 16 Sep 2025 14:09:32 +0200 Konrad Leszczynski wrote:
> If the interface is down before setting a TC block, the queues are already
> disabled and setup cannot proceed.

I asked for more details in the commit message earlier.

Also where did the Fixes tags go? The Fixes tags should only be removed
for the patches which end up going to net-next.

Re: [PATCH net v4 1/2] net: stmmac: replace memcpy with ethtool_puts in ethtool

[Tony Nguyen]

On 9/16/2025 4:25 PM, Jakub Kicinski wrote:
> On Tue, 16 Sep 2025 19:12:22 +0200 Andrew Lunn wrote:
>> where desc can be any length, ironically making
>> dwmac5_safety_feat_dump() unsafe. This is why i asked that this be
>> changed to be the same stmmac_stats, so [ETH_GSTRING_LEN]
>> __nonstring. But that seems to of fallen on deaf ears.
> 
> Yes, very strange.
> 
> Tony, Przemek, I suspect the folks here are from "different part of
> Intel" but I think they need some guidance..

I'll reach out and see if we can provide them some help.

Thanks,
Tony