Re: [PATCH] net: b44: use ethtool_puts

[Jacob Keller]

On 6/4/2026 1:50 AM, David Laight wrote:
> On Sat, 30 May 2026 17:03:34 -0700
> Rosen Penev <rosenp@xxxxxxxxx> wrote:
> 
>> There's a subtle error with the memcpy here, where b44_gstrings should
>> not be dereferenced. Dereferening causes the following error with W=1:
> 
> The definition is:
> static const char b44_gstrings[][ETH_GSTRING_LEN] = ...
> 
> There is nothing wrong with using memcpy().
> 
>>
>> In file included from drivers/net/ethernet/broadcom/b44.c:17:
>> In file included from ./include/linux/module.h:18:
>> In file included from ./include/linux/kmod.h:9:
>> In file included from ./include/linux/umh.h:4:
>> In file included from ./include/linux/gfp.h:7:
>> In file included from ./include/linux/mmzone.h:8:
>> In file included from ./include/linux/spinlock.h:56:
>> In file included from ./include/linux/preempt.h:79:
>> In file included from ./arch/powerpc/include/asm/preempt.h:5:
>> In file included from ./include/asm-generic/preempt.h:5:
>> In file included from ./include/linux/thread_info.h:23:
>> In file included from ./arch/powerpc/include/asm/current.h:13:
>> In file included from ./arch/powerpc/include/asm/paca.h:16:
>> In file included from ./include/linux/string.h:386:
>> ./include/linux/fortify-string.h:578:4: error: call to
>> '__read_overflow2_field' declared with 'warning' attribute: detected read
>> beyond size of field (2nd parameter); maybe use>
>>   578 |                        __read_overflow2_field(q_size_field, size);
>>       |                         ^
>>
>> Instead of fixing the memcpy, use ethtool_puts, which is the proper
>> helper for printing ethtool gstrings.
> 
> ethtool_puts() lets you pass short strings as well as ones that don't
> contain a '\0' terminator.
> It isn't needed here because all the strings are already padded.
> 

While not strictly needed, I think use of ethtool_puts in the ethtool
strings code makes sense and is easier to understand that it is correct.
memcpy is likely faster, but I don't think that speed is critical here.


>>
>> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
>> ---
>>  drivers/net/ethernet/broadcom/b44.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
>> index 90df02e0039c..f994636fbd5f 100644
>> --- a/drivers/net/ethernet/broadcom/b44.c
>> +++ b/drivers/net/ethernet/broadcom/b44.c
>> @@ -2031,11 +2031,11 @@ static int b44_set_pauseparam(struct net_device *dev,
>>  
>>  static void b44_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>>  {
>> -	switch(stringset) {
>> -	case ETH_SS_STATS:
>> -		memcpy(data, *b44_gstrings, sizeof(b44_gstrings));
> 
> Just remove the * or replace with an & - either is valid.
> The existing code is reading b44_gstrings[0] so overruns into the following
> strings.
> 

Right, that would have been the simplest fix.