Besides being simpler, using strlcpy instead of strncpy+truncation
fixes part of the following class of new gcc warnings:
drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
destination size [-Werror=stringop-truncation]
strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Note that this is not a proper fix for this warning (and not all of the
occurences give the warning either - the strings are not always static).
The warning was intended to have developers check the return code of
strncpy and act in case of truncation (print a warning, abort the
function or something similar if the original string was not nul
terminated); the change to strlcpy only works because gcc does not
handle the function the same way.
Suggested-by: Ville Syrjälä <[email protected]>
Signed-off-by: Dominique Martinet <[email protected]>
---
Running this fixes 30 occurences of the problem in 17 different
components of the kernel, and while the produced patches are fairly
straight-forward I'm not sure who I should expect to pick this up as
it is sent as a series.
I expect each maintainer will pick their share of the patchs if they
agree with it and the rest will just be dropped?
.../coccinelle/misc/strncpy_truncation.cocci | 41 +++++++++++++++++++
1 file changed, 41 insertions(+)
create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
new file mode 100644
index 000000000000..28b5c2a290ac
--- /dev/null
+++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
@@ -0,0 +1,41 @@
+/// Use strlcpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
+///
+// Confidence: High
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual context
+virtual report
+virtual org
+
+@r@
+expression dest, src, sz;
+position p;
+@@
+
+strncpy@p(dest, src, sz);
+dest[sz - 1] = '\0';
+
+@script:python depends on org@
+p << r.p;
+@@
+
+cocci.print_main("strncpy followed by truncation can be strlcpy",p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "SUGGESTION: strncpy followed by truncation can be strlcpy"
+coccilib.report.print_report(p[0],msg)
+
+@ok depends on patch@
+expression r.dest, r.src, r.sz;
+position r.p;
+@@
+
+-strncpy@p(
++strlcpy(
+ dest, src, sz);
+-dest[sz - 1] = '\0';
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/block/aoe/aoenet.c | 3 +--
drivers/gpio/gpiolib.c | 12 ++++--------
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
index 63773a90581d..1b2871056f8f 100644
--- a/drivers/block/aoe/aoenet.c
+++ b/drivers/block/aoe/aoenet.c
@@ -39,8 +39,7 @@ static struct ktstate kts;
#ifndef MODULE
static int __init aoe_iflist_setup(char *str)
{
- strncpy(aoe_iflist, str, IFLISTSZ);
- aoe_iflist[IFLISTSZ - 1] = '\0';
+ strlcpy(aoe_iflist, str, IFLISTSZ);
return 1;
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..be08277699ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1015,12 +1015,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
memset(&chipinfo, 0, sizeof(chipinfo));
- strncpy(chipinfo.name, dev_name(&gdev->dev),
+ strlcpy(chipinfo.name, dev_name(&gdev->dev),
sizeof(chipinfo.name));
- chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
- strncpy(chipinfo.label, gdev->label,
+ strlcpy(chipinfo.label, gdev->label,
sizeof(chipinfo.label));
- chipinfo.label[sizeof(chipinfo.label)-1] = '\0';
chipinfo.lines = gdev->ngpio;
if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
return -EFAULT;
@@ -1036,16 +1034,14 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
desc = &gdev->descs[lineinfo.line_offset];
if (desc->name) {
- strncpy(lineinfo.name, desc->name,
+ strlcpy(lineinfo.name, desc->name,
sizeof(lineinfo.name));
- lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
} else {
lineinfo.name[0] = '\0';
}
if (desc->label) {
- strncpy(lineinfo.consumer, desc->label,
+ strlcpy(lineinfo.consumer, desc->label,
sizeof(lineinfo.consumer));
- lineinfo.consumer[sizeof(lineinfo.consumer)-1] = '\0';
} else {
lineinfo.consumer[0] = '\0';
}
--
2.17.1
Using strlcpy fixes this new gcc warning:
drivers/gpu/drm/drm_property.c: In function ‘drm_property_create’:
drivers/gpu/drm/drm_property.c:125:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
strncpy(property->name, name, DRM_PROP_NAME_LEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/gpu/drm/drm_property.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index cdb10f885a4f..8f0ccfa91f8b 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -122,8 +122,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
property->num_values = num_values;
INIT_LIST_HEAD(&property->enum_list);
- strncpy(property->name, name, DRM_PROP_NAME_LEN);
- property->name[DRM_PROP_NAME_LEN-1] = '\0';
+ strlcpy(property->name, name, DRM_PROP_NAME_LEN);
list_add_tail(&property->head, &dev->mode_config.property_list);
@@ -416,8 +415,7 @@ int drm_property_add_enum(struct drm_property *property,
if (!prop_enum)
return -ENOMEM;
- strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
- prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
+ strlcpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
prop_enum->value = value;
property->values[index] = value;
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
index 058ff46b5f16..579eb17db9f4 100644
--- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
+++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
@@ -40,8 +40,7 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
int i;
/* Convert device name to lowercase */
- strncpy(cname, device->chip->name, sizeof(cname));
- cname[sizeof(cname) - 1] = '\0';
+ strlcpy(cname, device->chip->name, sizeof(cname));
i = strlen(cname);
while (i) {
--i;
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/iio/common/st_sensors/st_sensors_core.c | 3 +--
drivers/iio/pressure/st_pressure_i2c.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 57db19182e95..26fbd1bd9413 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -380,8 +380,7 @@ void st_sensors_of_name_probe(struct device *dev,
return;
/* The name from the OF match takes precedence if present */
- strncpy(name, of_id->data, len);
- name[len - 1] = '\0';
+ strlcpy(name, of_id->data, len);
}
EXPORT_SYMBOL(st_sensors_of_name_probe);
#else
diff --git a/drivers/iio/pressure/st_pressure_i2c.c b/drivers/iio/pressure/st_pressure_i2c.c
index fbb59059e942..2026a1012012 100644
--- a/drivers/iio/pressure/st_pressure_i2c.c
+++ b/drivers/iio/pressure/st_pressure_i2c.c
@@ -94,9 +94,8 @@ static int st_press_i2c_probe(struct i2c_client *client,
if ((ret < 0) || (ret >= ST_PRESS_MAX))
return -ENODEV;
- strncpy(client->name, st_press_id_table[ret].name,
+ strlcpy(client->name, st_press_id_table[ret].name,
sizeof(client->name));
- client->name[sizeof(client->name) - 1] = '\0';
} else if (!id)
return -ENODEV;
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/message/fusion/mptctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630dd545..8d22d6134a89 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -2514,8 +2514,8 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int data_size)
if (mpt_config(ioc, &cfg) == 0) {
ManufacturingPage0_t *pdata = (ManufacturingPage0_t *) pbuf;
if (strlen(pdata->BoardTracerNumber) > 1) {
- strncpy(karg.serial_number, pdata->BoardTracerNumber, 24);
- karg.serial_number[24-1]='\0';
+ strlcpy(karg.serial_number,
+ pdata->BoardTracerNumber, 24);
}
}
pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf, buf_dma);
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 7 ++-----
drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 7 ++-----
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index 2e14a3ae1d8b..35b4d72d1997 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -638,12 +638,9 @@ static void hns_nic_get_drvinfo(struct net_device *net_dev,
{
struct hns_nic_priv *priv = netdev_priv(net_dev);
- strncpy(drvinfo->version, HNAE_DRIVER_VERSION,
+ strlcpy(drvinfo->version, HNAE_DRIVER_VERSION,
sizeof(drvinfo->version));
- drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
-
- strncpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo->driver));
- drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
+ strlcpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo->driver));
strncpy(drvinfo->bus_info, priv->dev->bus->name,
sizeof(drvinfo->bus_info));
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 40c0425b4023..630c8d186707 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -493,13 +493,10 @@ static void hns3_get_drvinfo(struct net_device *netdev,
struct hns3_nic_priv *priv = netdev_priv(netdev);
struct hnae3_handle *h = priv->ae_handle;
- strncpy(drvinfo->version, hns3_driver_version,
+ strlcpy(drvinfo->version, hns3_driver_version,
sizeof(drvinfo->version));
- drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
-
- strncpy(drvinfo->driver, h->pdev->driver->name,
+ strlcpy(drvinfo->driver, h->pdev->driver->name,
sizeof(drvinfo->driver));
- drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
strncpy(drvinfo->bus_info, pci_name(h->pdev),
sizeof(drvinfo->bus_info));
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index b2d2ec8c11e2..f7178cdb6bd8 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -553,8 +553,7 @@ myri10ge_validate_firmware(struct myri10ge_priv *mgp,
}
/* save firmware version for ethtool */
- strncpy(mgp->fw_version, hdr->version, sizeof(mgp->fw_version));
- mgp->fw_version[sizeof(mgp->fw_version) - 1] = '\0';
+ strlcpy(mgp->fw_version, hdr->version, sizeof(mgp->fw_version));
sscanf(mgp->fw_version, "%d.%d.%d", &mgp->fw_ver_major,
&mgp->fw_ver_minor, &mgp->fw_ver_tiny);
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/net/ethernet/qlogic/qed/qed_debug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
index a14e48489029..4fe0a72230e8 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
@@ -6212,8 +6212,7 @@ static void qed_read_str_from_buf(void *buf, u32 *offset, u32 size, char *dest)
{
const char *source_str = &((const char *)buf)[*offset];
- strncpy(dest, source_str, size);
- dest[size - 1] = '\0';
+ strlcpy(dest, source_str, size);
*offset += size;
}
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
index b7df576bb84d..58ccd72d672c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
@@ -584,8 +584,7 @@ struct dma_pub *dma_attach(char *name, struct brcms_c_info *wlc,
rxextheadroom, nrxpost, rxoffset, txregbase, rxregbase);
/* make a private copy of our callers name */
- strncpy(di->name, name, MAXNAMEL);
- di->name[MAXNAMEL - 1] = '\0';
+ strlcpy(di->name, name, MAXNAMEL);
di->dmadev = core->dma_dev;
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/net/wireless/ti/wl1251/acx.c | 9 +--------
drivers/net/wireless/ti/wl18xx/main.c | 5 +----
drivers/net/wireless/ti/wlcore/boot.c | 5 +----
3 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
index f78fc3880423..c4f1a63300bb 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251 *wl, char *buf, size_t len)
}
/* be careful with the buffer sizes */
- strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
-
- /*
- * if the firmware version string is exactly
- * sizeof(rev->fw_version) long or fw_len is less than
- * sizeof(rev->fw_version) it won't be null terminated
- */
- buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
+ strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
out:
kfree(rev);
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index ca0f936fc119..8595e9bf1cfa 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1529,12 +1529,9 @@ static int wl18xx_handle_static_data(struct wl1271 *wl,
struct wl18xx_static_data_priv *static_data_priv =
(struct wl18xx_static_data_priv *) static_data->priv;
- strncpy(wl->chip.phy_fw_ver_str, static_data_priv->phy_version,
+ strlcpy(wl->chip.phy_fw_ver_str, static_data_priv->phy_version,
sizeof(wl->chip.phy_fw_ver_str));
- /* make sure the string is NULL-terminated */
- wl->chip.phy_fw_ver_str[sizeof(wl->chip.phy_fw_ver_str) - 1] = '\0';
-
wl1271_info("PHY firmware version: %s", static_data_priv->phy_version);
return 0;
diff --git a/drivers/net/wireless/ti/wlcore/boot.c b/drivers/net/wireless/ti/wlcore/boot.c
index f00509ea8aca..6b33951d5b34 100644
--- a/drivers/net/wireless/ti/wlcore/boot.c
+++ b/drivers/net/wireless/ti/wlcore/boot.c
@@ -55,12 +55,9 @@ static int wlcore_boot_parse_fw_ver(struct wl1271 *wl,
{
int ret;
- strncpy(wl->chip.fw_ver_str, static_data->fw_version,
+ strlcpy(wl->chip.fw_ver_str, static_data->fw_version,
sizeof(wl->chip.fw_ver_str));
- /* make sure the string is NULL-terminated */
- wl->chip.fw_ver_str[sizeof(wl->chip.fw_ver_str) - 1] = '\0';
-
ret = sscanf(wl->chip.fw_ver_str + 4, "%u.%u.%u.%u.%u",
&wl->chip.fw_ver[0], &wl->chip.fw_ver[1],
&wl->chip.fw_ver[2], &wl->chip.fw_ver[3],
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/power/supply/test_power.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 57246cdbd042..64adf630f64f 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -297,8 +297,7 @@ static int map_get_value(struct battery_property_map *map, const char *key,
char buf[MAX_KEYLENGTH];
int cr;
- strncpy(buf, key, MAX_KEYLENGTH);
- buf[MAX_KEYLENGTH-1] = '\0';
+ strlcpy(buf, key, MAX_KEYLENGTH);
cr = strnlen(buf, MAX_KEYLENGTH) - 1;
if (cr < 0)
--
2.17.1
Using strlcpy fixes this new gcc warning:
kernel/trace/blktrace.c: In function ‘do_blk_trace_setup’:
kernel/trace/blktrace.c:497:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
kernel/trace/blktrace.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 987d9a9ae283..2478d9838eab 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -494,8 +494,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
if (!buts->buf_size || !buts->buf_nr)
return -EINVAL;
- strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
- buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
+ strlcpy(buts->name, name, BLKTRACE_BDEV_SIZE);
/*
* some device names have larger paths - convert the slashes
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
drivers/scsi/ibmvscsi/ibmvscsi.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 17df76f0be3c..79eb8af03a19 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1274,14 +1274,12 @@ static void send_mad_capabilities(struct ibmvscsi_host_data *hostdata)
if (hostdata->client_migrated)
hostdata->caps.flags |= cpu_to_be32(CLIENT_MIGRATED);
- strncpy(hostdata->caps.name, dev_name(&hostdata->host->shost_gendev),
+ strlcpy(hostdata->caps.name, dev_name(&hostdata->host->shost_gendev),
sizeof(hostdata->caps.name));
- hostdata->caps.name[sizeof(hostdata->caps.name) - 1] = '\0';
location = of_get_property(of_node, "ibm,loc-code", NULL);
location = location ? location : dev_name(hostdata->dev);
- strncpy(hostdata->caps.loc, location, sizeof(hostdata->caps.loc));
- hostdata->caps.loc[sizeof(hostdata->caps.loc) - 1] = '\0';
+ strlcpy(hostdata->caps.loc, location, sizeof(hostdata->caps.loc));
req->common.type = cpu_to_be32(VIOSRP_CAPABILITIES_TYPE);
req->buffer = cpu_to_be64(hostdata->caps_addr);
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
kernel/debug/kdb/kdb_support.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 990b3cc526c8..1f6a4b6bde0b 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -119,8 +119,7 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
* What was Rusty smoking when he wrote that code?
*/
if (symtab->sym_name != knt1) {
- strncpy(knt1, symtab->sym_name, knt1_size);
- knt1[knt1_size-1] = '\0';
+ strlcpy(knt1, symtab->sym_name, knt1_size);
}
for (i = 0; i < ARRAY_SIZE(kdb_name_table); ++i) {
if (kdb_name_table[i] &&
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
tools/accounting/getdelays.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/accounting/getdelays.c b/tools/accounting/getdelays.c
index 9f420d98b5fb..66817a7a4fce 100644
--- a/tools/accounting/getdelays.c
+++ b/tools/accounting/getdelays.c
@@ -314,8 +314,7 @@ int main(int argc, char *argv[])
err(1, "Invalid rcv buf size\n");
break;
case 'm':
- strncpy(cpumask, optarg, sizeof(cpumask));
- cpumask[sizeof(cpumask) - 1] = '\0';
+ strlcpy(cpumask, optarg, sizeof(cpumask));
maskset = 1;
printf("cpumask %s maskset %d\n", cpumask, maskset);
break;
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
tools/perf/util/bpf-loader.h | 3 +--
tools/perf/util/util.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
index 5d3aefd6fae7..8d08a1fc97a0 100644
--- a/tools/perf/util/bpf-loader.h
+++ b/tools/perf/util/bpf-loader.h
@@ -143,10 +143,9 @@ __bpf_strerror(char *buf, size_t size)
{
if (!size)
return 0;
- strncpy(buf,
+ strlcpy(buf,
"ERROR: eBPF object loading is disabled during compiling.\n",
size);
- buf[size - 1] = '\0';
return 0;
}
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index eac5b858a371..8b9e3aa7aad3 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -459,8 +459,7 @@ fetch_kernel_version(unsigned int *puint, char *str,
return -1;
if (str && str_size) {
- strncpy(str, utsname.release, str_size);
- str[str_size - 1] = '\0';
+ strlcpy(str, utsname.release, str_size);
}
if (!puint || int_ver_ready)
--
2.17.1
Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch
tools/power/cpupower/bench/parse.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
index 9ba8a44ad2a7..1566b89989b2 100644
--- a/tools/power/cpupower/bench/parse.c
+++ b/tools/power/cpupower/bench/parse.c
@@ -221,9 +221,8 @@ int prepare_config(const char *path, struct config *config)
sscanf(val, "%u", &config->cpu);
else if (strcmp("governor", opt) == 0) {
- strncpy(config->governor, val,
+ strlcpy(config->governor, val,
sizeof(config->governor));
- config->governor[sizeof(config->governor) - 1] = '\0';
}
else if (strcmp("priority", opt) == 0) {
--
2.17.1
On 7/13/2018 3:25 AM, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Acked-by: Arend van Spriel <[email protected]>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
I would prefer to have the motivation in the commit message of this patch.
Regards,
Arend
On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
I don't know about other maintainers, but I know I wouldn't take such a
horrid changelog description as this :)
good luck!
greg k-h
On Fri, Jul 13, 2018 at 03:14:43AM +0200, Dominique Martinet wrote:
> Besides being simpler, using strlcpy instead of strncpy+truncation
> fixes part of the following class of new gcc warnings:
>
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
> destination size [-Werror=stringop-truncation]
> strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Note that this is not a proper fix for this warning (and not all of the
> occurences give the warning either - the strings are not always static).
> The warning was intended to have developers check the return code of
> strncpy and act in case of truncation (print a warning, abort the
> function or something similar if the original string was not nul
> terminated); the change to strlcpy only works because gcc does not
> handle the function the same way.
>
> Suggested-by: Ville Syrjälä <[email protected]>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Running this fixes 30 occurences of the problem in 17 different
> components of the kernel, and while the produced patches are fairly
> straight-forward I'm not sure who I should expect to pick this up as
> it is sent as a series.
> I expect each maintainer will pick their share of the patchs if they
> agree with it and the rest will just be dropped?
Masahiro Yamada <[email protected]> takes coccinelle patches,
so please cc him or your patch would be lost.
> .../coccinelle/misc/strncpy_truncation.cocci | 41 +++++++++++++++++++
> 1 file changed, 41 insertions(+)
> create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
>
> diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
> new file mode 100644
> index 000000000000..28b5c2a290ac
> --- /dev/null
> +++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
> @@ -0,0 +1,41 @@
> +/// Use strlcpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
> +///
> +// Confidence: High
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual context
You might consider adding context rule or remove this line perhaps ?
> +virtual report
> +virtual org
> +
> +@r@
> +expression dest, src, sz;
> +position p;
> +@@
> +
> +strncpy@p(dest, src, sz);
> +dest[sz - 1] = '\0';
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +cocci.print_main("strncpy followed by truncation can be strlcpy",p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg = "SUGGESTION: strncpy followed by truncation can be strlcpy"
> +coccilib.report.print_report(p[0],msg)
> +
> +@ok depends on patch@
> +expression r.dest, r.src, r.sz;
> +position r.p;
> +@@
> +
> +-strncpy@p(
> ++strlcpy(
> + dest, src, sz);
> +-dest[sz - 1] = '\0';
The above rule produces an output that I think is not correct:
--------------------------------------------------------------
diff =
diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c
--- a//ti/wl1251/acx.c
+++ b//ti/wl1251/acx.c
@@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251
}
/* be careful with the buffer sizes */
- strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
-
- /*
- * if the firmware version string is exactly
- * sizeof(rev->fw_version) long or fw_len is less than
- * sizeof(rev->fw_version) it won't be null terminated
- */
- buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
+ strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
-----------------------------------------------------------------
I think the comment is useful and should not be removed. Also, consider
changing Confidence level appropriately.
Thanks.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
On 7/13/2018 9:38 AM, Greg Kroah-Hartman wrote:
> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>>
>> Signed-off-by: Dominique Martinet <[email protected]>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)
>
> good luck!
especially as that script is not in the kernel tree yet. The patch
adding that script contains a good motivation, but I would want to see
that in commit message of every patch or at least the gist of it.
Regards,
Arend
Himanshu Jha wrote on Fri, Jul 13, 2018:
> > I expect each maintainer will pick their share of the patchs if they
> > agree with it and the rest will just be dropped?
>
> Masahiro Yamada <[email protected]> takes coccinelle patches,
> so please cc him or your patch would be lost.
Thanks, will do.
> > +virtual patch
> > +virtual context
>
> You might consider adding context rule or remove this line perhaps ?
Victim of copypasta, I'll remove this.
> > +-strncpy@p(
> > ++strlcpy(
> > + dest, src, sz);
> > +-dest[sz - 1] = '\0';
>
> The above rule produces an output that I think is not correct:
> --------------------------------------------------------------
> diff =
> diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c
> --- a//ti/wl1251/acx.c
> +++ b//ti/wl1251/acx.c
> @@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251
> }
>
> /* be careful with the buffer sizes */
> - strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
> -
> - /*
> - * if the firmware version string is exactly
> - * sizeof(rev->fw_version) long or fw_len is less than
> - * sizeof(rev->fw_version) it won't be null terminated
> - */
> - buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
> + strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
>
> -----------------------------------------------------------------
>
> I think the comment is useful and should not be removed.
I agree this comment is useful now that I'm taking a closer look, I
glanced at this too fast.
I'm not sure how to make coccinelle not remove comments between lines
though?
> Also, consider changing Confidence level appropriately.
I am (was?) pretty confident on the change itself, the only exceptions
would be if someone relied on strncpy to fill the end of the buffer with
zero to not leak data somewhere but that is not easy to judge by itself
(although I hope rare enough)
I'm honestly not sure what would be appropriate in this case.
--
Dominique Martinet
Arend van Spriel wrote on Fri, Jul 13, 2018:
> The patch adding that script contains a good motivation, but I would want to
> see that in commit message of every patch or at least the gist of
> it.
In retrospect, I definitely agree - I was happy I got coccinelle to work
and a bit too tired to make rationale decisions when I sent the serie as
it's not a kind of thing I'm used to.
For the patch you ack'd, in particular, there would be no gcc warning in
the first place because the source string's size is not known at compile
time and for some reason gcc does not mind silent truncation in that
case, so the usefulnes of the patch is fairly limited in the first
place (it's possibly simpler/good to aim for consistency but that's
about it). I however didn't take the time to make that analysis for all
the patches.
> especially as that script is not in the kernel tree yet.
I did think about that, but wasn't sure what was appropriate in this
case.
I now think it would have been better to save everyone a dozen of mails
and wait for the coccinelle patch to land first; but it's a bit late for
regret :)
I'll only catter after the coccinelle script until it lands, so if
anyone is inclined to take one of the rest as they are, great, but
otherwise feel free to ignore them for now.
(In particular, this very patch should not remove the first comment
here, as pointed out by Himanshu Jha in reply to the first patch)
Thanks for taking the time to give feedback,
--
Dominique Martinet
On Fri, Jul 13, 2018 at 10:00:23AM +0200, Dominique Martinet wrote:
> Himanshu Jha wrote on Fri, Jul 13, 2018:
> > > I expect each maintainer will pick their share of the patchs if they
> > > agree with it and the rest will just be dropped?
> >
> > Masahiro Yamada <[email protected]> takes coccinelle patches,
> > so please cc him or your patch would be lost.
>
> Thanks, will do.
>
> > > +virtual patch
> > > +virtual context
> >
> > You might consider adding context rule or remove this line perhaps ?
>
> Victim of copypasta, I'll remove this.
>
> > > +-strncpy@p(
> > > ++strlcpy(
> > > + dest, src, sz);
> > > +-dest[sz - 1] = '\0';
> >
> > The above rule produces an output that I think is not correct:
> > --------------------------------------------------------------
> > diff =
> > diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c
> > --- a//ti/wl1251/acx.c
> > +++ b//ti/wl1251/acx.c
> > @@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251
> > }
> >
> > /* be careful with the buffer sizes */
> > - strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
> > -
> > - /*
> > - * if the firmware version string is exactly
> > - * sizeof(rev->fw_version) long or fw_len is less than
> > - * sizeof(rev->fw_version) it won't be null terminated
> > - */
> > - buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
> > + strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
> >
> > -----------------------------------------------------------------
> >
> > I think the comment is useful and should not be removed.
>
> I agree this comment is useful now that I'm taking a closer look, I
> glanced at this too fast.
> I'm not sure how to make coccinelle not remove comments between lines
> though?
Well, there is no such facility in Coccinelle to ignore comments.
You can hack with other facilities provided in SmPL though ;)
Try this:
$ spatch -D patch --sp-file strlcopy.cocci --very-quiet drivers/net/wireless/ti/wl1251/acx.c
---------------------------------------------------------------------
virtual patch
@depends on patch@
expression dest, src, sz;
identifier f;
@@
(
- strncpy(
+ strlcpy(
dest, src, sizeof(sz));
- dest[sizeof(sz) - 1] = '\0';
|
- strncpy(
+ strlcpy(
dest, src, f);
- dest[f - 1] = '\0';
)
---------------------------------------------------------------------
This eliminates that case because expression is generic metavariable and
it somehow matched whole "min(len, sizeof(...)..", so it better to
divide the rules as done above to be more specific about the matching
pattern.
I thought to replace "identifier f" with "constant F" but that misses
few cases.
Also, it is advised to put a space affer '+/-'
Thanks.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
On Fri, 13 Jul 2018, Himanshu Jha wrote:
> On Fri, Jul 13, 2018 at 10:00:23AM +0200, Dominique Martinet wrote:
> > Himanshu Jha wrote on Fri, Jul 13, 2018:
> > > > I expect each maintainer will pick their share of the patchs if they
> > > > agree with it and the rest will just be dropped?
> > >
> > > Masahiro Yamada <[email protected]> takes coccinelle patches,
> > > so please cc him or your patch would be lost.
> >
> > Thanks, will do.
> >
> > > > +virtual patch
> > > > +virtual context
> > >
> > > You might consider adding context rule or remove this line perhaps ?
> >
> > Victim of copypasta, I'll remove this.
> >
> > > > +-strncpy@p(
> > > > ++strlcpy(
> > > > + dest, src, sz);
> > > > +-dest[sz - 1] = '\0';
> > >
> > > The above rule produces an output that I think is not correct:
> > > --------------------------------------------------------------
> > > diff =
> > > diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c
> > > --- a//ti/wl1251/acx.c
> > > +++ b//ti/wl1251/acx.c
> > > @@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251
> > > }
> > >
> > > /* be careful with the buffer sizes */
> > > - strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
> > > -
> > > - /*
> > > - * if the firmware version string is exactly
> > > - * sizeof(rev->fw_version) long or fw_len is less than
> > > - * sizeof(rev->fw_version) it won't be null terminated
> > > - */
> > > - buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
> > > + strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
> > >
> > > -----------------------------------------------------------------
> > >
> > > I think the comment is useful and should not be removed.
> >
> > I agree this comment is useful now that I'm taking a closer look, I
> > glanced at this too fast.
> > I'm not sure how to make coccinelle not remove comments between lines
> > though?
>
> Well, there is no such facility in Coccinelle to ignore comments.
> You can hack with other facilities provided in SmPL though ;)
>
> Try this:
>
> $ spatch -D patch --sp-file strlcopy.cocci --very-quiet drivers/net/wireless/ti/wl1251/acx.c
>
> ---------------------------------------------------------------------
> virtual patch
>
> @depends on patch@
> expression dest, src, sz;
> identifier f;
> @@
>
> (
> - strncpy(
> + strlcpy(
> dest, src, sizeof(sz));
> - dest[sizeof(sz) - 1] = '\0';
> |
> - strncpy(
> + strlcpy(
> dest, src, f);
> - dest[f - 1] = '\0';
> )
> ---------------------------------------------------------------------
>
> This eliminates that case because expression is generic metavariable and
> it somehow matched whole "min(len, sizeof(...)..", so it better to
> divide the rules as done above to be more specific about the matching
> pattern.
>
> I thought to replace "identifier f" with "constant F" but that misses
> few cases.
>
> Also, it is advised to put a space affer '+/-'
Thanks Himanshu for the suggestions.
However, I'm not sure to follow the discussion. The original problem was
that Coccinelle was removing a comment that should be preserved. I think
that this occurs because the line just below the comment is completely
removed. Coccinelle considers that the comment belongs with that line and
if the line is removed the comment won't make much sense.
In Himanshu's solution, the code is just not transformed at all, so as a
side effect the comment stays too. Is that what is wanted in this case?
julia
> Thanks Himanshu for the suggestions.
>
> However, I'm not sure to follow the discussion. The original problem was
> that Coccinelle was removing a comment that should be preserved. I think
> that this occurs because the line just below the comment is completely
> removed. Coccinelle considers that the comment belongs with that line and
> if the line is removed the comment won't make much sense.
>
> In Himanshu's solution, the code is just not transformed at all, so as a
> side effect the comment stays too. Is that what is wanted in this case?
Yes, there is no transformation with my solution which I advised to
prevent comment removal(which i thought was useful).
My rule is more narrower approach than the regular ones which used
"expression" metavariable.
Rest upto Dominique to choose whatever suits better :)
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
On Fri, Jul 13, 2018 at 03:25:58AM +0200, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
It would have been good for you to mention in the changelog how
you reviewed your change to verify that the extra zero padding
from strncpy() isn't required.
However... I have taken a look and can't see any problem so:
Reviewed-by: Daniel Thompson <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
> kernel/debug/kdb/kdb_support.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index 990b3cc526c8..1f6a4b6bde0b 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -119,8 +119,7 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
> * What was Rusty smoking when he wrote that code?
> */
> if (symtab->sym_name != knt1) {
> - strncpy(knt1, symtab->sym_name, knt1_size);
> - knt1[knt1_size-1] = '\0';
> + strlcpy(knt1, symtab->sym_name, knt1_size);
> }
> for (i = 0; i < ARRAY_SIZE(kdb_name_table); ++i) {
> if (kdb_name_table[i] &&
> --
> 2.17.1
>
For the file drivers/net/wireless/ti/wl1251/acx.c, the following keeps the
comment before the buf update and moves the strlcpy call below it. It
does however drop the comment just before the original call to strncpy.
julia
@r@
expression dest, src;
expression f;
@@
- strncpy(dest,src,f);
dest[f - 1] = '\0'
+ + strlcpy(dest,src,f)
;
@@
expression r.dest, r.src;
expression r.f;
@@
- dest[f - 1] = '\0' + strlcpy(dest,src,f);
+ strlcpy(dest,src,f);
On 7/12/18 7:25 PM, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
This is a weird combination in terms of a single patch, why is
it patching aoe and gpiolib in one patch?
--
Jens Axboe
Daniel Thompson wrote on Fri, Jul 13, 2018:
> On Fri, Jul 13, 2018 at 03:25:58AM +0200, Dominique Martinet wrote:
> > Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> >
> > Signed-off-by: Dominique Martinet <[email protected]>
>
> It would have been good for you to mention in the changelog how
> you reviewed your change to verify that the extra zero padding
> from strncpy() isn't required.
I'm sorry, I went a bit too fast with the send-email button on this
path series; I agree it is lacking.
> However... I have taken a look and can't see any problem so:
>
> Reviewed-by: Daniel Thompson <[email protected]>
Thank you for the review, I will update the commit message with more
details and change strlcpy to strscpy in a v2
David Laight wrote on Fri, Jul 13, 2018:
> > - strncpy(knt1, symtab->sym_name, knt1_size);
> > - knt1[knt1_size-1] = '\0';
> > + strlcpy(knt1, symtab->sym_name, knt1_size);
>
> You'd be better using strscpy() not strlcpy().
> The return value of strlcpy() is the length of the source string.
> If the source string isn't '\0' terminated horrid things happen.
I was suggested to use strlcpy for drm and didn't realize this at this
point, but you are correct; strlcpy is far from being as protective.
I'll update the coccinelle patch to use strscpy and resend this.
> David
>
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
> P Please consider the environment and don't print this e-mail unless
> you really need to
(By the way, your e-mail client sends both html and text, but the text
version only included the footer so my mail client, which prefers text,
was displaying an empty mail message. If possible I would appreciate if
you could disable either version so as to get a coherent message)
--
Dominique Martinet
Jens Axboe wrote on Fri, Jul 13, 2018:
> This is a weird combination in terms of a single patch, why is
> it patching aoe and gpiolib in one patch?
I must have failed some git add command, I didn't realize they were
together until you pointed it out just now.
This has been discussed more lengthly on the netdev side but the whole
patch serie has been (quite) a bit too hasty; on one side I've been
given some interesting feedback that I would not have gotten just
sending the coccinelle patch first but I really should have waited for
that to land first and most importantly spend more time on each
individual patch, this was rude of me.
I will make sure they are split in a v2, and send them individually
(well, per big component) as self-explaining patches rather than as a
block, once the coccinelle patch has been accepted.
Thank you for the review,
--
Dominique Martinet
Himanshu Jha wrote on Fri, Jul 13, 2018:
> Try this:
>
> $ spatch -D patch --sp-file strlcopy.cocci --very-quiet drivers/net/wireless/ti/wl1251/acx.c
>
> ---------------------------------------------------------------------
> virtual patch
>
> @depends on patch@
> expression dest, src, sz;
> identifier f;
> @@
>
> (
> - strncpy(
> + strlcpy(
> dest, src, sizeof(sz));
> - dest[sizeof(sz) - 1] = '\0';
> |
> - strncpy(
> + strlcpy(
> dest, src, f);
> - dest[f - 1] = '\0';
> )
> ---------------------------------------------------------------------
>
> This eliminates that case because expression is generic metavariable and
> it somehow matched whole "min(len, sizeof(...)..", so it better to
> divide the rules as done above to be more specific about the matching
> pattern.
>
> I thought to replace "identifier f" with "constant F" but that misses
> few cases.
My first test started with 'constant sz' as well and I found expression
to be better.
If I understand this correctly, it just makes sure not to match the
'min(...)' expression so the problem doesn't arise, but it's not really
a solution as it is really a chance that this comment comes here for
this more complex expression.
I'd rather just advise to pay attention to comments and drop the
confidence level
> Also, it is advised to put a space affer '+/-'
Ok, thanks
Julia Lawall wrote on Fri, Jul 13, 2018:
> For the file drivers/net/wireless/ti/wl1251/acx.c, the following keeps the
> comment before the buf update and moves the strlcpy call below it. It
> does however drop the comment just before the original call to strncpy.
Nice, doing it in two passes does the trick; it even keeps the comment
before strncpy if there was no comment in between.
I'm sorry to write that after being provided with such a nice
work-around though, now that I'm a bit less tired I've had a look at the
comments again and it does not make sense to keep the second comment as
is -- the whole point of using strlcpy (or now strscpy as per feedback
elsewhere) is that the string will be null terminated properly after the
call, so I'll stick to the original version.
I also see the position does not seem to be useful for the patch phase,
spatch automatically only applies only to what was matched earlier in
report mode?
>>> +-strncpy@p
>>> ++strlcpy
>>> + (dest, src, sz);
>>
>> This also came from the example I picked, but if this does not make a
>> difference as it sounds like I will update to that.
>
> Probably not removing something just to add it back would be a good
> idea.
Actually playing with your example made me realize that removing and
re-adding argument does fix the indentation issue in the original code I
had noticed for mptctl, so it might actually unexpectedly be a good idea
to go in the opposite direction and make coccinelle remove/add arguments
in the general case (e.g. if strncpy and strlcpy hadn't been the same
length)
The jury is still out for this one :)
Thanks for all the feedbacks,
--
Dominique Martinet
On Jul 13, 2018, at 12:38 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>>
>> Signed-off-by: Dominique Martinet <[email protected]>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)
>
> good luck!
>
> greg k-h
I would be very concerned about the potential for information leak, because
of the different behavior of the two functions.
--
Mark Rustad, Networking Division, Intel Corporation
Besides being simpler, using strscpy instead of strncpy+truncation
fixes part of the following class of new gcc warnings:
drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
destination size [-Werror=stringop-truncation]
strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Note that this is note a proper fix for this warning. The warning was
intended to have developers check the return code of strncpy and
act in case of truncation (print a warning, abort the function or
something similar if the original string was not nul terminated);
the change to strscpy only works because gcc does not handle the
function the same way.
v2:
- Use strscpy instead of strlcpy, as strlcpy can read after the number
of requested bytes in the source string, and none of the replaced users
want to know the source string size length
- Add longer semantic patch information, warning in particular for
information leak
- Lowered Confidence level to medium because of the possibility of
information leak, that needs manual checking
- Fix spacing of the diff section and removed unused virtual context
Signed-off-by: Dominique Martinet <[email protected]>
---
Thanks for the many feedback I received; I hope I didn't miss any.
In particular, I have conciously not removed the intermediate msg
variable; as I made the message longer I actually added one more of
these for the org mode section.
I also have decided to let spatch remove the second comment, given the
confidence level has been lowered, the user should be able to manually
adjust the result if required.
I will resend the other patchs of the series a much smaller number at
a time after doing all the appropriate checks and giving them a better
comment, after this has been merged.
.../coccinelle/misc/strncpy_truncation.cocci | 50 +++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
new file mode 100644
index 000000000000..dd157fc8ec5f
--- /dev/null
+++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
@@ -0,0 +1,50 @@
+/// Use strscpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
+///
+//# This makes an effort to find occurences of strncpy followed by forced
+//# truncation, which can generate gcc stringop-truncation warnings when
+//# source and destination buffers are the same size.
+//# Using strscpy will always do that nul-termination for us and not read
+//# more than the maximum bytes requested in src, use that instead.
+//#
+//# The result needs checking that the destination buffer does not need
+//# its tail zeroed (either cleared beforehand or will never leave the
+//# kernel) so as not to leak information
+//
+// Confidence: Medium
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual report
+virtual org
+
+@r@
+expression dest, src, sz;
+position p;
+@@
+
+strncpy@p(dest, src, sz);
+dest[sz - 1] = '\0';
+
+@script:python depends on org@
+p << r.p;
+@@
+
+msg = "strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
+cocci.print_main(msg, p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
+coccilib.report.print_report(p[0], msg)
+
+@ok depends on patch@
+expression r.dest, r.src, r.sz;
+@@
+
+- strncpy
++ strscpy
+ (dest, src, sz);
+- dest[sz - 1] = '\0';
--
2.17.1
> +msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> +coccilib.report.print_report(p[0], msg)
This is the first SUGGESTION. I don't know if anyone out there is relying
on it always being WARNING or ERROR.
julia
Julia Lawall wrote on Sat, Jul 14, 2018:
> Not a big deal, but actually the v2 goes below the ---
I've seen both being done (if you look at the git log of the linux
kernel and search for 'v2' you will have some matches)
The list was a bit long in this case, but I think it's worth at least
mentioning that the previous version used strlcpy and why I changed in
the commit message.
> > +msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> > +coccilib.report.print_report(p[0], msg)
>
> This is the first SUGGESTION. I don't know if anyone out there is relying
> on it always being WARNING or ERROR.
Eh, I must have been really unlucky with the scripts I looked at, one
just happened to have SUGGESTION used like this (misc/warn.cocci), but
now you said that I can see it's the only one!
I'm not sure on what to do here, if you think there could be scripts
relying on that then I'll change this to WARNING, but the wording feels
a bit strong and "suggestion" leaves more room for interpretation.
> Copyright stuff in the other sub-thread
Replying here instead to limit the number of mails sent,
I think people would look at git blame/log if there is no name in the
file, but I can understand it is simpler if a name is present.
Just a nitpick on format, all copyright comments on cocci scripts end
with the license; since that will be added as an SPDX tag instead do you
mind if I do not list it again there?
Also just a head's up, I'll be AFK for the next ~48 hours; I'll post a
v3 of the patch with license/copyright added, possibly suggestion
changed, and whatever else comes up by then :)
Thanks,
--
Dominique Martinet
I would like to contribute another bit of patch review for your proposal.
https://patchwork.kernel.org/patch/10524633/
https://lore.kernel.org/lkml/[email protected]/
* I have noticed that an action word was missing in the commit subject.
* The commit description was started with the wording “Besides being simpler, …”.
I would prefer an other ordering of information there.
It seems that the mentioned compilation error gave you motivation for
the shown source code transformation.
I suggest to move the concrete update suggestion behind it.
> +virtual patch
> +virtual report
> +virtual org
How do you think about to reduce a bit of code repetition?
+virtual patch, report, org
> In particular, I have conciously not removed the intermediate msg
> variable; as I made the message longer I actually added one more of
> these for the org mode section.
I propose again to reconsider such an implementation detail once more
so that unnecessary Python code could be avoided.
The Linux coding style supports the usage of long message strings.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=2db39a2f491a48ec740e0214a7dd584eefc2137d#n81
Can string literals be nicer than extra variables (because of questionable
source code layout concerns)?
Regards,
Markus
On Sat, 14 Jul 2018, Dominique Martinet wrote:
> Julia Lawall wrote on Sat, Jul 14, 2018:
> > Not a big deal, but actually the v2 goes below the ---
>
> I've seen both being done (if you look at the git log of the linux
> kernel and search for 'v2' you will have some matches)
I guess. Normally I would conseider that since the v1 is not in the git
history, no one care about the delta between the v1 and v2. If there is
important information it should just be in the commit message.
> The list was a bit long in this case, but I think it's worth at least
> mentioning that the previous version used strlcpy and why I changed in
> the commit message.
I guess, but you could say that strlcpy was not used for a certain reason,
without making it historical information.
>
> > > +msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> > > +coccilib.report.print_report(p[0], msg)
> >
> > This is the first SUGGESTION. I don't know if anyone out there is relying
> > on it always being WARNING or ERROR.
>
> Eh, I must have been really unlucky with the scripts I looked at, one
> just happened to have SUGGESTION used like this (misc/warn.cocci), but
> now you said that I can see it's the only one!
>
> I'm not sure on what to do here, if you think there could be scripts
> relying on that then I'll change this to WARNING, but the wording feels
> a bit strong and "suggestion" leaves more room for interpretation.
I guess that if there is already one, then another won't hurt.
>
>
> > Copyright stuff in the other sub-thread
>
> Replying here instead to limit the number of mails sent,
> I think people would look at git blame/log if there is no name in the
> file, but I can understand it is simpler if a name is present.
One less command to type.
>
> Just a nitpick on format, all copyright comments on cocci scripts end
> with the license; since that will be added as an SPDX tag instead do you
> mind if I do not list it again there?
I know nothing about SPDX tags. If something is added, I don't know how
it is done.
julia
>
>
> Also just a head's up, I'll be AFK for the next ~48 hours; I'll post a
> v3 of the patch with license/copyright added, possibly suggestion
> changed, and whatever else comes up by then :)
>
> Thanks,
> --
> Dominique Martinet
>
On Fri, 13 Jul 2018 03:25:34 +0200
Dominique Martinet <[email protected]> wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
> drivers/iio/common/st_sensors/st_sensors_core.c | 3 +--
> drivers/iio/pressure/st_pressure_i2c.c | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 57db19182e95..26fbd1bd9413 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -380,8 +380,7 @@ void st_sensors_of_name_probe(struct device *dev,
> return;
>
> /* The name from the OF match takes precedence if present */
> - strncpy(name, of_id->data, len);
> - name[len - 1] = '\0';
> + strlcpy(name, of_id->data, len);
> }
> EXPORT_SYMBOL(st_sensors_of_name_probe);
> #else
> diff --git a/drivers/iio/pressure/st_pressure_i2c.c b/drivers/iio/pressure/st_pressure_i2c.c
> index fbb59059e942..2026a1012012 100644
> --- a/drivers/iio/pressure/st_pressure_i2c.c
> +++ b/drivers/iio/pressure/st_pressure_i2c.c
> @@ -94,9 +94,8 @@ static int st_press_i2c_probe(struct i2c_client *client,
> if ((ret < 0) || (ret >= ST_PRESS_MAX))
> return -ENODEV;
>
> - strncpy(client->name, st_press_id_table[ret].name,
> + strlcpy(client->name, st_press_id_table[ret].name,
> sizeof(client->name));
> - client->name[sizeof(client->name) - 1] = '\0';
> } else if (!id)
> return -ENODEV;
>
On Fri, Jul 13, 2018 at 05:18:51PM +0200, Dominique Martinet wrote:
> Daniel Thompson wrote on Fri, Jul 13, 2018:
> > On Fri, Jul 13, 2018 at 03:25:58AM +0200, Dominique Martinet wrote:
> > > Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> > >
> > > Signed-off-by: Dominique Martinet <[email protected]>
> >
> > It would have been good for you to mention in the changelog how
> > you reviewed your change to verify that the extra zero padding
> > from strncpy() isn't required.
>
> I'm sorry, I went a bit too fast with the send-email button on this
> path series; I agree it is lacking.
>
> > However... I have taken a look and can't see any problem so:
> >
> > Reviewed-by: Daniel Thompson <[email protected]>
>
> Thank you for the review, I will update the commit message with more
> details and change strlcpy to strscpy in a v2
You can keep the reviewed by after the change to strscpy().
Jonathan Cameron wrote on Sun, Jul 15, 2018:
> On Fri, 13 Jul 2018 03:25:34 +0200
> Dominique Martinet <[email protected]> wrote:
> > Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> >
> > Signed-off-by: Dominique Martinet <[email protected]>
>
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.
Thanks!
I have been pointed out that strlcpy, unlike strncpy, will read past the
size given in the input string and thus is Bad™ if the input string is
not nul terminated.
After taking the time to check I believe this should not happen as the
original name seems to come from a dentry's d_name after proper
preparation (a buffer is allocated precisely for this purpose), but it
will not hurt to wait for that version.
The second reason I was waiting is that I intended to check for each
patch if it is safe to not pad the end of the string with zeroes (to
avoid e.g. information leaks) and that seems OK as well here after a
quick check but I wouldn't trust my own eyes this late so I'll let you
be judge of that if you feel like taking v1 anyway.
Otherwise, I'll recheck properly and submit a v2 with strscpy and a
better commit message after the coccinelle script is taken for inclusion
and doing a better check but this might take a while longer.
Thanks,
--
Dominique Martinet
Using strscpy instead of strncpy+truncation is simpler and fixes part
of the following class of new gcc warnings:
drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
destination size [-Werror=stringop-truncation]
strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Note that this is not a proper fix for this warning. The warning was
intended to have developers check the return code of strncpy and
act in case of truncation (print a warning, abort the function or
something similar if the original string was not nul terminated);
the change to strscpy only works because gcc does not handle the
function the same way.
A previous version of this patch suggested to use strlcpy instead,
but strscpy is preferred because it does not read more than the given
length of the source string unlike strlcpy, which could read after the
end of the buffer in case of unterminated string.
strscpy does however not clear the end of the destination buffer, so
there is a risk of information leak if the full buffer is copied as is
out of the kernel - this needs manual checking.
Signed-off-by: Dominique Martinet <[email protected]>
---
v2:
- Use strscpy instead of strlcpy, as strlcpy can read after the number
of requested bytes in the source string, and none of the replaced users
want to know the source string size length
- Add longer semantic patch information, warning in particular for
information leak
- Lowered Confidence level to medium because of the possibility of
information leak, that needs manual checking
- Fix spacing of the diff section and removed unused virtual context
v3:
- Add license/copyright
- Rewording of commit message
I didn't see many other remarks, but kept SUGGESTION as discussed.
I didn't move all virtuals in a single line because none of the other
kernel patch do it, and still do not see any advantage of moving the
string to not use a variable so kept that as well.
This should hopefully be the last version :)
.../coccinelle/misc/strncpy_truncation.cocci | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
new file mode 100644
index 000000000000..7732cde23a85
--- /dev/null
+++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Use strscpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
+///
+//# This makes an effort to find occurences of strncpy followed by forced
+//# truncation, which can generate gcc stringop-truncation warnings when
+//# source and destination buffers are the same size.
+//# Using strscpy will always do that nul-termination for us and not read
+//# more than the maximum bytes requested in src, use that instead.
+//#
+//# The result needs checking that the destination buffer does not need
+//# its tail zeroed (either cleared beforehand or will never leave the
+//# kernel) so as not to leak information
+//
+// Confidence: Medium
+// Copyright: (C) 2018 Dominique Martinet
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual report
+virtual org
+
+@r@
+expression dest, src, sz;
+position p;
+@@
+
+strncpy@p(dest, src, sz);
+dest[sz - 1] = '\0';
+
+@script:python depends on org@
+p << r.p;
+@@
+
+msg = "strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
+cocci.print_main(msg, p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
+coccilib.report.print_report(p[0], msg)
+
+@ok depends on patch@
+expression r.dest, r.src, r.sz;
+@@
+
+- strncpy
++ strscpy
+ (dest, src, sz);
+- dest[sz - 1] = '\0';
--
2.17.1
On Fri, 20 Jul 2018, Dominique Martinet wrote:
> Using strscpy instead of strncpy+truncation is simpler and fixes part
> of the following class of new gcc warnings:
>
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
> destination size [-Werror=stringop-truncation]
> strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Note that this is not a proper fix for this warning. The warning was
> intended to have developers check the return code of strncpy and
> act in case of truncation (print a warning, abort the function or
> something similar if the original string was not nul terminated);
> the change to strscpy only works because gcc does not handle the
> function the same way.
>
> A previous version of this patch suggested to use strlcpy instead,
> but strscpy is preferred because it does not read more than the given
> length of the source string unlike strlcpy, which could read after the
> end of the buffer in case of unterminated string.
>
> strscpy does however not clear the end of the destination buffer, so
> there is a risk of information leak if the full buffer is copied as is
> out of the kernel - this needs manual checking.
As fasr as I can tell from lkml, only one of these patches has been
accepted? There was also a concern about an information leak that there
was no response to. Actually, I would prefer that more of the generated
patches are accepted before accepting the semantic patch, for something
that is not quite so obviously correct.
julia
>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
> v2:
> - Use strscpy instead of strlcpy, as strlcpy can read after the number
> of requested bytes in the source string, and none of the replaced users
> want to know the source string size length
> - Add longer semantic patch information, warning in particular for
> information leak
> - Lowered Confidence level to medium because of the possibility of
> information leak, that needs manual checking
> - Fix spacing of the diff section and removed unused virtual context
>
> v3:
> - Add license/copyright
> - Rewording of commit message
>
> I didn't see many other remarks, but kept SUGGESTION as discussed.
> I didn't move all virtuals in a single line because none of the other
> kernel patch do it, and still do not see any advantage of moving the
> string to not use a variable so kept that as well.
>
> This should hopefully be the last version :)
>
> .../coccinelle/misc/strncpy_truncation.cocci | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
>
> diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
> new file mode 100644
> index 000000000000..7732cde23a85
> --- /dev/null
> +++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Use strscpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
> +///
> +//# This makes an effort to find occurences of strncpy followed by forced
> +//# truncation, which can generate gcc stringop-truncation warnings when
> +//# source and destination buffers are the same size.
> +//# Using strscpy will always do that nul-termination for us and not read
> +//# more than the maximum bytes requested in src, use that instead.
> +//#
> +//# The result needs checking that the destination buffer does not need
> +//# its tail zeroed (either cleared beforehand or will never leave the
> +//# kernel) so as not to leak information
> +//
> +// Confidence: Medium
> +// Copyright: (C) 2018 Dominique Martinet
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual report
> +virtual org
> +
> +@r@
> +expression dest, src, sz;
> +position p;
> +@@
> +
> +strncpy@p(dest, src, sz);
> +dest[sz - 1] = '\0';
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +msg = "strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> +cocci.print_main(msg, p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> +coccilib.report.print_report(p[0], msg)
> +
> +@ok depends on patch@
> +expression r.dest, r.src, r.sz;
> +@@
> +
> +- strncpy
> ++ strscpy
> + (dest, src, sz);
> +- dest[sz - 1] = '\0';
> --
> 2.17.1
>
>
Julia Lawall wrote on Fri, Jul 20, 2018:
> > strscpy does however not clear the end of the destination buffer, so
> > there is a risk of information leak if the full buffer is copied as is
> > out of the kernel - this needs manual checking.
>
> As fasr as I can tell from lkml, only one of these patches has been
> accepted? There was also a concern about an information leak that there
> was no response to. Actually, I would prefer that more of the generated
> patches are accepted before accepting the semantic patch, for something
> that is not quite so obviously correct.
As I'm pointing to the script which generated the patch in the generated
patches, I got told that it would be better to get the coccinelle script
accepted first, and asked others to hold on taking the patches at
several places - I didn't resend any v2 of these with strscpy yet mostly
for that reason.
There were concerns for information leaks that I believe I adressed in
the specific patch that was pointed out by the concern (I might have
missed some?), but I'll take the time to check all the patches
individually before resending as well as filling in better commit
messages which also was one of the main concerns.
I'm however a bit stuck if I'm waiting for the cocinelle script to be
accepted to resend the patches, but you're waiting for the individual
patches to be accepted to take the script... :)
I guess there is no value in the script landing first by itself, I'll
just remove the script path from the commit messages and resend the
first few this weekend.
--
Dominique Martinet
Julia Lawall wrote on Fri, Jul 20, 2018:
> On Fri, 20 Jul 2018, Dominique Martinet wrote:
> > I guess there is no value in the script landing first by itself, I'll
> > just remove the script path from the commit messages and resend the
> > first few this weekend.
>
> It's not that there is no value to the script. The problem is that I
> don't know if the script is correct - I'm not familiar with these string
> functions. Once the script is in the kernel, it stays there beyond your
> patches, so I would prefer to know that it is correct up front, rather
> than having to remove it afterwards.
I understand, I didn't say there is no value in the script ("landing
first by itself" doesn't mean it should/can not be taken later)
I'll bump this thread again in a couple of weeks after having resent
most of the other patches
--
Dominique Martinet
On Fri, 20 Jul 2018, Dominique Martinet wrote:
> Julia Lawall wrote on Fri, Jul 20, 2018:
> > > strscpy does however not clear the end of the destination buffer, so
> > > there is a risk of information leak if the full buffer is copied as is
> > > out of the kernel - this needs manual checking.
> >
> > As fasr as I can tell from lkml, only one of these patches has been
> > accepted? There was also a concern about an information leak that there
> > was no response to. Actually, I would prefer that more of the generated
> > patches are accepted before accepting the semantic patch, for something
> > that is not quite so obviously correct.
>
> As I'm pointing to the script which generated the patch in the generated
> patches, I got told that it would be better to get the coccinelle script
> accepted first, and asked others to hold on taking the patches at
> several places - I didn't resend any v2 of these with strscpy yet mostly
> for that reason.
I can't accept a semantic patch for which I can't judge the correctness.
It would be better to put a proper commit message in the individual
patches and get them accepted first.
The actual change is made by a script that is only a few lines long. You
can put those lines in your commit message if you like.
> There were concerns for information leaks that I believe I adressed in
> the specific patch that was pointed out by the concern (I might have
> missed some?), but I'll take the time to check all the patches
> individually before resending as well as filling in better commit
> messages which also was one of the main concerns.
>
> I'm however a bit stuck if I'm waiting for the cocinelle script to be
> accepted to resend the patches, but you're waiting for the individual
> patches to be accepted to take the script... :)
>
>
> I guess there is no value in the script landing first by itself, I'll
> just remove the script path from the commit messages and resend the
> first few this weekend.
It's not that there is no value to the script. The problem is that I
don't know if the script is correct - I'm not familiar with these string
functions. Once the script is in the kernel, it stays there beyond your
patches, so I would prefer to know that it is correct up front, rather
than having to remove it afterwards.
julia
On Fri, 20 Jul 2018, Dominique Martinet wrote:
> Julia Lawall wrote on Fri, Jul 20, 2018:
> > On Fri, 20 Jul 2018, Dominique Martinet wrote:
> > > I guess there is no value in the script landing first by itself, I'll
> > > just remove the script path from the commit messages and resend the
> > > first few this weekend.
> >
> > It's not that there is no value to the script. The problem is that I
> > don't know if the script is correct - I'm not familiar with these string
> > functions. Once the script is in the kernel, it stays there beyond your
> > patches, so I would prefer to know that it is correct up front, rather
> > than having to remove it afterwards.
>
> I understand, I didn't say there is no value in the script ("landing
> first by itself" doesn't mean it should/can not be taken later)
>
> I'll bump this thread again in a couple of weeks after having resent
> most of the other patches
Thanks.
The rule is also not so efficient in the patch case, because you have the
rule r that matches the pattern, and then the ok rule at the end that
matches the same pattern. It would be better to put depends on org ||
report in the rule r, and let the patch rule be freestanding, ie just
declare dest, src, and sz, not r.dest, etc.
If you like, you can also add the context case by just putting a * in
front of the strncpy call in the r rule. That highlights the change in
diff-like output, which can in general be useful to see the context in
which the issue occurs.
julia
> I didn't move all virtuals in a single line because none of the other
> kernel patch do it,
It can be occasionally useful to preserve another bit of coding tradition.
I dared to propose the reconsideration of such an implementation detail.
> and still do not see any advantage of moving the string to not use a variable
> so kept that as well.
I am curious if other software developers (besides me) would like to achieve
more source code reduction according to the principle “Don't repeat yourself”.
> This should hopefully be the last version :)
* Do any related development concerns need further safety checks for this
evolving source code transformation pattern?
* Julia Lawall has presented additional development ideas today.
https://lore.kernel.org/lkml/alpine.DEB.2.20.1807200759230.2349@hadrien/
https://lkml.org/lkml/2018/7/20/71
Regards,
Markus
> The problem is that I don't know if the script is correct
Will the clarification of such a concern evolve into another interesting
software development adventure?
> - I'm not familiar with these string functions.
How are the chances to improve the understanding of affected programming
interfaces anyhow?
https://elixir.bootlin.com/linux/v4.18-rc5/source/lib/string.c#L154
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/string.c?id=28c20cc73b9cc4288c86c2a3fc62af4087de4b19#n154
Regards,
Markus
On Mon, 16 Jul 2018 13:42:06 +0200
Dominique Martinet <[email protected]> wrote:
> Jonathan Cameron wrote on Sun, Jul 15, 2018:
> > On Fri, 13 Jul 2018 03:25:34 +0200
> > Dominique Martinet <[email protected]> wrote:
> > > Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> > >
> > > Signed-off-by: Dominique Martinet <[email protected]>
> >
> > Applied to the togreg branch of iio.git and pushed out as testing
> > for the autobuilders to play with it.
>
> Thanks!
>
> I have been pointed out that strlcpy, unlike strncpy, will read past the
> size given in the input string and thus is Bad™ if the input string is
> not nul terminated.
>
> After taking the time to check I believe this should not happen as the
> original name seems to come from a dentry's d_name after proper
> preparation (a buffer is allocated precisely for this purpose), but it
> will not hurt to wait for that version.
>
>
> The second reason I was waiting is that I intended to check for each
> patch if it is safe to not pad the end of the string with zeroes (to
> avoid e.g. information leaks) and that seems OK as well here after a
> quick check but I wouldn't trust my own eyes this late so I'll let you
> be judge of that if you feel like taking v1 anyway.
>
> Otherwise, I'll recheck properly and submit a v2 with strscpy and a
> better commit message after the coccinelle script is taken for inclusion
> and doing a better check but this might take a while longer.
>
>
> Thanks,
In this particular case I'm fairly sure it is safe so I'll leave it as is.
Thanks,
Jonathan
On 07/12/2018 07:26 PM, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
> tools/power/cpupower/bench/parse.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
> index 9ba8a44ad2a7..1566b89989b2 100644
> --- a/tools/power/cpupower/bench/parse.c
> +++ b/tools/power/cpupower/bench/parse.c
> @@ -221,9 +221,8 @@ int prepare_config(const char *path, struct config *config)
> sscanf(val, "%u", &config->cpu);
>
> else if (strcmp("governor", opt) == 0) {
> - strncpy(config->governor, val,
> + strlcpy(config->governor, val,
> sizeof(config->governor));
> - config->governor[sizeof(config->governor) - 1] = '\0';
> }
>
> else if (strcmp("priority", opt) == 0) {
>
Thanks for the patch. I will pull this in for 4.19-rc1
-- Shuah
Greg Kroah-Hartman <[email protected]> writes:
> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>>
>> Signed-off-by: Dominique Martinet <[email protected]>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)
I agree and I'll drop this. If this is still needed please resend with a
better commit log.
--
Kalle Valo
Hello!
On 12 July 2018 at 20:26, Dominique Martinet <[email protected]> wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
> tools/power/cpupower/bench/parse.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
> index 9ba8a44ad2a7..1566b89989b2 100644
> --- a/tools/power/cpupower/bench/parse.c
> +++ b/tools/power/cpupower/bench/parse.c
> @@ -221,9 +221,8 @@ int prepare_config(const char *path, struct config *config)
> sscanf(val, "%u", &config->cpu);
>
> else if (strcmp("governor", opt) == 0) {
> - strncpy(config->governor, val,
> + strlcpy(config->governor, val,
> sizeof(config->governor));
> - config->governor[sizeof(config->governor) - 1] = '\0';
> }
>
> else if (strcmp("priority", opt) == 0) {
> --
> 2.17.1
I can't get cpupower to compile anymore now that it made its way to linux-next:
[/linux/tools/power/cpupower]$ make
CC lib/cpufreq.o
[...]
make[1]: Entering directory '/linux/tools/power/cpupower/bench'
CC main.o
CC parse.o
parse.c: In function ‘prepare_config’:
parse.c:224:4: warning: implicit declaration of function ‘strlcpy’
[-Wimplicit-function-declaration]
strlcpy(config->governor, val,
^
CC system.o
CC benchmark.o
CC cpufreq-bench
.//parse.o: In function `prepare_config':
/linux/tools/power/cpupower/bench/parse.c:224: undefined reference
to `strlcpy'
collect2: error: ld returned 1 exit status
Makefile:25: recipe for target 'cpufreq-bench' failed
make[1]: *** [cpufreq-bench] Error 1
make[1]: Leaving directory '/linux/tools/power/cpupower/bench'
Makefile:258: recipe for target 'compile-bench' failed
make: *** [compile-bench] Error 2
Does it need anything special to make?
Greetings!
Daniel Díaz
[email protected]
Daniel Díaz wrote on Tue, Aug 14, 2018:
> I can't get cpupower to compile anymore now that it made its way to linux-next:
> [/linux/tools/power/cpupower]$ make
> CC lib/cpufreq.o
> [...]
> make[1]: Entering directory '/linux/tools/power/cpupower/bench'
> CC main.o
> CC parse.o
> parse.c: In function ‘prepare_config’:
> parse.c:224:4: warning: implicit declaration of function ‘strlcpy’
> [-Wimplicit-function-declaration]
> strlcpy(config->governor, val,
> ^
> CC system.o
> CC benchmark.o
> CC cpufreq-bench
> .//parse.o: In function `prepare_config':
> /linux/tools/power/cpupower/bench/parse.c:224: undefined reference
> to `strlcpy'
> collect2: error: ld returned 1 exit status
> Makefile:25: recipe for target 'cpufreq-bench' failed
> make[1]: *** [cpufreq-bench] Error 1
> make[1]: Leaving directory '/linux/tools/power/cpupower/bench'
> Makefile:258: recipe for target 'compile-bench' failed
> make: *** [compile-bench] Error 2
>
> Does it need anything special to make?
Ugh, no, I am really ashamed about this patch series for insufficient
testing in general. It is currently "under rework" for an indefinite
time frame as I have had other priorities but I'll add cpupower to the
list...
More precisely, the function is defined in the linux kernel but for
userspace strlcpy is only available through libbsd, and I don't believe
we should pull that in just for this.
I'll send a second patch using snprintf and warning if a truncation
occurs (which is the proper fix that the gcc folks intended people to do
anyway) when I get around to it, but I would recommend to just revert
the patch for now.
Shuah, could you take the patch off please if you haven't pushed it to
linus yet?
Sorry for the time you might have spent on this,
--
Dominique Martinet
On 08/14/2018 01:27 PM, Dominique Martinet wrote:
> Daniel Díaz wrote on Tue, Aug 14, 2018:
>> I can't get cpupower to compile anymore now that it made its way to linux-next:
>> [/linux/tools/power/cpupower]$ make
>> CC lib/cpufreq.o
>> [...]
>> make[1]: Entering directory '/linux/tools/power/cpupower/bench'
>> CC main.o
>> CC parse.o
>> parse.c: In function ‘prepare_config’:
>> parse.c:224:4: warning: implicit declaration of function ‘strlcpy’
>> [-Wimplicit-function-declaration]
>> strlcpy(config->governor, val,
>> ^
>> CC system.o
>> CC benchmark.o
>> CC cpufreq-bench
>> .//parse.o: In function `prepare_config':
>> /linux/tools/power/cpupower/bench/parse.c:224: undefined reference
>> to `strlcpy'
>> collect2: error: ld returned 1 exit status
>> Makefile:25: recipe for target 'cpufreq-bench' failed
>> make[1]: *** [cpufreq-bench] Error 1
>> make[1]: Leaving directory '/linux/tools/power/cpupower/bench'
>> Makefile:258: recipe for target 'compile-bench' failed
>> make: *** [compile-bench] Error 2
>>
>> Does it need anything special to make?
>
> Ugh, no, I am really ashamed about this patch series for insufficient
> testing in general. It is currently "under rework" for an indefinite
> time frame as I have had other priorities but I'll add cpupower to the
> list...
> More precisely, the function is defined in the linux kernel but for
> userspace strlcpy is only available through libbsd, and I don't believe
> we should pull that in just for this.
>
> I'll send a second patch using snprintf and warning if a truncation
> occurs (which is the proper fix that the gcc folks intended people to do
> anyway) when I get around to it, but I would recommend to just revert
> the patch for now.
>
>
> Shuah, could you take the patch off please if you haven't pushed it to
> linus yet?
>
>
> Sorry for the time you might have spent on this,
>
I will go ahead and revert it.
thanks,
-- Shuah
Jens,
I noticed this old patch in my inbox. It looks like a legit cleanup.
Want to take it?
-- Steve
On Fri, 13 Jul 2018 03:26:02 +0200
Dominique Martinet <[email protected]> wrote:
> Using strlcpy fixes this new gcc warning:
> kernel/trace/blktrace.c: In function ‘do_blk_trace_setup’:
> kernel/trace/blktrace.c:497:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
> strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <[email protected]>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
> kernel/trace/blktrace.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 987d9a9ae283..2478d9838eab 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -494,8 +494,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
> if (!buts->buf_size || !buts->buf_nr)
> return -EINVAL;
>
> - strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
> - buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
> + strlcpy(buts->name, name, BLKTRACE_BDEV_SIZE);
>
> /*
> * some device names have larger paths - convert the slashes
On 3/14/19 7:37 PM, Steven Rostedt wrote:
>
> Jens,
>
> I noticed this old patch in my inbox. It looks like a legit cleanup.
> Want to take it?
Indeed, I've applied it. Thanks!
--
Jens Axboe
Jens, Steven,
Jens Axboe wrote on Thu, Mar 14, 2019:
> On 3/14/19 7:37 PM, Steven Rostedt wrote:
> > I noticed this old patch in my inbox. It looks like a legit cleanup.
> > Want to take it?
>
> Indeed, I've applied it. Thanks!
Thanks. I am terribly sorry about this patch series to be honest, I did
not prepare it properly and sent too many generic patches at once but
more importantly some were unsafe (strlcpy expects the input string to
be validly formatted, because it basically does strlen() on it to check
how much hasn't been copied for its return value)
I was pointed out strscpy instead as a safer alternative.
In this case `name` comes from bdevname() which is disk_name() in
block/partition-generic.c which is a snprintf, so we are guaranted
null truncation from there and it should be OK, but I wanted to check
and point it out.
Anyway, thanks!
--
Dominique
On 3/15/19 12:30 AM, Dominique Martinet wrote:
> Jens, Steven,
>
> Jens Axboe wrote on Thu, Mar 14, 2019:
>> On 3/14/19 7:37 PM, Steven Rostedt wrote:
>>> I noticed this old patch in my inbox. It looks like a legit cleanup.
>>> Want to take it?
>>
>> Indeed, I've applied it. Thanks!
>
> Thanks. I am terribly sorry about this patch series to be honest, I did
> not prepare it properly and sent too many generic patches at once but
> more importantly some were unsafe (strlcpy expects the input string to
> be validly formatted, because it basically does strlen() on it to check
> how much hasn't been copied for its return value)
> I was pointed out strscpy instead as a safer alternative.
>
> In this case `name` comes from bdevname() which is disk_name() in
> block/partition-generic.c which is a snprintf, so we are guaranted
> null truncation from there and it should be OK, but I wanted to check
> and point it out.
Dropped.
--
Jens Axboe