From 1e9642ada02bb9c39257ad3bddf4b460c29a97ce Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 7 Jun 2013 10:08:37 +1000 Subject: [PATCH] clean up scary strncpy(dst, src, strlen(src)) uses Fix various weird constructions of strncpy(dst, src, strlen(src)). Length limits should be about the space available in the destination, not repurposed as a method to either always include or always exclude a trailing NULL byte. Either the NULL should always be copied (using strlcpy), or it should not be copied (using something like memcpy). Readable code should not depend on the weird behavior of strncpy when it hits the length limit. Better to avoid the anti-pattern entirely. Signed-off-by: Kees Cook Acked-by: Greg Kroah-Hartman [staging] Acked-by: Rafael J. Wysocki [acpi] Cc: Martin Schwidefsky Cc: Heiko Carstens Cc: Ursula Braun Cc: Frank Blaschka Cc: Richard Weinberger Signed-off-by: Andrew Morton --- Documentation/accounting/getdelays.c | 3 ++- drivers/acpi/sysfs.c | 3 +-- drivers/s390/net/qeth_l3_sys.c | 6 ++---- drivers/staging/tidspbridge/rmgr/drv_interface.c | 3 +-- fs/hppfs/hppfs.c | 11 ++++++----- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/Documentation/accounting/getdelays.c b/Documentation/accounting/getdelays.c index f8ebcde43b17..5e4773dc3e5f 100644 --- a/Documentation/accounting/getdelays.c +++ b/Documentation/accounting/getdelays.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -299,7 +300,7 @@ int main(int argc, char *argv[]) break; case 'C': containerset = 1; - strncpy(containerpath, optarg, strlen(optarg) + 1); + strlcpy(containerpath, optarg, sizeof(containerpath)); break; case 'w': logfile = strdup(optarg); diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c index 5c5d1624fa2c..05306a59aedc 100644 --- a/drivers/acpi/sysfs.c +++ b/drivers/acpi/sysfs.c @@ -677,10 +677,9 @@ void acpi_irq_stats_init(void) else sprintf(buffer, "bug%02X", i); - name = kzalloc(strlen(buffer) + 1, GFP_KERNEL); + name = kstrdup(buffer, GFP_KERNEL); if (name == NULL) goto fail; - strncpy(name, buffer, strlen(buffer) + 1); sysfs_attr_init(&counter_attrs[i].attr); counter_attrs[i].attr.name = name; diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c index e70af2406ff9..d1c8025b0b03 100644 --- a/drivers/s390/net/qeth_l3_sys.c +++ b/drivers/s390/net/qeth_l3_sys.c @@ -315,10 +315,8 @@ static ssize_t qeth_l3_dev_hsuid_store(struct device *dev, if (qeth_configure_cq(card, QETH_CQ_ENABLED)) return -EPERM; - for (i = 0; i < 8; i++) - card->options.hsuid[i] = ' '; - card->options.hsuid[8] = '\0'; - strncpy(card->options.hsuid, tmp, strlen(tmp)); + snprintf(card->options.hsuid, sizeof(card->options.hsuid), + "%-8s", tmp); ASCEBC(card->options.hsuid, 8); if (card->dev) memcpy(card->dev->perm_addr, card->options.hsuid, 9); diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c index df0f37ea1ee5..c4d632c6741a 100644 --- a/drivers/staging/tidspbridge/rmgr/drv_interface.c +++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c @@ -421,12 +421,11 @@ static int omap3_bridge_startup(struct platform_device *pdev) drv_datap->tc_wordswapon = tc_wordswapon; if (base_img) { - drv_datap->base_img = kmalloc(strlen(base_img) + 1, GFP_KERNEL); + drv_datap->base_img = kstrdup(base_img, GFP_KERNEL); if (!drv_datap->base_img) { err = -ENOMEM; goto err2; } - strncpy(drv_datap->base_img, base_img, strlen(base_img) + 1); } dev_set_drvdata(bridge, drv_datap); diff --git a/fs/hppfs/hppfs.c b/fs/hppfs/hppfs.c index cd3e38972c86..d619b83ec307 100644 --- a/fs/hppfs/hppfs.c +++ b/fs/hppfs/hppfs.c @@ -69,7 +69,7 @@ static char *dentry_name(struct dentry *dentry, int extra) struct dentry *parent; char *root, *name; const char *seg_name; - int len, seg_len; + int len, seg_len, root_len; len = 0; parent = dentry; @@ -81,7 +81,8 @@ static char *dentry_name(struct dentry *dentry, int extra) } root = "proc"; - len += strlen(root); + root_len = strlen(root); + len += root_len; name = kmalloc(len + extra + 1, GFP_KERNEL); if (name == NULL) return NULL; @@ -91,7 +92,7 @@ static char *dentry_name(struct dentry *dentry, int extra) while (parent->d_parent != parent) { if (is_pid(parent)) { seg_name = "pid"; - seg_len = strlen("pid"); + seg_len = strlen(seg_name); } else { seg_name = parent->d_name.name; @@ -100,10 +101,10 @@ static char *dentry_name(struct dentry *dentry, int extra) len -= seg_len + 1; name[len] = '/'; - strncpy(&name[len + 1], seg_name, seg_len); + memcpy(&name[len + 1], seg_name, seg_len); parent = parent->d_parent; } - strncpy(name, root, strlen(root)); + memcpy(name, root, root_len); return name; } -- 2.39.5