[PATCH] Fix overflow of name in struct net_device, replaced str* and sprintf with strn* and snprintf respectivly.

Subsystems: networking [general], the rest

STALE5667d

2 messages, 2 authors, 2011-03-14 · open the first message on its own page

[PATCH] Fix overflow of name in struct net_device, replaced str* and sprintf with strn* and snprintf respectivly.

From: Sasikanth V <hidden>
Date: 2011-03-14 20:07:40

Signed-off-by: Sasikanth V <redacted>
---
 net/core/dev.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 6561021..9d06c1e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -510,7 +510,7 @@ int netdev_boot_setup_check(struct net_device *dev)
 
 	for (i = 0; i < NETDEV_BOOT_SETUP_MAX; i++) {
 		if (s[i].name[0] != '\0' && s[i].name[0] != ' ' &&
-		    !strcmp(dev->name, s[i].name)) {
+		    !strncmp(dev->name, s[i].name, IFNAMSIZ)) {
 			dev->irq 	= s[i].map.irq;
 			dev->base_addr 	= s[i].map.base_addr;
 			dev->mem_start 	= s[i].map.mem_start;
@@ -539,7 +539,7 @@ unsigned long netdev_boot_base(const char *prefix, int unit)
 	char name[IFNAMSIZ];
 	int i;
 
-	sprintf(name, "%s%d", prefix, unit);
+	snprintf(name, IFNAMSIZ, "%s%d", prefix, unit);
 
 	/*
 	 * If device already registered then return base of 1
@@ -549,7 +549,7 @@ unsigned long netdev_boot_base(const char *prefix, int unit)
 		return 1;
 
 	for (i = 0; i < NETDEV_BOOT_SETUP_MAX; i++)
-		if (!strcmp(name, s[i].name))
+		if (!strncmp(name, s[i].name, IFNAMSIZ))
 			return s[i].map.base_addr;
 	return 0;
 }
@@ -836,7 +836,7 @@ EXPORT_SYMBOL(dev_get_by_flags_rcu);
  */
 int dev_valid_name(const char *name)
 {
-	if (*name == '\0')
+	if (!name || *name == '\0')
 		return 0;
 	if (strlen(name) >= IFNAMSIZ)
 		return 0;
@@ -3834,7 +3834,7 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg)
 		return -ENODEV;
 	}
 
-	strcpy(ifr.ifr_name, dev->name);
+	strncpy(ifr.ifr_name, dev->name, IFNAMSIZ);
 	rcu_read_unlock();
 
 	if (copy_to_user(arg, &ifr, sizeof(struct ifreq)))
@@ -4821,7 +4821,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
 		return -EFAULT;
 
-	ifr.ifr_name[IFNAMSIZ-1] = 0;
+	ifr.ifr_name[IFNAMSIZ-1] = '\0';
 
 	colon = strchr(ifr.ifr_name, ':');
 	if (colon)
@@ -5694,7 +5694,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		goto free_all;
 #endif
 
-	strcpy(dev->name, name);
+	strncpy(dev->name, name, IFNAMSIZ);
 	return dev;
 
 free_all:
-- 
1.7.3.4

Re: [PATCH] Fix overflow of name in struct net_device, replaced str* and sprintf with strn* and snprintf respectivly.

From: Ben Hutchings <hidden>
Date: 2011-03-14 20:29:40

On Tue, 2011-03-15 at 01:37 +0530, Sasikanth V wrote:
quoted hunk
Signed-off-by: Sasikanth V <redacted>
---
 net/core/dev.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 6561021..9d06c1e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -510,7 +510,7 @@ int netdev_boot_setup_check(struct net_device *dev)
 
 	for (i = 0; i < NETDEV_BOOT_SETUP_MAX; i++) {
 		if (s[i].name[0] != '\0' && s[i].name[0] != ' ' &&
-		    !strcmp(dev->name, s[i].name)) {
+		    !strncmp(dev->name, s[i].name, IFNAMSIZ)) {
If s[i].name is too long, so what?  No change is required here.
quoted hunk
 			dev->irq 	= s[i].map.irq;
 			dev->base_addr 	= s[i].map.base_addr;
 			dev->mem_start 	= s[i].map.mem_start;
@@ -539,7 +539,7 @@ unsigned long netdev_boot_base(const char *prefix, int unit)
 	char name[IFNAMSIZ];
 	int i;
 
-	sprintf(name, "%s%d", prefix, unit);
+	snprintf(name, IFNAMSIZ, "%s%d", prefix, unit);
This looks reasonable.
quoted hunk
 	/*
 	 * If device already registered then return base of 1
@@ -549,7 +549,7 @@ unsigned long netdev_boot_base(const char *prefix, int unit)
 		return 1;
 
 	for (i = 0; i < NETDEV_BOOT_SETUP_MAX; i++)
-		if (!strcmp(name, s[i].name))
+		if (!strncmp(name, s[i].name, IFNAMSIZ))
Not required.
quoted hunk
 			return s[i].map.base_addr;
 	return 0;
 }
@@ -836,7 +836,7 @@ EXPORT_SYMBOL(dev_get_by_flags_rcu);
  */
 int dev_valid_name(const char *name)
 {
-	if (*name == '\0')
+	if (!name || *name == '\0')
Not required; passing NULL is a bug.
quoted hunk
 		return 0;
 	if (strlen(name) >= IFNAMSIZ)
 		return 0;
@@ -3834,7 +3834,7 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg)
 		return -ENODEV;
 	}
 
-	strcpy(ifr.ifr_name, dev->name);
+	strncpy(ifr.ifr_name, dev->name, IFNAMSIZ);
If dev->name is longer than IFNAMSIZ (including null terminator) than we
already failed.  This change is not required.
quoted hunk
 	rcu_read_unlock();
 
 	if (copy_to_user(arg, &ifr, sizeof(struct ifreq)))
@@ -4821,7 +4821,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
 		return -EFAULT;
 
-	ifr.ifr_name[IFNAMSIZ-1] = 0;
+	ifr.ifr_name[IFNAMSIZ-1] = '\0';
This is just noise.
 
quoted hunk
 	colon = strchr(ifr.ifr_name, ':');
 	if (colon)
@@ -5694,7 +5694,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		goto free_all;
 #endif
 
-	strcpy(dev->name, name);
+	strncpy(dev->name, name, IFNAMSIZ);
This doesn't help, as strncpy() does not ensure null termination.  And I
don't think truncating the name is helpful either.  Perhaps we should do
this right at the beginning of the function:

	if (WARN_ON(strlen(name) >= IFNAMSIZ))
		return NULL;

Ben.
 	return dev;
 
 free_all:
-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help