linux-hardening - Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

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: <546c9819-20e7-4474-9281-5d1567263637@intel.com>
Date: Wed, 6 Aug 2025 17:29:55 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Nathan Chancellor <nathan@...nel.org>
CC: Kees Cook <kees@...nel.org>, Masahiro Yamada <masahiroy@...nel.org>,
	Nicolas Schier <nicolas.schier@...ux.dev>, <linux-kbuild@...r.kernel.org>,
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>, Bill Wendling
	<morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
	<linux-kernel@...r.kernel.org>, <llvm@...ts.linux.dev>,
	<linux-hardening@...r.kernel.org>
Subject: Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

From: Nathan Chancellor <nathan@...nel.org>
Date: Tue, 5 Aug 2025 14:48:23 -0700

> On Tue, Aug 05, 2025 at 04:50:28PM +0200, Alexander Lobakin wrote:
>> From: Nathan Chancellor <nathan@...nel.org>
>> Date: Sun, 3 Aug 2025 10:32:35 -0700
>>
>>> On Sat, Aug 02, 2025 at 11:43:32AM -0700, Kees Cook wrote:
>>>> With the few remaining fixes now landed, we can re-enable the option
>>>> -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang
>>>> have the required multi-dimensional nonstring attribute support.
>>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
>>> index 55a1a96cd834..05d4323c6a13 100644
>>> --- a/drivers/net/ethernet/ti/netcp_ethss.c
>>> +++ b/drivers/net/ethernet/ti/netcp_ethss.c
>>> @@ -771,7 +771,7 @@ static struct netcp_module xgbe_module;
>>>  
>>>  /* Statistic management */
>>>  struct netcp_ethtool_stat {
>>> -	char desc[ETH_GSTRING_LEN];
>>> +	char desc[ETH_GSTRING_LEN] __nonstring;
>>
>>
>> Hmmm, ETH_GSTRING_LEN is the maximum length of the driver's statistics
>> name to be reported to Ethtool and this *includes* \0 at the end.
>> If this compilation flag triggers a warning here, the driver devs need
>> to fix their code. There should always be \0 at the end, `desc` is a
>> "proper" C 0-terminated string.
> 
> Ack, I had misunderstood a previous fix that Kees did for a similar but
> different instance of the warning in another Ethernet driver and I
> did not look much further than the driver copying these values around
> with memcpy(). This does trigger a warning, from the original message:
> 
>   drivers/net/ethernet/ti/netcp_ethss.c:1049:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization]
>    1049 |         GBENU_STATS_HOST(ale_unknown_ucast_bytes),
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST'
>     956 |         "GBE_HOST:"#field, GBENU_STATS0_MODULE,                 \
>         |         ^~~~~~~~~~~~~~~~~
>   drivers/net/ethernet/ti/netcp_ethss.c:1051:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization]
>    1051 |         GBENU_STATS_HOST(ale_unknown_mcast_bytes),
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST'
>     956 |         "GBE_HOST:"#field, GBENU_STATS0_MODULE,                 \
>         |         ^~~~~~~~~~~~~~~~~
>   drivers/net/ethernet/ti/netcp_ethss.c:1053:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization]
>    1053 |         GBENU_STATS_HOST(ale_unknown_bcast_bytes),
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST'
>     956 |         "GBE_HOST:"#field, GBENU_STATS0_MODULE,                 \
>         |         ^~~~~~~~~~~~~~~~~
> 
> So it seems to me like this is a legitimate problem? Are these

Yes, it's not a false-positive. I don't know how it worked at the end of
the day, may either the kernel Ethtool core or the userspace Ethtool put
\0 at the end (or maybe it was just luck or nobody tested Ethtool stats
on this driver).

> descriptions expected to be stable once they are released or are we able

Ethtool private stats are not "ABI" at all. Moreover, if they result in
incorrect code, this needs to be fixed no matter if someone already
wrote scripts dependent on these names or not.

> to adjust them? We could maybe shave an 'o' from 'unknown' to easily
> resolve this without losing much in the way of quick visual processing.

I've no idea why it's popular to define Ethtool stats names in drivers
using a fixed array of ETH_GSTRING_LEN and then do memcpy().
I've been always using just `const char * const[]` + strscpy() (then
switched the latter to ethtool_puts()/ethtool_sprintf() -- we even have
special helpers for that). In case some name goes past ETH_GSTRING_LEN,
it would just be truncated, but always have \0 at the end.
Plus most of the names are shorter than 32, so defining such arrays of
32 just wastes space in .rodata.

> 
> Cheers,
> Nathan

Thanks,
Olek