[PATCH 00/22] gcc-7 -Wformat-* warnings

[Arnd Bergmann]

This series addresses all warnings that gcc-7 introduces for
-Wformat-overflow= and turns off the -Wformat-truncation by default
(they remain enabled with "make W=1").

The -Wformat-overflow patches take varying approaches:

- When the final use of the buffer is not limited and we print
into an intermediate variable on the stack, I generally make
the temporary buffer slightly larger to accomodate all
theoretically possible values. Usually the code is already
correct for all expected values, but gcc doesn't see that.

- In some cases, we use a fixed-length buffer as the %s input
for an sprintf to another buffer of the same length. Here
I could make the first buffer slightly smaller so that gcc
can prove the copies to be correct.

- In cases where the output buffer is required to have a fixed
length, I use snprintf() instead of sprintf(). This turns
the overflow warning into a truncation warning that is then
ignored. Here it would be much nicer to have a way to tell
the compiler what the maximum expected length is, but I
couldn't figure out a way to actually shut up the truncation
warning completely. Any ideas would be welcome.

Please review and apply as needed.

Arnd

Arnd Bergmann (22):
kbuild: disable -Wformat-truncation warnings by default
scsi: megaraid: fix format-overflow warning
scsi: mpt3sas: fix format overflow warning
scsi: fusion: fix string overflow warning
scsi: gdth: avoid buffer overflow warning
scsi: fnic: fix format string overflow warning
scsi: gdth: increase the procfs event buffer size
isdn: divert: fix sprintf buffer overflow warning
net: niu: fix format string overflow warning:
bnx2x: fix format overflow warning
net: thunder_bgx: avoid format string overflow warning
vmxnet3: avoid format strint overflow warning
liquidio: fix possible eeprom format string overflow
[media] usbvision-i2c: fix format overflow warning
hwmon: applesmc: fix format string overflow
x86: intel-mid: fix a format string overflow warning
platform/x86: alienware-wmi: fix format string overflow warning
gpio: acpi: fix string overflow for large pin numbers
block: DAC960: shut up format-overflow warning
sound: pci: avoid string overflow warnings
fscache: fix fscache_objlist_show format processing
IB/mlx4: fix sprintf format warning

.../intel-mid/device_libs/platform_max7315.c | 6 ++++--
drivers/block/DAC960.c | 12 +++++++----
drivers/gpio/gpiolib-acpi.c | 2 +-
drivers/hwmon/applesmc.c | 2 +-
drivers/infiniband/hw/mlx4/sysfs.c | 2 +-
drivers/isdn/divert/isdn_divert.c | 25 +++++++++++-----------
drivers/media/usb/usbvision/usbvision-i2c.c | 4 ++--
drivers/message/fusion/mptbase.c | 2 +-
.../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 3 ++-
drivers/net/ethernet/cavium/liquidio/lio_ethtool.c | 2 +-
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 2 +-
drivers/net/ethernet/sun/niu.c | 4 ++--
drivers/net/vmxnet3/vmxnet3_int.h | 2 +-
drivers/platform/x86/alienware-wmi.c | 2 +-
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/gdth.c | 2 +-
drivers/scsi/gdth_proc.c | 2 +-
drivers/scsi/megaraid.c | 6 ++++--
drivers/scsi/mpt3sas/mpt3sas_base.h | 2 +-
fs/fscache/object-list.c | 3 ++-
scripts/Makefile.extrawarn | 3 +++
sound/pci/mixart/mixart.h | 4 ++--
sound/pci/pcxhr/pcxhr.h | 4 ++--
sound/pci/rme9652/hdspm.c | 2 +-
24 files changed, 57 insertions(+), 43 deletions(-)

--
2.9.0

[Arnd Bergmann]

gcc-7 complains that the firmware version strings might overflow
for some values:

drivers/scsi/megaraid.c: In function 'megaraid_probe_one':
drivers/scsi/megaraid.c:314:33: error: '%d' directive writing between 1 and 2 bytes into a region of size between 1 and 2 [-Werror=format-overflow=]
drivers/scsi/megaraid.c:314:33: note: directive argument in the range [0, 15]
drivers/scsi/megaraid.c:314:3: note: 'sprintf' output between 7 and 9 bytes into a destination of size 7
drivers/scsi/megaraid.c:320:35: error: '%d' directive writing between 1 and 2 bytes into a region of size between 1 and 2 [-Werror=format-overflow=]
drivers/scsi/megaraid.c:320:35: note: directive argument in the range [0, 15]
drivers/scsi/megaraid.c:320:3: note: 'sprintf' output between 7 and 9 bytes into a destination of size 7

This makes the code use a truncating snprintf() instead, which shuts
up that warning.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---
drivers/scsi/megaraid.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 3c63c292cb92..7195cff51d4c 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -311,13 +311,15 @@ mega_query_adapter(adapter_t *adapter)
right 8 bits making them zero. This 0 value was hardcoded to fix
sparse warnings. */
if (adapter->product_info.subsysvid == PCI_VENDOR_ID_HP) {
- sprintf (adapter->fw_version, "%c%d%d.%d%d",
+ snprintf(adapter->fw_version, sizeof(adapter->fw_version),
+ "%c%d%d.%d%d",
adapter->product_info.fw_version[2],
0,
adapter->product_info.fw_version[1] & 0x0f,
0,
adapter->product_info.fw_version[0] & 0x0f);
- sprintf (adapter->bios_version, "%c%d%d.%d%d",
+ snprintf(adapter->bios_version, sizeof(adapter->fw_version),
+ "%c%d%d.%d%d",
adapter->product_info.bios_version[2],
0,
adapter->product_info.bios_version[1] & 0x0f,
--
2.9.0

[Arnd Bergmann]

With x86 allmodconfig, we currently get 233 -Wformat-truncation warnings,
which makes the entire warnings rather useless.

This turns off the warning by default, unless we specify W=1 or higher

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

scripts/Makefile.extrawarn | 3 +++
1 file changed, 3 insertions(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index fb3522fd8702..4b63c2f71adb 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -67,5 +67,8 @@ KBUILD_CFLAGS += $(call cc-disable-warning, format)
KBUILD_CFLAGS += $(call cc-disable-warning, sign-compare)
KBUILD_CFLAGS += $(call cc-disable-warning, format-zero-length)
KBUILD_CFLAGS += $(call cc-disable-warning, uninitialized)
+else
+# noisy gcc-7 warnings
+KBUILD_CFLAGS += $(call cc-option,-Wformat-truncation=0)
endif
endif
--
2.9.0

[Arnd Bergmann]

We print the driver name into one string and then add and ID
and copy it into a second string of the same length, at which
point gcc complains about a possible overflow:

drivers/scsi/mpt3sas/mpt3sas_scsih.c: In function '_scsih_probe':
drivers/scsi/mpt3sas/mpt3sas_scsih.c:8884:21: error: '_cm' directive writing 3 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
printf(ioc->name, "%s_cm%d", ioc->driver_name, ioc->id);
^~~~~~~~~
drivers/scsi/mpt3sas/mpt3sas_scsih.c:8884:21: note: directive argument in the range [0, 255]
drivers/scsi/mpt3sas/mpt3sas_scsih.c:8884:2: note: 'sprintf' output between 5 and 38 bytes into a destination of size 32
sprintf(ioc->name, "%s_cm%d", ioc->driver_name, ioc->id);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Making the first string shorter is sufficient to avoid the
warning here, as we know it can only contain either "mpt2sas"
or "mpt3sas".

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/scsi/mpt3sas/mpt3sas_base.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 099ab4ca7edf..a77bb7dc12b1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -970,7 +970,7 @@ struct MPT3SAS_ADAPTER {
u8 id;
int cpu_count;
char name[MPT_NAME_LENGTH];
- char driver_name[MPT_NAME_LENGTH];
+ char driver_name[MPT_NAME_LENGTH - 8];
char tmp_string[MPT_STRING_LENGTH];
struct pci_dev *pdev;
Mpi2SystemInterfaceRegs_t __iomem *chip;
--
2.9.0

[Arnd Bergmann]

gcc warns that the temporary buffer might be too small here:

drivers/net/ethernet/cavium/thunder/thunder_bgx.c: In function 'bgx_probe':
drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: error: '%d' directive writing between 1 and 10 bytes into a region of size between 9 and 11 [-Werror=format-overflow=]
sprintf(str, "BGX%d LMAC%d mode", bgx->bgx_id, lmacid);
^~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: note: directive argument in the range [0, 2147483647]
drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:3: note: 'sprintf' output between 16 and 27 bytes into a destination of size 20

This probably can't happen, but it can't hurt to make it long
enough for the theoretical limit.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index a0ca68ce3fbb..79112563a25a 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -1008,7 +1008,7 @@ static void bgx_print_qlm_mode(struct bgx *bgx, u8 lmacid)
{
struct device *dev = &bgx->pdev->dev;
struct lmac *lmac;
- char str[20];
+ char str[27];

if (!bgx->is_dlm && lmacid)
return;
--
2.9.0

[Arnd Bergmann]

gcc-7 notices that we copy a fixed length string into another
string of the same size, with additional characters:

drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
^~~~~~~~~~
drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48

We know this is fine as the template name is always "usbvision", so
we can easily avoid the warning by using this as the format string
directly.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/media/usb/usbvision/usbvision-i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbvision/usbvision-i2c.c b/drivers/media/usb/usbvision/usbvision-i2c.c
index fdf6b6e285da..aae9f69884da 100644
--- a/drivers/media/usb/usbvision/usbvision-i2c.c
+++ b/drivers/media/usb/usbvision/usbvision-i2c.c
@@ -187,8 +187,8 @@ int usbvision_i2c_register(struct usb_usbvision *usbvision)

usbvision->i2c_adap = i2c_adap_template;

- sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
- usbvision->dev->bus->busnum, usbvision->dev->devpath);
+ sprintf(usbvision->i2c_adap.name, "usbvision-%d-%s",
+ usbvision->dev->bus->busnum, usbvision->dev->devpath);
PDEBUG(DBG_I2C, "Adaptername: %s", usbvision->i2c_adap.name);
usbvision->i2c_adap.dev.parent = &usbvision->dev->dev;

--
2.9.0

[Arnd Bergmann]

We print a 256 byte event string into a buffer that is only 161
bytes long, this is clearly wrong:

drivers/scsi/gdth_proc.c: In function 'gdth_show_info':
drivers/scsi/gdth.c:3660:41: error: '%s' directive writing up to 255 bytes into a region of size between 141 and 150 [-Werror=format-overflow=]
sprintf(buffer,"Adapter %d: %s\n",
^~
/git/arm-soc/drivers/scsi/gdth.c:3660:13: note: 'sprintf' output between 13 and 277 bytes into a destination of size 161
sprintf(buffer,"Adapter %d: %s\n",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dvr->eu.async.ionode,dvr->event_string);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

gcc calculates that the worst case buffer size would be 277 bytes,
so we can use that.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/scsi/gdth_proc.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
index be609db66807..d08b2716752c 100644
--- a/drivers/scsi/gdth_proc.c
+++ b/drivers/scsi/gdth_proc.c
@@ -147,7 +147,7 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host *host)

gdth_cmd_str *gdtcmd;
gdth_evt_str *estr;
- char hrec[161];
+ char hrec[277];

char *buf;
gdth_dskstat_str *pds;
--
2.9.0

[Arnd Bergmann]

gcc-7 notices that the pin_table is an array of 16-bit numbers,
but we assume it can be printed as a two-character hexadecimal
string:

drivers/gpio/gpiolib-acpi.c: In function 'acpi_gpiochip_request_interrupt':
drivers/gpio/gpiolib-acpi.c:206:24: warning: '%02X' directive writing between 2 and 4 bytes into a region of size 3 [-Wformat-overflow=]
sprintf(ev_name, "_%c%02X",
^~~~
drivers/gpio/gpiolib-acpi.c:206:20: note: directive argument in the range [0, 65535]
sprintf(ev_name, "_%c%02X",
^~~~~~~~~
drivers/gpio/gpiolib-acpi.c:206:3: note: 'sprintf' output between 5 and 7 bytes into a destination of size 5
sprintf(ev_name, "_%c%02X",
^~~~~~~~~~~~~~~~~~~~~~~~~~~
agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pin);
~~~~

This can't be right, so this changes it to truncate the number to
an 8-bit pin number.

Fixes: 0d1c28a449c6 ("gpiolib-acpi: Add ACPI5 event model support to gpio.")

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/gpio/gpiolib-acpi.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c9b42dd12dfa..c3faea724af8 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -205,7 +205,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
char ev_name[5];
sprintf(ev_name, "_%c%02X",
agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
- pin);
+ (u8)pin);
if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
handler = acpi_gpio_irq_handler;
}
--
2.9.0

[Arnd Bergmann]

We have space for exactly one character for the index in "max7315_%d_base",
but as gcc points out having more would cause an string overflow:

arch/x86/platform/intel-mid/device_libs/platform_max7315.c: In function 'max7315_platform_data':
arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:26: error: '%d' directive writing between 1 and 11 bytes into a region of size 9 [-Werror=format-overflow=]
sprintf(base_pin_name, "max7315_%d_base", nr);
^~~~~~~~~~~~~~~~~
arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:26: note: directive argument in the range [-2147483647, 2147483647]
arm-soc/arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:3: note: 'sprintf' output between 15 and 25 bytes into a destination of size 17
sprintf(base_pin_name, "max7315_%d_base", nr);

This makes it use an snprintf() to truncate the string if that happened
rather than overflowing the stack.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

arch/x86/platform/intel-mid/device_libs/platform_max7315.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
index 6e075afa7877..58337b2bc682 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
@@ -38,8 +38,10 @@ static void __init *max7315_platform_data(void *info)
*/
strcpy(i2c_info->type, "max7315");
if (nr++) {
- sprintf(base_pin_name, "max7315_%d_base", nr);
- sprintf(intr_pin_name, "max7315_%d_int", nr);
+ snprintf(base_pin_name, sizeof(base_pin_name),
+ "max7315_%d_base", nr);
+ snprintf(intr_pin_name, sizeof(intr_pin_name),
+ "max7315_%d_int", nr);
} else {
strcpy(base_pin_name, "max7315_base");
strcpy(intr_pin_name, "max7315_int");
--
2.9.0

[Arnd Bergmann]

gcc points out a theorerical string overflow:

drivers/message/fusion/mptbase.c: In function 'mpt_detach':
drivers/message/fusion/mptbase.c:2103:17: error: '%s' directive writing up to 31 bytes into a region of size 28 [-Werror=format-overflow=]
sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s/summary", ioc->name);
^~~~~
drivers/message/fusion/mptbase.c:2103:2: note: 'sprintf' output between 13 and 44 bytes into a destination of size 32

We can simply double the size of the local buffer here to be on the
safe side.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/message/fusion/mptbase.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 62cff5afc6bd..46b67a67edc8 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -2079,7 +2079,7 @@ void
mpt_detach(struct pci_dev *pdev)
{
MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
- char pname[32];
+ char pname[64];
u8 cb_idx;
unsigned long flags;
struct workqueue_struct *wq;
--
2.9.0

[Arnd Bergmann]

gcc notices that we would overflow the buffer for the
inquiry of the product name if we have too many adapters:

drivers/scsi/gdth.c: In function 'gdth_next':
drivers/scsi/gdth.c:2357:29: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
sprintf(inq.product,"Host Drive #%02d",t);
^~~~~~~~~~~~~~~~~~~
drivers/scsi/gdth.c:2357:9: note: 'sprintf' output between 16 and 17 bytes into a destination of size 16
sprintf(inq.product,"Host Drive #%02d",t);

This won't happen in practice, so just use snprintf to
truncate the string.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/scsi/gdth.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index facc7271f932..a4473356a9dc 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -2354,7 +2354,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
inq.resp_aenc = 2;
inq.add_length= 32;
strcpy(inq.vendor,ha->oem_name);
- sprintf(inq.product,"Host Drive #%02d",t);
+ snprintf(inq.product, sizeof(inq.product), "Host Drive #%02d",t);
strcpy(inq.revision," ");
gdth_copy_internal_data(ha, scp, (char*)&inq, sizeof(gdth_inq_data));
break;
--
2.9.0

[Arnd Bergmann]

gcc-7 notices that "-event-%d" could be more than 11 characters long
if we had larger 'vector' numbers:

drivers/net/vmxnet3/vmxnet3_drv.c: In function 'vmxnet3_activate_dev':
drivers/net/vmxnet3/vmxnet3_drv.c:2095:40: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
sprintf(intr->event_msi_vector_name, "%s-event-%d",
^~~~~~~~~~~~~
drivers/net/vmxnet3/vmxnet3_drv.c:2095:3: note: 'sprintf' output between 9 and 33 bytes into a destination of size 32

The current code is safe, but making the string a little longer
is harmless and lets gcc see that it's ok.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/net/vmxnet3/vmxnet3_int.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_int.h b/drivers/net/vmxnet3/vmxnet3_int.h
index ba1c9f93592b..9c51b8be0038 100644
--- a/drivers/net/vmxnet3/vmxnet3_int.h
+++ b/drivers/net/vmxnet3/vmxnet3_int.h
@@ -311,7 +311,7 @@ struct vmxnet3_intr {
u8 num_intrs; /* # of intr vectors */
u8 event_intr_idx; /* idx of the intr vector for event */
u8 mod_levels[VMXNET3_LINUX_MAX_MSIX_VECT]; /* moderation level */
- char event_msi_vector_name[IFNAMSIZ+11];
+ char event_msi_vector_name[IFNAMSIZ+17];
#ifdef CONFIG_PCI_MSI
struct msix_entry msix_entries[VMXNET3_LINUX_MAX_MSIX_VECT];
#endif
--
2.9.0

[Arnd Bergmann]

We get a warning for the port_name string that might be longer than
six characters if we had more than 10 ports:

drivers/net/ethernet/sun/niu.c: In function 'niu_put_parent':
drivers/net/ethernet/sun/niu.c:9563:21: error: '%d' directive writing between 1 and 3 bytes into a region of size 2 [-Werror=format-overflow=]
sprintf(port_name, "port%d", port);
^~~~~~~~
drivers/net/ethernet/sun/niu.c:9563:21: note: directive argument in the range [0, 255]
drivers/net/ethernet/sun/niu.c:9563:2: note: 'sprintf' output between 6 and 8 bytes into a destination of size 6
sprintf(port_name, "port%d", port);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/sun/niu.c: In function 'niu_pci_init_one':
drivers/net/ethernet/sun/niu.c:9538:22: error: '%d' directive writing between 1 and 3 bytes into a region of size 2 [-Werror=format-overflow=]
sprintf(port_name, "port%d", port);
^~~~~~~~
drivers/net/ethernet/sun/niu.c:9538:22: note: directive argument in the range [0, 255]
drivers/net/ethernet/sun/niu.c:9538:3: note: 'sprintf' output between 6 and 8 bytes into a destination of size 6

While we know that the port number is small, there is no harm in
making the format string two bytes longer to avoid the warning.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/net/ethernet/sun/niu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 46cb7f8955a2..4bb04aaf9650 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -9532,7 +9532,7 @@ static struct niu_parent *niu_get_parent(struct niu *np,
p = niu_new_parent(np, id, ptype);

if (p) {
- char port_name[6];
+ char port_name[8];
int err;

sprintf(port_name, "port%d", port);
@@ -9553,7 +9553,7 @@ static void niu_put_parent(struct niu *np)
{
struct niu_parent *p = np->parent;
u8 port = np->port;
- char port_name[6];
+ char port_name[8];

BUG_ON(!p || p->ports[port] != np);

--
2.9.0

[Arnd Bergmann]

The MSI interrupt name can require 11 bytes in addition to the device name,
for a total of 23 bytes:

drivers/scsi/fnic/fnic_isr.c: In function 'fnic_request_intr':
drivers/scsi/fnic/fnic_isr.c:192:4: error: '-fcs-rq' directive writing 7 bytes into a region of size between 5 and 16 [-Werror=format-overflow=]
"%.11s-fcs-rq", fnic->name);
drivers/scsi/fnic/fnic_isr.c:206:3: note: 'sprintf' output between 12 and 23 bytes into a destination of size 16
sprintf(fnic->msix[FNIC_MSIX_ERR_NOTIFY].devname,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
"%.11s-err-notify", fnic->name);

This extends the buffer to fit any possible value.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/scsi/fnic/fnic.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 67aab965c0f4..d094ba59ed15 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -180,7 +180,7 @@ enum fnic_msix_intr_index {

struct fnic_msix_entry {
int requested;
- char devname[IFNAMSIZ];
+ char devname[IFNAMSIZ + 11];
irqreturn_t (*isr)(int, void *);
void *devid;
};
--
2.9.0

[Arnd Bergmann]

gcc points out a possible format string overflow for a large value of 'zone':

drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
sprintf(buffer, "zone%02X", i);
^~~~
drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
sprintf(buffer, "zone%02X", i);
^~~~~~~~~~
drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10

While the zone should never be that large, it's easy to make the
buffer a few bytes longer so gcc can prove this to be safe.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/platform/x86/alienware-wmi.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index 0831b428c217..acc01242da82 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -421,7 +421,7 @@ static DEVICE_ATTR(lighting_control_state, 0644, show_control_state,
static int alienware_zone_init(struct platform_device *dev)
{
int i;
- char buffer[10];
+ char buffer[13];
char *name;

if (interface == WMAX) {
--
2.9.0

[Arnd Bergmann]

With gcc-7, we get various warnings about a possible string overflow:

sound/pci/rme9652/hdspm.c: In function 'snd_hdspm_create_alsa_devices':
sound/pci/rme9652/hdspm.c:2123:17: error: ' MIDIoverMADI' directive writing 13 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
sound/pci/pcxhr/pcxhr.c: In function 'pcxhr_probe':
sound/pci/pcxhr/pcxhr.c:1647:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
sound/pci/mixart/mixart.c: In function 'snd_mixart_probe':
sound/pci/mixart/mixart.c:1353:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
^~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1353:28: note: using the range [-2147483648, 2147483647] for directive argument
sound/pci/mixart/mixart.c:1353:3: note: 'sprintf' output between 10 and 51 bytes into a destination of size 32
sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1354:27: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 80 [-Werror=format-overflow=]
sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
^~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1354:27: note: using the range [-2147483648, 2147483647] for directive argument
sound/pci/mixart/mixart.c:1354:3: note: 'sprintf' output between 10 and 99 bytes into a destination of size 80

I have checked these all and found that the driver-private
shortname strings for mixart and pcxhr are longer than necessary,
and making them shorter will be safe while also making it clear
that no overflow can happen when they get passed as a substring
into the card shortname.

For hdspm, we have a local buffer of the same size as its substring.
In this case, making the buffer a little longer is safe as the
functions that take it as an argument all use length checking and
the strings we pass into it are actually short enough.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

sound/pci/mixart/mixart.h | 4 ++--
sound/pci/pcxhr/pcxhr.h | 4 ++--
sound/pci/rme9652/hdspm.c | 2 +-

3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/pci/mixart/mixart.h b/sound/pci/mixart/mixart.h
index 426743871540..c8309e327663 100644
--- a/sound/pci/mixart/mixart.h
+++ b/sound/pci/mixart/mixart.h
@@ -75,8 +75,8 @@ struct mixart_mgr {
struct mem_area mem[2];

/* share the name */
- char shortname[32]; /* short name of this soundcard */
- char longname[80]; /* name of this soundcard */
+ char shortname[16]; /* short name of this soundcard */
+ char longname[40]; /* name of this soundcard */

/* one and only blocking message or notification may be pending */
u32 pending_event;
diff --git a/sound/pci/pcxhr/pcxhr.h b/sound/pci/pcxhr/pcxhr.h
index 9e39e509a3ef..4909a43ce3d9 100644
--- a/sound/pci/pcxhr/pcxhr.h
+++ b/sound/pci/pcxhr/pcxhr.h
@@ -75,8 +75,8 @@ struct pcxhr_mgr {
unsigned long port[3];

/* share the name */
- char shortname[32]; /* short name of this soundcard */
- char longname[96]; /* name of this soundcard */
+ char shortname[16]; /* short name of this soundcard */
+ char longname[40]; /* name of this soundcard */

struct pcxhr_rmh *prmh;

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 254c3d040118..a1cbf5938a0e 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -2061,7 +2061,7 @@ static int snd_hdspm_create_midi(struct snd_card *card,
struct hdspm *hdspm, int id)
{
int err;
- char buf[32];
+ char buf[64];

hdspm->midi[id].id = id;
hdspm->midi[id].hdspm = hdspm;
--
2.9.0

[Arnd Bergmann]

gcc notices that large queue numbers would overflow the queue name
string:

drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c: In function 'bnx2x_get_strings':
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:25: error: '%d' directive writing between 1 and 10 bytes into a region of size 5 [-Werror=format-overflow=]
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:25: note: directive argument in the range [0, 2147483647]
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:5: note: 'sprintf' output between 2 and 11 bytes into a destination of size 5

There is a hard limit in place that makes the number at most two
digits, so the code is fine. This changes it to use snprintf()
to truncate instead of overflowing, which shuts up that warning.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 21bc4bed6b26..1e33abde4a3e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -3162,7 +3162,8 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
if (is_multi(bp)) {
for_each_eth_queue(bp, i) {
memset(queue_name, 0, sizeof(queue_name));
- sprintf(queue_name, "%d", i);
+ snprintf(queue_name, sizeof(queue_name),
+ "%d", i);
for (j = 0; j < BNX2X_NUM_Q_STATS; j++)
snprintf(buf + (k + j)*ETH_GSTRING_LEN,
ETH_GSTRING_LEN,
--
2.9.0

[Arnd Bergmann]

gcc-7 points out that a negative port_num value would overflow
the string buffer:

drivers/infiniband/hw/mlx4/sysfs.c: In function 'mlx4_ib_device_register_sysfs':
drivers/infiniband/hw/mlx4/sysfs.c:251:16: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
drivers/infiniband/hw/mlx4/sysfs.c:251:2: note: 'sprintf' output between 2 and 11 bytes into a destination of size 10
drivers/infiniband/hw/mlx4/sysfs.c:303:17: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
drivers/infiniband/hw/mlx4/sysfs.c:303:3: note: 'sprintf' output between 2 and 11 bytes into a destination of size 10

While we should be able to assume that port_num is positive here,
making the buffer one byte longer has no downsides and avoids the
warning.

Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/infiniband/hw/mlx4/sysfs.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
index 0ba5ba7540c8..e219093d2764 100644
--- a/drivers/infiniband/hw/mlx4/sysfs.c
+++ b/drivers/infiniband/hw/mlx4/sysfs.c
@@ -221,7 +221,7 @@ void del_sysfs_port_mcg_attr(struct mlx4_ib_dev *device, int port_num,
static int add_port_entries(struct mlx4_ib_dev *device, int port_num)
{
int i;
- char buff[10];
+ char buff[11];
struct mlx4_ib_iov_port *port = NULL;
int ret = 0 ;
struct ib_port_attr attr;
--
2.9.0

[Arnd Bergmann]

gcc reports that the temporary buffer for computing the
string length may be too small here:

drivers/net/ethernet/cavium/liquidio/lio_ethtool.c: In function 'lio_get_eeprom_len':
/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c:345:21: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
len = sprintf(buf, "boardname:%s serialnum:%s maj:%lld min:%lld\n",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/cavium/liquidio/lio_ethtool.c:345:6: note: 'sprintf' output between 35 and 167 bytes into a destination of size 128
len = sprintf(buf, "boardname:%s serialnum:%s maj:%lld min:%lld\n",

This extends it to 192 bytes, which is certainly enough. As far
as I could tell, there are no other constraints that require a specific
maximum size.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/net/ethernet/cavium/liquidio/lio_ethtool.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
index 28ecda3d3404..ebd353bc78ff 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
@@ -335,7 +335,7 @@ lio_ethtool_get_channels(struct net_device *dev,

static int lio_get_eeprom_len(struct net_device *netdev)
{
- u8 buf[128];
+ u8 buf[192];
struct lio *lio = GET_LIO(netdev);
struct octeon_device *oct_dev = lio->oct_dev;
struct octeon_board_info *board_info;
--
2.9.0

[Arnd Bergmann]

One string we pass into the cs->info buffer might be too long,
as pointed out by gcc:

drivers/isdn/divert/isdn_divert.c: In function 'll_callback':
drivers/isdn/divert/isdn_divert.c:488:22: error: '%d' directive writing between 1 and 3 bytes into a region of size between 1 and 69 [-Werror=format-overflow=]
sprintf(cs->info, "%d 0x%lx %s %s %s %s 0x%x 0x%x %d %d %s\n",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/isdn/divert/isdn_divert.c:488:22: note: directive argument in the range [0, 255]
drivers/isdn/divert/isdn_divert.c:488:4: note: 'sprintf' output 25 or more bytes (assuming 129) into a destination of size 90

This is unlikely to actually cause problems, so let's use snprintf
as a simple workaround to shut up the warning and truncate the
buffer instead.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/isdn/divert/isdn_divert.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/isdn/divert/isdn_divert.c b/drivers/isdn/divert/isdn_divert.c
index 060d357f107f..6f423bc49d0d 100644
--- a/drivers/isdn/divert/isdn_divert.c
+++ b/drivers/isdn/divert/isdn_divert.c
@@ -485,18 +485,19 @@ static int isdn_divert_icall(isdn_ctrl *ic)
cs->deflect_dest[0] = '\0';
retval = 4; /* only proceed */
}
- sprintf(cs->info, "%d 0x%lx %s %s %s %s 0x%x 0x%x %d %d %s\n",
- cs->akt_state,
- cs->divert_id,
- divert_if.drv_to_name(cs->ics.driver),
- (ic->command == ISDN_STAT_ICALLW) ? "1" : "0",
- cs->ics.parm.setup.phone,
- cs->ics.parm.setup.eazmsn,
- cs->ics.parm.setup.si1,
- cs->ics.parm.setup.si2,
- cs->ics.parm.setup.screen,
- dv->rule.waittime,
- cs->deflect_dest);
+ snprintf(cs->info, sizeof(cs->info),
+ "%d 0x%lx %s %s %s %s 0x%x 0x%x %d %d %s\n",
+ cs->akt_state,
+ cs->divert_id,
+ divert_if.drv_to_name(cs->ics.driver),
+ (ic->command == ISDN_STAT_ICALLW) ? "1" : "0",
+ cs->ics.parm.setup.phone,
+ cs->ics.parm.setup.eazmsn,
+ cs->ics.parm.setup.si1,
+ cs->ics.parm.setup.si2,
+ cs->ics.parm.setup.screen,
+ dv->rule.waittime,
+ cs->deflect_dest);
if ((dv->rule.action == DEFLECT_REPORT) ||
(dv->rule.action == DEFLECT_REJECT)) {
put_info_buffer(cs->info);
--
2.9.0

[Arnd Bergmann]

gcc-7 warns that the key might exceed five bytes for lage index
values:

drivers/hwmon/applesmc.c: In function 'applesmc_show_fan_position':
drivers/hwmon/applesmc.c:906:18: error: '%d' directive writing between 1 and 5 bytes into a region of size 4 [-Werror=format-overflow=]
sprintf(newkey, FAN_ID_FMT, to_index(attr));
^~~~~~~
drivers/hwmon/applesmc.c:906:18: note: directive argument in the range [0, 65535]
drivers/hwmon/applesmc.c:906:2: note: 'sprintf' output between 5 and 9 bytes into a destination of size 5

As the key is required to be four characters plus trailing zero,
we know that the index has to be small here. I'm using snprintf()
to avoid the warning. This would truncate the string instead of
overflowing.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/hwmon/applesmc.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 0af7fd311979..515163b9a89f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -903,7 +903,7 @@ static ssize_t applesmc_show_fan_position(struct device *dev,
char newkey[5];
u8 buffer[17];

- sprintf(newkey, FAN_ID_FMT, to_index(attr));
+ snprintf(newkey, sizeof(newkey), FAN_ID_FMT, to_index(attr));

ret = applesmc_read_key(newkey, buffer, 16);
buffer[16] = 0;
--
2.9.0

[Arnd Bergmann]

gcc points out a minor bug in the handling of unknown
cookie types, which could result in a string overflow
when the integer is copied into a 3-byte string:

fs/fscache/object-list.c: In function 'fscache_objlist_show':
fs/fscache/object-list.c:265:19: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
sprintf(_type, "%02u", cookie->def->type);
^~~~~~
fs/fscache/object-list.c:265:4: note: 'sprintf' output between 3 and 4 bytes into a destination of size 3

This is currently harmless as no code sets a type other
than 0 or 1, but it makes sense to use snprintf() here
to avoid overflowing the array if that changes.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

fs/fscache/object-list.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
index 67f940892ef8..b5ab06fabc60 100644
--- a/fs/fscache/object-list.c
+++ b/fs/fscache/object-list.c
@@ -262,7 +262,8 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
type = "DT";
break;
default:
- sprintf(_type, "%02u", cookie->def->type);
+ snprintf(_type, sizeof(_type), "%02u",
+ cookie->def->type);
type = _type;
break;
}
--
2.9.0

[Arnd Bergmann]

gcc-7 points out that a large controller number would overflow the
string length for the procfs name and the firmware version string:

drivers/block/DAC960.c: In function 'DAC960_Probe':
drivers/block/DAC960.c:6591:38: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
drivers/block/DAC960.c: In function 'DAC960_V1_ReadControllerConfiguration':
drivers/block/DAC960.c:1681:40: error: '%02d' directive writing between 2 and 3 bytes into a region of size between 2 and 5 [-Werror=format-overflow=]
drivers/block/DAC960.c:1681:40: note: directive argument in the range [0, 255]
drivers/block/DAC960.c:1681:3: note: 'sprintf' output between 10 and 14 bytes into a destination of size 12

Both of these seem appropriately sized, and using snprintf()
instead of sprintf() improves this by ensuring that even
incorrect data won't cause undefined behavior here.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

drivers/block/DAC960.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/block/DAC960.c b/drivers/block/DAC960.c
index 245a879b036e..255591ab3716 100644
--- a/drivers/block/DAC960.c
+++ b/drivers/block/DAC960.c
@@ -1678,9 +1678,12 @@ static bool DAC960_V1_ReadControllerConfiguration(DAC960_Controller_T
Enquiry2->FirmwareID.FirmwareType = '0';
Enquiry2->FirmwareID.TurnID = 0;
}
- sprintf(Controller->FirmwareVersion, "%d.%02d-%c-%02d",
- Enquiry2->FirmwareID.MajorVersion, Enquiry2->FirmwareID.MinorVersion,
- Enquiry2->FirmwareID.FirmwareType, Enquiry2->FirmwareID.TurnID);
+ snprintf(Controller->FirmwareVersion, sizeof(Controller->FirmwareVersion),
+ "%d.%02d-%c-%02d",
+ Enquiry2->FirmwareID.MajorVersion,
+ Enquiry2->FirmwareID.MinorVersion,
+ Enquiry2->FirmwareID.FirmwareType,
+ Enquiry2->FirmwareID.TurnID);
if (!((Controller->FirmwareVersion[0] == '5' &&
strcmp(Controller->FirmwareVersion, "5.06") >= 0) ||
(Controller->FirmwareVersion[0] == '4' &&
@@ -6588,7 +6591,8 @@ static void DAC960_CreateProcEntries(DAC960_Controller_T *Controller)
&dac960_proc_fops);
}

- sprintf(Controller->ControllerName, "c%d", Controller->ControllerNumber);
+ snprintf(Controller->ControllerName, sizeof(Controller->ControllerName),
+ "c%d", Controller->ControllerNumber);
ControllerProcEntry = proc_mkdir(Controller->ControllerName,
DAC960_ProcDirectoryEntry);
proc_create_data("initial_status", 0, ControllerProcEntry, &dac960_initial_status_proc_fops, Controller);
--
2.9.0

[Takashi Iwai]

Thanks for the patch. I have seen it but ignored, so far, as not sure
which action is the best. An alternative solution is to use
snprintf() blindly, for example.

For mixart, it's even better to drop mgr->shortname[] and longname[]
assignment. The shortname is the fixed string, and the longname is
used only at copying to card->longname, so we can create a string
there from the scratch.


Takashi

[Robin Murphy]

On 14/07/17 13:07, Arnd Bergmann wrote:
> gcc warns that the temporary buffer might be too small here:
>
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c: In function 'bgx_probe':
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: error: '%d' directive writing between 1 and 10 bytes into a region of size between 9 and 11 [-Werror=format-overflow=]
> sprintf(str, "BGX%d LMAC%d mode", bgx->bgx_id, lmacid);
> ^~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: note: directive argument in the range [0, 2147483647]
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:3: note: 'sprintf' output between 16 and 27 bytes into a destination of size 20
>
> This probably can't happen, but it can't hurt to make it long
> enough for the theoretical limit.

Probably indeed - both bgx_id and lmacid are u8 here, which would make
the maximum length of that string, including null terminator, exactly 20
characters.

So in this case the warning is not only silly, it's actively wrong;
sure, the arguments themselves are being promoted to ints at that point,
but GCC *knows* the original type, or it couldn't have generated the
correct code for the call :/

Robin.

[Andy Shevchenko]

On Fri, 2017-07-14 at 14:07 +0200, Arnd Bergmann wrote:
> gcc-7 notices that the pin_table is an array of 16-bit numbers,
> but we assume it can be printed as a two-character hexadecimal
> string:
>
> drivers/gpio/gpiolib-acpi.c: In function
> 'acpi_gpiochip_request_interrupt':
> drivers/gpio/gpiolib-acpi.c:206:24: warning: '%02X' directive writing
> between 2 and 4 bytes into a region of size 3 [-Wformat-overflow=]
>    sprintf(ev_name, "_%c%02X",
>                         ^~~~
> drivers/gpio/gpiolib-acpi.c:206:20: note: directive argument in the
> range [0, 65535]
>    sprintf(ev_name, "_%c%02X",
>                     ^~~~~~~~~
> drivers/gpio/gpiolib-acpi.c:206:3: note: 'sprintf' output between 5
> and 7 bytes into a destination of size 5
>    sprintf(ev_name, "_%c%02X",
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>     agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
>     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     pin);
>     ~~~~

This is obviously a false positive warning.

Here we have
int pin = u16 pin_table[0] <= 255 (implying >= 0).

I see few options how to make it more clear
1) your proposal;
2) use "%02hhX" instead;
3) use if (ret >= 0 && ret <= 255) condition.

I would choose one of the 2-3.

In case gcc will complain about 3), file a bug to gcc crazy warning.

>
> This can't be right, so this changes it to truncate the number to
> an 8-bit pin number.
>
> Fixes: 0d1c28a449c6 ("gpiolib-acpi: Add ACPI5 event model support to
> gpio.")
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---
>  drivers/gpio/gpiolib-acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c9b42dd12dfa..c3faea724af8 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -205,7 +205,7 @@ static acpi_status
> acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
>   char ev_name[5];
>   sprintf(ev_name, "_%c%02X",
>   agpio->triggering == ACPI_EDGE_SENSITIVE ?
> 'E' : 'L',
> - pin);
> + (u8)pin);
>   if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name,
> &evt_handle)))
>   handler = acpi_gpio_irq_handler;
>   }

--

Andy Shevchenko <andriy.s...@linux.intel.com>
Intel Finland Oy

[Leon Romanovsky]

On Fri, Jul 14, 2017 at 02:07:14PM +0200, Arnd Bergmann wrote:
> gcc-7 points out that a negative port_num value would overflow
> the string buffer:
>
> drivers/infiniband/hw/mlx4/sysfs.c: In function 'mlx4_ib_device_register_sysfs':
> drivers/infiniband/hw/mlx4/sysfs.c:251:16: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
> drivers/infiniband/hw/mlx4/sysfs.c:251:2: note: 'sprintf' output between 2 and 11 bytes into a destination of size 10
> drivers/infiniband/hw/mlx4/sysfs.c:303:17: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
> drivers/infiniband/hw/mlx4/sysfs.c:303:3: note: 'sprintf' output between 2 and 11 bytes into a destination of size 10
>
> While we should be able to assume that port_num is positive here,
> making the buffer one byte longer has no downsides and avoids the
> warning.
>
> Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---
> drivers/infiniband/hw/mlx4/sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

[Guenter Roeck]

On 07/14/2017 05:07 AM, Arnd Bergmann wrote:
> gcc-7 warns that the key might exceed five bytes for lage index
> values:
>
> drivers/hwmon/applesmc.c: In function 'applesmc_show_fan_position':
> drivers/hwmon/applesmc.c:906:18: error: '%d' directive writing between 1 and 5 bytes into a region of size 4 [-Werror=format-overflow=]
> sprintf(newkey, FAN_ID_FMT, to_index(attr));
> ^~~~~~~
> drivers/hwmon/applesmc.c:906:18: note: directive argument in the range [0, 65535]
> drivers/hwmon/applesmc.c:906:2: note: 'sprintf' output between 5 and 9 bytes into a destination of size 5
>
> As the key is required to be four characters plus trailing zero,
> we know that the index has to be small here. I'm using snprintf()
> to avoid the warning. This would truncate the string instead of
> overflowing.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

I submitted a more comprehensive patch a couple of days ago. There are other similar
sprintf() calls in the driver which gcc doesn't report.

Guenter

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:01 +0200

> We get a warning for the port_name string that might be longer than
> six characters if we had more than 10 ports:
>
> drivers/net/ethernet/sun/niu.c: In function 'niu_put_parent':
> drivers/net/ethernet/sun/niu.c:9563:21: error: '%d' directive writing between 1 and 3 bytes into a region of size 2 [-Werror=format-overflow=]
> sprintf(port_name, "port%d", port);
> ^~~~~~~~
> drivers/net/ethernet/sun/niu.c:9563:21: note: directive argument in the range [0, 255]
> drivers/net/ethernet/sun/niu.c:9563:2: note: 'sprintf' output between 6 and 8 bytes into a destination of size 6
> sprintf(port_name, "port%d", port);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/sun/niu.c: In function 'niu_pci_init_one':
> drivers/net/ethernet/sun/niu.c:9538:22: error: '%d' directive writing between 1 and 3 bytes into a region of size 2 [-Werror=format-overflow=]
> sprintf(port_name, "port%d", port);
> ^~~~~~~~
> drivers/net/ethernet/sun/niu.c:9538:22: note: directive argument in the range [0, 255]
> drivers/net/ethernet/sun/niu.c:9538:3: note: 'sprintf' output between 6 and 8 bytes into a destination of size 6
>
> While we know that the port number is small, there is no harm in
> making the format string two bytes longer to avoid the warning.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Applied.

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:02 +0200

> gcc notices that large queue numbers would overflow the queue name
> string:
>
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c: In function 'bnx2x_get_strings':
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:25: error: '%d' directive writing between 1 and 10 bytes into a region of size 5 [-Werror=format-overflow=]
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:25: note: directive argument in the range [0, 2147483647]
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3165:5: note: 'sprintf' output between 2 and 11 bytes into a destination of size 5
>
> There is a hard limit in place that makes the number at most two
> digits, so the code is fine. This changes it to use snprintf()
> to truncate instead of overflowing, which shuts up that warning.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Applied.

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:04 +0200

> gcc-7 notices that "-event-%d" could be more than 11 characters long
> if we had larger 'vector' numbers:
>
> drivers/net/vmxnet3/vmxnet3_drv.c: In function 'vmxnet3_activate_dev':
> drivers/net/vmxnet3/vmxnet3_drv.c:2095:40: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
> sprintf(intr->event_msi_vector_name, "%s-event-%d",
> ^~~~~~~~~~~~~
> drivers/net/vmxnet3/vmxnet3_drv.c:2095:3: note: 'sprintf' output between 9 and 33 bytes into a destination of size 32
>
> The current code is safe, but making the string a little longer
> is harmless and lets gcc see that it's ok.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Applied.

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:00 +0200

> One string we pass into the cs->info buffer might be too long,
> as pointed out by gcc:
>
> drivers/isdn/divert/isdn_divert.c: In function 'll_callback':
> drivers/isdn/divert/isdn_divert.c:488:22: error: '%d' directive writing between 1 and 3 bytes into a region of size between 1 and 69 [-Werror=format-overflow=]
> sprintf(cs->info, "%d 0x%lx %s %s %s %s 0x%x 0x%x %d %d %s\n",
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/isdn/divert/isdn_divert.c:488:22: note: directive argument in the range [0, 255]
> drivers/isdn/divert/isdn_divert.c:488:4: note: 'sprintf' output 25 or more bytes (assuming 129) into a destination of size 90
>
> This is unlikely to actually cause problems, so let's use snprintf
> as a simple workaround to shut up the warning and truncate the
> buffer instead.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Applied.

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:03 +0200

> gcc warns that the temporary buffer might be too small here:
>
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c: In function 'bgx_probe':
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: error: '%d' directive writing between 1 and 10 bytes into a region of size between 9 and 11 [-Werror=format-overflow=]
> sprintf(str, "BGX%d LMAC%d mode", bgx->bgx_id, lmacid);
> ^~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:16: note: directive argument in the range [0, 2147483647]
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c:1020:3: note: 'sprintf' output between 16 and 27 bytes into a destination of size 20
>
> This probably can't happen, but it can't hurt to make it long
> enough for the theoretical limit.
>

[David Miller]

From: Arnd Bergmann <ar...@arndb.de>
Date: Fri, 14 Jul 2017 14:07:05 +0200

> gcc reports that the temporary buffer for computing the
> string length may be too small here:
>
> drivers/net/ethernet/cavium/liquidio/lio_ethtool.c: In function 'lio_get_eeprom_len':
> /drivers/net/ethernet/cavium/liquidio/lio_ethtool.c:345:21: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
> len = sprintf(buf, "boardname:%s serialnum:%s maj:%lld min:%lld\n",
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/cavium/liquidio/lio_ethtool.c:345:6: note: 'sprintf' output between 35 and 167 bytes into a destination of size 128
> len = sprintf(buf, "boardname:%s serialnum:%s maj:%lld min:%lld\n",
>
> This extends it to 192 bytes, which is certainly enough. As far
> as I could tell, there are no other constraints that require a specific
> maximum size.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Applied.

[Mario.Li...@dell.com]

> -----Original Message-----
> From: Arnd Bergmann [mailto:ar...@arndb.de]
> Sent: Friday, July 14, 2017 7:07 AM
> To: linux-...@vger.kernel.org; Darren Hart <dvh...@infradead.org>; Andy
> Shevchenko <an...@infradead.org>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; Linus Torvalds
> <torv...@linux-foundation.org>; Guenter Roeck <li...@roeck-us.net>;
> ak...@linux-foundation.org; net...@vger.kernel.org; David S . Miller
> <da...@davemloft.net>; James E . J . Bottomley <je...@linux.vnet.ibm.com>;
> Martin K . Petersen <martin....@oracle.com>; linux...@vger.kernel.org;
> x...@kernel.org; Arnd Bergmann <ar...@arndb.de>; Limonciello, Mario
> <Mario_Li...@Dell.com>; Arvind Yadav <arvind....@gmail.com>;
> platform-...@vger.kernel.org
> Subject: [PATCH 17/22] platform/x86: alienware-wmi: fix format string overflow
> warning

LGTM, Thanks.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>

[Andy Shevchenko]

On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <ar...@arndb.de> wrote:
> gcc points out a possible format string overflow for a large value of 'zone':
>
> drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
> drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
> sprintf(buffer, "zone%02X", i);
> ^~~~
> drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
> sprintf(buffer, "zone%02X", i);
> ^~~~~~~~~~
> drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10
>
> While the zone should never be that large, it's easy to make the
> buffer a few bytes longer so gcc can prove this to be safe.

Please, be a bit smarter on such fixes.

Here we need to convert

int i;

to

u8 i;

I will take it after addressing above.

P.S. You may do this change across the file.

> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---
> drivers/platform/x86/alienware-wmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
> index 0831b428c217..acc01242da82 100644
> --- a/drivers/platform/x86/alienware-wmi.c
> +++ b/drivers/platform/x86/alienware-wmi.c
> @@ -421,7 +421,7 @@ static DEVICE_ATTR(lighting_control_state, 0644, show_control_state,
> static int alienware_zone_init(struct platform_device *dev)
> {
> int i;
> - char buffer[10];
> + char buffer[13];
> char *name;
>
> if (interface == WMAX) {
> --
> 2.9.0
>

--
With Best Regards,
Andy Shevchenko

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 9:18 PM, Andy Shevchenko
<andy.sh...@gmail.com> wrote:
> On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <ar...@arndb.de> wrote:
>> gcc points out a possible format string overflow for a large value of 'zone':
>>
>> drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
>> drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
>> sprintf(buffer, "zone%02X", i);
>> ^~~~
>> drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
>> sprintf(buffer, "zone%02X", i);
>> ^~~~~~~~~~
>> drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10
>>
>> While the zone should never be that large, it's easy to make the
>> buffer a few bytes longer so gcc can prove this to be safe.
>
> Please, be a bit smarter on such fixes.
>
> Here we need to convert
>
> int i;
>
> to
>
> u8 i;

That was my first impulse, but then I decided not to change the
idiomatic 'int i' for the index variable to 'u8' as that would be
less idiomatic.

> I will take it after addressing above.
>
> P.S. You may do this change across the file.

How about changing it to 'u8 zone'?

Arnd

[Andy Shevchenko]

On Fri, Jul 14, 2017 at 10:37 PM, Arnd Bergmann <ar...@arndb.de> wrote:
> On Fri, Jul 14, 2017 at 9:18 PM, Andy Shevchenko
> <andy.sh...@gmail.com> wrote:
>> On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <ar...@arndb.de> wrote:
>>> gcc points out a possible format string overflow for a large value of 'zone':

>> Here we need to convert
>>
>> int i;
>>
>> to
>>
>> u8 i;
>
> That was my first impulse, but then I decided not to change the
> idiomatic 'int i' for the index variable to 'u8' as that would be
> less idiomatic.
>
>> I will take it after addressing above.
>>
>> P.S. You may do this change across the file.
>
> How about changing it to 'u8 zone'?

I'm ultimately fine with that (just gentle reminder you might fix all
3 occurrences of it in that driver).

[Arnd Bergmann]

Makes sense. I didn't remember the syntax for 2) and couldn't find
it in the man page when I first looked. This seems like a good solution
here.

I'm pretty sure I tried 3) a few times when the warning first showed
up last year, but couldn't get that to work. Filing a gcc bug also seems
like a good idea, but I should first see if it's already fixed. The version
I use for testing at the moment is from late April, and others may
have complained about that already.

Arnd

[David Laight]

From: Arnd Bergmann
> Sent: 14 July 2017 13:07
> gcc points out a theorerical string overflow:
>
> drivers/message/fusion/mptbase.c: In function 'mpt_detach':
> drivers/message/fusion/mptbase.c:2103:17: error: '%s' directive writing up to 31 bytes into a region
> of size 28 [-Werror=format-overflow=]
> sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s/summary", ioc->name);
> ^~~~~
> drivers/message/fusion/mptbase.c:2103:2: note: 'sprintf' output between 13 and 44 bytes into a
> destination of size 32
>
> We can simply double the size of the local buffer here to be on the
> safe side.

I think I'd change it to snprintf() as well.
Saves any worries if ioc->name isn't '\0' terminated.

David

[Arnd Bergmann]

Ok, fair enough, I'll send a new version right away.

Arnd

[Arnd Bergmann]

gcc points out a theorerical string overflow:

drivers/message/fusion/mptbase.c: In function 'mpt_detach':
drivers/message/fusion/mptbase.c:2103:17: error: '%s' directive writing up to 31 bytes into a region of size 28 [-Werror=format-overflow=]
sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s/summary", ioc->name);
^~~~~
drivers/message/fusion/mptbase.c:2103:2: note: 'sprintf' output between 13 and 44 bytes into a destination of size 32

We can simply double the size of the local buffer here to be on the

safe side, and using snprintf() instead of sprintf() protects us
if ioc->name was not terminated properly.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

v2: also use snprintf
---
drivers/message/fusion/mptbase.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 62cff5afc6bd..84eab28665f3 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -2079,7 +2079,7 @@ void
mpt_detach(struct pci_dev *pdev)
{
MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
- char pname[32];
+ char pname[64];
u8 cb_idx;
unsigned long flags;
struct workqueue_struct *wq;
@@ -2100,11 +2100,11 @@ mpt_detach(struct pci_dev *pdev)
spin_unlock_irqrestore(&ioc->fw_event_lock, flags);
destroy_workqueue(wq);

- sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s/summary", ioc->name);
+ snprintf(pname, sizeof(pname), MPT_PROCFS_MPTBASEDIR "/%s/summary", ioc->name);
remove_proc_entry(pname, NULL);
- sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s/info", ioc->name);
+ snprintf(pname, sizeof(pname), MPT_PROCFS_MPTBASEDIR "/%s/info", ioc->name);
remove_proc_entry(pname, NULL);
- sprintf(pname, MPT_PROCFS_MPTBASEDIR "/%s", ioc->name);
+ snprintf(pname, sizeof(pname), MPT_PROCFS_MPTBASEDIR "/%s", ioc->name);
remove_proc_entry(pname, NULL);

/* call per device driver remove entry point */
--
2.9.0

[Hans Verkuil]

On 17/07/17 14:57, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 2:53 PM, Hans Verkuil <hver...@xs4all.nl> wrote:
>> On 14/07/17 14:07, Arnd Bergmann wrote:
>>> gcc-7 notices that we copy a fixed length string into another
>>> string of the same size, with additional characters:
>>>
>>> drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
>>> drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
>>> sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
>>> ^~~~~~~~~~
>>> drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48
>>>
>>> We know this is fine as the template name is always "usbvision", so
>>> we can easily avoid the warning by using this as the format string
>>> directly.
>>
>> Hmm, how about replacing sprintf by snprintf? That feels a lot safer (this is very
>> old code, it's not surprising it is still using sprintf).
>
> With snprintf(), you will still get a -Wformat-truncation warning. One
> of my patches
> disables that warning by default, but Mauro likes build-testing with
> "make W=1", so
> it would still show up then.
>
> However, we can do both: replace the string and use snprintf().

Yes please!

Regards,

Hans

[Arnd Bergmann]

Arnd

[Hans Verkuil]

On 14/07/17 14:07, Arnd Bergmann wrote:
> gcc-7 notices that we copy a fixed length string into another
> string of the same size, with additional characters:
>
> drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
> drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
> sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
> ^~~~~~~~~~
> drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48
>
> We know this is fine as the template name is always "usbvision", so
> we can easily avoid the warning by using this as the format string
> directly.

Hmm, how about replacing sprintf by snprintf? That feels a lot safer (this is very
old code, it's not surprising it is still using sprintf).

Regards,

Hans

>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---

> drivers/media/usb/usbvision/usbvision-i2c.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/usbvision/usbvision-i2c.c b/drivers/media/usb/usbvision/usbvision-i2c.c
> index fdf6b6e285da..aae9f69884da 100644
> --- a/drivers/media/usb/usbvision/usbvision-i2c.c
> +++ b/drivers/media/usb/usbvision/usbvision-i2c.c
> @@ -187,8 +187,8 @@ int usbvision_i2c_register(struct usb_usbvision *usbvision)
>
> usbvision->i2c_adap = i2c_adap_template;
>
> - sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
> - usbvision->dev->bus->busnum, usbvision->dev->devpath);
> + sprintf(usbvision->i2c_adap.name, "usbvision-%d-%s",
> + usbvision->dev->bus->busnum, usbvision->dev->devpath);
> PDEBUG(DBG_I2C, "Adaptername: %s", usbvision->i2c_adap.name);
> usbvision->i2c_adap.dev.parent = &usbvision->dev->dev;
>
>

[Arnd Bergmann]

gcc-7 notices that we copy a fixed length string into another
string of the same size, with additional characters:

drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
^~~~~~~~~~
drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48

Using snprintf() makes the code more robust in general, but will still
trigger a possible warning about truncation in the string.
We know this won't happen as the template name is always "usbvision", so
we can easily avoid the warning as well by using this as the format string
directly.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

v2: use snprintf()
---
drivers/media/usb/usbvision/usbvision-i2c.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbvision/usbvision-i2c.c b/drivers/media/usb/usbvision/usbvision-i2c.c
index fdf6b6e285da..38749331e7df 100644
--- a/drivers/media/usb/usbvision/usbvision-i2c.c
+++ b/drivers/media/usb/usbvision/usbvision-i2c.c
@@ -187,8 +187,9 @@ int usbvision_i2c_register(struct usb_usbvision *usbvision)

usbvision->i2c_adap = i2c_adap_template;

- sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
- usbvision->dev->bus->busnum, usbvision->dev->devpath);

+ snprintf(usbvision->i2c_adap.name, sizeof(usbvision->i2c_adap.name),
+ "usbvision-%d-%s",

+ usbvision->dev->bus->busnum, usbvision->dev->devpath);
PDEBUG(DBG_I2C, "Adaptername: %s", usbvision->i2c_adap.name);
usbvision->i2c_adap.dev.parent = &usbvision->dev->dev;

--
2.9.0

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 2:28 PM, Takashi Iwai <ti...@suse.de> wrote:
> On Fri, 14 Jul 2017 14:07:12 +0200,
>

> Thanks for the patch. I have seen it but ignored, so far, as not sure
> which action is the best. An alternative solution is to use
> snprintf() blindly, for example.
>
> For mixart, it's even better to drop mgr->shortname[] and longname[]
> assignment. The shortname is the fixed string, and the longname is
> used only at copying to card->longname, so we can create a string
> there from the scratch.

I've done that now, and tried to be a little smarter with the other
conversions. I also found related problems in ISA drivers after
randconfig testing and fixed those as well.

Sent a 7-patch series now as a replacement.

Arnd

[Arnd Bergmann]

gcc points out a possible format string overflow for a large value of 'zone':

drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
sprintf(buffer, "zone%02X", i);
^~~~
drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
sprintf(buffer, "zone%02X", i);
^~~~~~~~~~

drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10

This replaces the 'int' variable with an 'u8' to make sure
it always fits, renaming the variable to 'zone' for clarity.

Unfortunately, gcc-7.1.1 still warns about it with that change, which
seems to be unintended by the gcc developers. I have opened a bug
against gcc with a reduced test case. As a workaround, I also
change the format string to use "%02hhX", which shuts up the
warning in that version.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81483
Link: https://patchwork.ozlabs.org/patch/788415/
Suggested-by: Andy Shevchenko <an...@infradead.org>

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

v2: completely rewrite the patch after discussing with Andy
---
drivers/platform/x86/alienware-wmi.c | 38 ++++++++++++++++++------------------
1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index 0831b428c217..c56a9ecba959 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -255,12 +255,12 @@ static int parse_rgb(const char *buf, struct platform_zone *zone)

static struct platform_zone *match_zone(struct device_attribute *attr)
{
- int i;
- for (i = 0; i < quirks->num_zones; i++) {
- if ((struct device_attribute *)zone_data[i].attr == attr) {
+ u8 zone;
+ for (zone = 0; zone < quirks->num_zones; zone++) {
+ if ((struct device_attribute *)zone_data[zone].attr == attr) {
pr_debug("alienware-wmi: matched zone location: %d\n",
- zone_data[i].location);
- return &zone_data[i];
+ zone_data[zone].location);
+ return &zone_data[zone];
}
}
return NULL;
@@ -420,7 +420,7 @@ static DEVICE_ATTR(lighting_control_state, 0644, show_control_state,

static int alienware_zone_init(struct platform_device *dev)
{

- int i;
+ u8 zone;
char buffer[10];
char *name;

@@ -457,19 +457,19 @@ static int alienware_zone_init(struct platform_device *dev)
if (!zone_data)
return -ENOMEM;

- for (i = 0; i < quirks->num_zones; i++) {
- sprintf(buffer, "zone%02X", i);
+ for (zone = 0; zone < quirks->num_zones; zone++) {
+ sprintf(buffer, "zone%02hhX", zone);
name = kstrdup(buffer, GFP_KERNEL);
if (name == NULL)
return 1;
- sysfs_attr_init(&zone_dev_attrs[i].attr);
- zone_dev_attrs[i].attr.name = name;
- zone_dev_attrs[i].attr.mode = 0644;
- zone_dev_attrs[i].show = zone_show;
- zone_dev_attrs[i].store = zone_set;
- zone_data[i].location = i;
- zone_attrs[i] = &zone_dev_attrs[i].attr;
- zone_data[i].attr = &zone_dev_attrs[i];
+ sysfs_attr_init(&zone_dev_attrs[zone].attr);
+ zone_dev_attrs[zone].attr.name = name;
+ zone_dev_attrs[zone].attr.mode = 0644;
+ zone_dev_attrs[zone].show = zone_show;
+ zone_dev_attrs[zone].store = zone_set;
+ zone_data[zone].location = zone;
+ zone_attrs[zone] = &zone_dev_attrs[zone].attr;
+ zone_data[zone].attr = &zone_dev_attrs[zone];
}
zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
zone_attribute_group.attrs = zone_attrs;
@@ -484,9 +484,9 @@ static void alienware_zone_exit(struct platform_device *dev)
sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group);
led_classdev_unregister(&global_led);
if (zone_dev_attrs) {
- int i;
- for (i = 0; i < quirks->num_zones; i++)
- kfree(zone_dev_attrs[i].attr.name);
+ u8 zone;
+ for (zone = 0; zone < quirks->num_zones; zone++)
+ kfree(zone_dev_attrs[zone].attr.name);
}
kfree(zone_dev_attrs);
kfree(zone_data);
--
2.9.0

[Andy Shevchenko]

On Thu, Jul 20, 2017 at 7:00 PM, Arnd Bergmann <ar...@arndb.de> wrote:
> gcc points out a possible format string overflow for a large value of 'zone':
>
> drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
> drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
> sprintf(buffer, "zone%02X", i);
> ^~~~
> drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
> sprintf(buffer, "zone%02X", i);
> ^~~~~~~~~~
> drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10
>
> This replaces the 'int' variable with an 'u8' to make sure
> it always fits, renaming the variable to 'zone' for clarity.
>
> Unfortunately, gcc-7.1.1 still warns about it with that change, which
> seems to be unintended by the gcc developers. I have opened a bug
> against gcc with a reduced test case. As a workaround, I also
> change the format string to use "%02hhX", which shuts up the
> warning in that version.
>

Thanks, pushed to testing with slight change (+ empty lines after u8
zone; where it's applicable).
I'm not going to move this to fixes queue since it looks to me not
critical at all. Drop me a message if you think otherwise.

[Martin K. Petersen]

Arnd,

> This series addresses all warnings that gcc-7 introduces for
> -Wformat-overflow= and turns off the -Wformat-truncation by default
> (they remain enabled with "make W=1").

Applied the SCSI patches. Thanks!

--
Martin K. Petersen Oracle Linux Engineering

[Arnd Bergmann]

On Mon, Jul 24, 2017 at 11:22 AM, Andy Shevchenko
<andy.sh...@gmail.com> wrote:
> On Thu, Jul 20, 2017 at 7:00 PM, Arnd Bergmann <ar...@arndb.de> wrote:
>> gcc points out a possible format string overflow for a large value of 'zone':
>>
>> drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init':
>> drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=]
>> sprintf(buffer, "zone%02X", i);
>> ^~~~
>> drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646]
>> sprintf(buffer, "zone%02X", i);
>> ^~~~~~~~~~
>> drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10
>>
>> This replaces the 'int' variable with an 'u8' to make sure
>> it always fits, renaming the variable to 'zone' for clarity.
>>
>> Unfortunately, gcc-7.1.1 still warns about it with that change, which
>> seems to be unintended by the gcc developers. I have opened a bug
>> against gcc with a reduced test case. As a workaround, I also
>> change the format string to use "%02hhX", which shuts up the
>> warning in that version.
>>
>
> Thanks, pushed to testing with slight change (+ empty lines after u8
> zone; where it's applicable).
> I'm not going to move this to fixes queue since it looks to me not
> critical at all. Drop me a message if you think otherwise.

Sounds good, thanks! This instance is harmless, and the warning is now
globally disabled in stable kernels (and in mainline). I plan to send a patch
to re-enable the warning in mainline once all the other instances are
addressed. I don't think that Greg will backport that patch, but if he does,
then he may need some additional 30 patches besides this one.

Arnd

[Jérémy Lefaure]

> gcc points out a minor bug in the handling of unknown
> cookie types, which could result in a string overflow
> when the integer is copied into a 3-byte string:
>
> fs/fscache/object-list.c: In function 'fscache_objlist_show':
> fs/fscache/object-list.c:265:19: error: 'sprintf' may write a

> terminating nul past the end of the destination

> [-Werror=format-overflow=] sprintf(_type, "%02u", cookie->def->type);
> ^~~~~~ fs/fscache/object-list.c:265:4: note: 'sprintf' output between
> 3 and 4 bytes into a destination of size 3
>
> This is currently harmless as no code sets a type other
> than 0 or 1, but it makes sense to use snprintf() here
> to avoid overflowing the array if that changes.

>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---

Hi,
I sent a patch to fix this issue in April [1]. It was accepted by David
Howells [2]. I don't know why it wasn't upstreamed.

> fs/fscache/object-list.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
> index 67f940892ef8..b5ab06fabc60 100644
> --- a/fs/fscache/object-list.c
> +++ b/fs/fscache/object-list.c
> @@ -262,7 +262,8 @@ static int fscache_objlist_show(struct seq_file
> *m, void *v) type = "DT";
> break;
> default:
> - sprintf(_type, "%02u", cookie->def->type);
> + snprintf(_type, sizeof(_type), "%02u",
> + cookie->def->type);
> type = _type;
> break;
> }
In my patch I didn't use snprintf (which is fine) but I used the
hexadecimal value (as it is in the documentation [3]). Is it too late
to change this patch ? If it is, I can send a patch to use an hex value.

Thank you,
Jérémy

[1]: https://marc.info/?l=linux-kernel&m=149263432022839&w=4
[2]: https://marc.info/?l=linux-kernel&m=149330544916184&w=4
[3]: see Documentation/filesystems/caching/fscache.txt