| Message ID | 20231219162516.898205-1-kvalo@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 989cd9fd1ffe1a964429325f9092ea8f0db3f953 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: p54: fix GCC format truncation warning with wiphy->fw_version | expand |
Kalle Valo <kvalo@kernel.org> 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 <kvalo@kernel.org> I forgot to include: Compile tested only.
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 <kvalo@kernel.org> |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 <chunkeey@gmail.com> (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 <chunkeey@gmail.com> 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 <kvalo@kernel.org> > > |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 <chunkeey@gmail.com> (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.
Kalle Valo <kvalo@kernel.org> 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 <kvalo@kernel.org> > Acked-by: Christian Lamparter <chunkeey@gmail.com> # Tested with Dell 1450 USB Patch applied to wireless.git, thanks. 989cd9fd1ffe wifi: p54: fix GCC format truncation warning with wiphy->fw_version
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); }
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 <kvalo@kernel.org> --- drivers/net/wireless/intersil/p54/fwio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 4e87ca403e2008b9e182239e1abbf6876a55eb33