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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, 14 Jul 2017 14:07:12 +0200,
Arnd Bergmann wrote:
>
> 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 <[email protected]>
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
> ---
> 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
>
>
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.
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> ---
> 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 <[email protected]>
Intel Finland Oy
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 <[email protected]>
> ---
> drivers/infiniband/hw/mlx4/sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Thanks,
Reviewed-by: Leon Romanovsky <[email protected]>
On 07/14/2017 06:07 AM, Arnd Bergmann wrote:
> 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.
Thanks Arnd, added for 4.14.
--
Jens Axboe
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 <[email protected]>
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
> ---
> 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;
>
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
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.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Applied.
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Friday, July 14, 2017 7:07 AM
> To: [email protected]; Darren Hart <[email protected]>; Andy
> Shevchenko <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Linus Torvalds
> <[email protected]>; Guenter Roeck <[email protected]>;
> [email protected]; [email protected]; David S . Miller
> <[email protected]>; James E . J . Bottomley <[email protected]>;
> Martin K . Petersen <[email protected]>; [email protected];
> [email protected]; Arnd Bergmann <[email protected]>; Limonciello, Mario
> <[email protected]>; Arvind Yadav <[email protected]>;
> [email protected]
> Subject: [PATCH 17/22] platform/x86: alienware-wmi: fix format string overflow
> warning
>
> 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 <[email protected]>
> ---
> 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
LGTM, Thanks.
Signed-off-by: Mario Limonciello <[email protected]>
On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Jul 14, 2017 at 9:18 PM, Andy Shevchenko
<[email protected]> wrote:
> On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <[email protected]> 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
On Fri, Jul 14, 2017 at 10:37 PM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Jul 14, 2017 at 9:18 PM, Andy Shevchenko
> <[email protected]> wrote:
>> On Fri, Jul 14, 2017 at 3:07 PM, Arnd Bergmann <[email protected]> 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).
--
With Best Regards,
Andy Shevchenko
On Fri, Jul 14, 2017 at 2:52 PM, Andy Shevchenko
<[email protected]> wrote:
> 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.
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
The 07/14/2017 09:04, David Miller wrote:
> From: Arnd Bergmann <[email protected]>
> 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 <[email protected]>
>
> Applied.
I had raised a bug for this earlier and attached a patch as well.
http://cabugzilla1.caveonetworks.com/octeon_bugzilla/show_bug.cgi?id=26421
--
Regards
Satanand
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
On Mon, Jul 17, 2017 at 11:17 AM, David Laight <[email protected]> wrote:
> 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.
Ok, fair enough, I'll send a new version right away.
Arnd
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 <[email protected]>
> ---
> 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;
>
>
On Mon, Jul 17, 2017 at 2:53 PM, Hans Verkuil <[email protected]> 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().
Arnd
On 17/07/17 14:57, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 2:53 PM, Hans Verkuil <[email protected]> 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
On Fri, Jul 14, 2017 at 2:28 PM, Takashi Iwai <[email protected]> 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,
> 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
> 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 <[email protected]>
> ---
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