GCC 13.2 warns:
drivers/net/wireless/intersil/p54/fwio.c:128:34: warning: '%s' directive output may be truncated writing up to 39 bytes into a region of size 32 [-Wformat-truncation=]
drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 16777215]
drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 255]
drivers/net/wireless/intersil/p54/fwio.c:127:17: note: 'snprintf' output between 7 and 52 bytes into a destination of size 32
The issue here is that wiphy->fw_version is 32 bytes and in theory the string
we try to place there can be 39 bytes. wiphy->fw_version is used for providing
the firmware version to user space via ethtool, so not really important.
fw_version in theory can be 24 bytes but in practise it's shorter, so even if
print only 19 bytes via ethtool there should not be any practical difference.
I did consider removing fw_var from the string altogether or making the maximum
length for fw_version 19 bytes, but chose this approach as it was the least
intrusive.
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/intersil/p54/fwio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
index b52cce38115d..c4fe70e05b9b 100644
--- a/drivers/net/wireless/intersil/p54/fwio.c
+++ b/drivers/net/wireless/intersil/p54/fwio.c
@@ -125,7 +125,7 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
"FW rev %s - Softmac protocol %x.%x\n",
fw_version, priv->fw_var >> 8, priv->fw_var & 0xff);
snprintf(dev->wiphy->fw_version, sizeof(dev->wiphy->fw_version),
- "%s - %x.%x", fw_version,
+ "%.19s - %x.%x", fw_version,
priv->fw_var >> 8, priv->fw_var & 0xff);
}
base-commit: 4e87ca403e2008b9e182239e1abbf6876a55eb33
--
2.39.2
Kalle Valo <[email protected]> wrote:
> GCC 13.2 warns:
>
> drivers/net/wireless/intersil/p54/fwio.c:128:34: warning: '%s' directive output may be truncated writing up to 39 bytes into a region of size 32 [-Wformat-truncation=]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 16777215]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 255]
> drivers/net/wireless/intersil/p54/fwio.c:127:17: note: 'snprintf' output between 7 and 52 bytes into a destination of size 32
>
> The issue here is that wiphy->fw_version is 32 bytes and in theory the string
> we try to place there can be 39 bytes. wiphy->fw_version is used for providing
> the firmware version to user space via ethtool, so not really important.
> fw_version in theory can be 24 bytes but in practise it's shorter, so even if
> print only 19 bytes via ethtool there should not be any practical difference.
>
> I did consider removing fw_var from the string altogether or making the maximum
> length for fw_version 19 bytes, but chose this approach as it was the least
> intrusive.
>
> Signed-off-by: Kalle Valo <[email protected]>
I forgot to include:
Compile tested only.
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 12/19/23 17:25, Kalle Valo wrote:
> GCC 13.2 warns:
>
> drivers/net/wireless/intersil/p54/fwio.c:128:34: warning: '%s' directive output may be truncated writing up to 39 bytes into a region of size 32 [-Wformat-truncation=]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 16777215]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 255]
> drivers/net/wireless/intersil/p54/fwio.c:127:17: note: 'snprintf' output between 7 and 52 bytes into a destination of size 32
>
> The issue here is that wiphy->fw_version is 32 bytes and in theory the string
> we try to place there can be 39 bytes.
Puh, I've been looking into /lib/vsprintf.c. Looking at the code, it seems
that it goes like this:
snprintf() -> vsnprintf() -> case FORMAT_TYPE_STR: -> string() -> string_nocheck():
| [...]
| if (buf < end)
| *buf = c;
| [...]
which dutifully checks for overruns (i.e. before writing into the buffer=wiphy->fw_version).
So, thankfully no blind memcpy/strcpy is taking place here.
Though, I don't know if this could be used for speculation attacks.
> wiphy->fw_version is used for providing
> the firmware version to user space via ethtool, so not really important.
> fw_version in theory can be 24 bytes but in practise it's shorter, so even if
> print only 19 bytes via ethtool there should not be any practical difference.
>
> I did consider removing fw_var from the string altogether or making the maximum
> length for fw_version 19 bytes, but chose this approach as it was the least
> intrusive.
>
> Signed-off-by: Kalle Valo <[email protected]>
|ethtool -i wlx0014a535e989
|driver: p54usb
|version: 6.7.0-rc6-wt+
|firmware-version: 2.13.25.0 - 5.9
|expansion-rom-version:
|bus-info: 5-2:1.0
|supports-statistics: yes
|supports-test: no
|supports-eeprom-access: no
|supports-register-dump: no
|supports-priv-flags: no
(yes, this doesn't change the output of ethtool. The firmware version is indeed
much much shorter than 24 bytes for the firmwares I know of.)
To be honest, I would write something like: "This patch silences gcc" in the commit
message. Rather than trying to come up with a well-intended justification. But I get
why this happens. That said, I would like to see gcc envolve... And maybe then it
will add warnings that go in the other direction (i.e. it will complain that
this %.19s was unnecessary here) :D.
Acked-by: Christian Lamparter <[email protected]> (Tested with Dell 1450 USB)
Regards,
Christian
> ---
> drivers/net/wireless/intersil/p54/fwio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
> index b52cce38115d..c4fe70e05b9b 100644
> --- a/drivers/net/wireless/intersil/p54/fwio.c
> +++ b/drivers/net/wireless/intersil/p54/fwio.c
> @@ -125,7 +125,7 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
> "FW rev %s - Softmac protocol %x.%x\n",
> fw_version, priv->fw_var >> 8, priv->fw_var & 0xff);
> snprintf(dev->wiphy->fw_version, sizeof(dev->wiphy->fw_version),
> - "%s - %x.%x", fw_version,
> + "%.19s - %x.%x", fw_version,
> priv->fw_var >> 8, priv->fw_var & 0xff);
> }
>
>
> base-commit: 4e87ca403e2008b9e182239e1abbf6876a55eb33
Christian Lamparter <[email protected]> writes:
> On 12/19/23 17:25, Kalle Valo wrote:
>> GCC 13.2 warns:
>> drivers/net/wireless/intersil/p54/fwio.c:128:34: warning: '%s'
>> directive output may be truncated writing up to 39 bytes into a
>> region of size 32 [-Wformat-truncation=]
>> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 16777215]
>> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 255]
>> drivers/net/wireless/intersil/p54/fwio.c:127:17: note: 'snprintf'
>> output between 7 and 52 bytes into a destination of size 32
>> The issue here is that wiphy->fw_version is 32 bytes and in theory
>> the string
>> we try to place there can be 39 bytes.
> Puh, I've been looking into /lib/vsprintf.c. Looking at the code, it seems
> that it goes like this:
>
> snprintf() -> vsnprintf() -> case FORMAT_TYPE_STR: -> string() -> string_nocheck():
> | [...]
> | if (buf < end)
> | *buf = c;
> | [...]
>
> which dutifully checks for overruns (i.e. before writing into the buffer=wiphy->fw_version).
> So, thankfully no blind memcpy/strcpy is taking place here.
> Though, I don't know if this could be used for speculation attacks.
>
>> wiphy->fw_version is used for providing
>> the firmware version to user space via ethtool, so not really important.
>> fw_version in theory can be 24 bytes but in practise it's shorter, so even if
>> print only 19 bytes via ethtool there should not be any practical difference.
>> I did consider removing fw_var from the string altogether or making
>> the maximum
>> length for fw_version 19 bytes, but chose this approach as it was the least
>> intrusive.
>> Signed-off-by: Kalle Valo <[email protected]>
>
> |ethtool -i wlx0014a535e989
> |driver: p54usb
> |version: 6.7.0-rc6-wt+
> |firmware-version: 2.13.25.0 - 5.9
> |expansion-rom-version:
> |bus-info: 5-2:1.0
> |supports-statistics: yes
> |supports-test: no
> |supports-eeprom-access: no
> |supports-register-dump: no
> |supports-priv-flags: no
>
> (yes, this doesn't change the output of ethtool. The firmware version is indeed
> much much shorter than 24 bytes for the firmwares I know of.)
Thanks for checking all this.
> To be honest, I would write something like: "This patch silences gcc" in the commit
> message. Rather than trying to come up with a well-intended justification. But I get
> why this happens. That said, I would like to see gcc envolve... And maybe then it
> will add warnings that go in the other direction (i.e. it will complain that
> this %.19s was unnecessary here) :D.
>
> Acked-by: Christian Lamparter <[email protected]> (Tested with Dell 1450 USB)
Yeah, I get why you dislike this. But it's just that net tree more or
less requires that our code is W=1 warning free and it will be soon
become a mess if we have existing warnings. So having this fixed, or
silenced, makes my life easier.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo <[email protected]> wrote:
> GCC 13.2 warns:
>
> drivers/net/wireless/intersil/p54/fwio.c:128:34: warning: '%s' directive output may be truncated writing up to 39 bytes into a region of size 32 [-Wformat-truncation=]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 16777215]
> drivers/net/wireless/intersil/p54/fwio.c:128:33: note: directive argument in the range [0, 255]
> drivers/net/wireless/intersil/p54/fwio.c:127:17: note: 'snprintf' output between 7 and 52 bytes into a destination of size 32
>
> The issue here is that wiphy->fw_version is 32 bytes and in theory the string
> we try to place there can be 39 bytes. wiphy->fw_version is used for providing
> the firmware version to user space via ethtool, so not really important.
> fw_version in theory can be 24 bytes but in practise it's shorter, so even if
> print only 19 bytes via ethtool there should not be any practical difference.
>
> I did consider removing fw_var from the string altogether or making the maximum
> length for fw_version 19 bytes, but chose this approach as it was the least
> intrusive.
>
> Compile tested only.
>
> Signed-off-by: Kalle Valo <[email protected]>
> Acked-by: Christian Lamparter <[email protected]> # Tested with Dell 1450 USB
Patch applied to wireless.git, thanks.
989cd9fd1ffe wifi: p54: fix GCC format truncation warning with wiphy->fw_version
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches