[PATCH 0/7] net: core: devname allocation cleanups

[PATCH 0/7] net: core: devname allocation cleanups

[Rasmus Villemoes]

It's somewhat confusing to have both dev_alloc_name and
dev_get_valid_name. I can't see why the former is less strict than the
latter, so make them (or rather dev_alloc_name_ns and
dev_get_valid_name) equivalent, hardening dev_alloc_name() a little.

Obvious follow-up patches would be to only export one function, and
make dev_alloc_name a static inline wrapper for that (whichever name
is chosen for the exported interface). But maybe there is a good
reason the two exported interfaces do different checking, so I'll
refrain from including the trivial but tree-wide renaming in this
series.

Rasmus Villemoes (7):
  net: core: improve sanity checking in __dev_alloc_name
  net: core: move dev_alloc_name_ns a little higher
  net: core: eliminate dev_alloc_name{,_ns} code duplication
  net: core: drop pointless check in __dev_alloc_name
  net: core: check dev_valid_name in __dev_alloc_name
  net: core: maybe return -EEXIST in __dev_alloc_name
  net: core: dev_get_valid_name is now the same as dev_alloc_name_ns

 net/core/dev.c | 62 +++++++++++++++++++++-------------------------------------
 1 file changed, 22 insertions(+), 40 deletions(-)

-- 
2.11.0

[PATCH 1/7] net: core: improve sanity checking in __dev_alloc_name

[Rasmus Villemoes]

__dev_alloc_name is called from the public (and exported)
dev_alloc_name(), so we don't have a guarantee that strlen(name) is at
most IFNAMSIZ. If somebody manages to get __dev_alloc_name called with a
% char beyond the 31st character, we'd be making a snprintf() call that
will very easily crash the kernel (using an appropriate %p extension,
we'll likely dereference some completely bogus pointer).

In the normal case where strlen() is sane, we don't even save anything
by limiting to IFNAMSIZ, so just use strchr().

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 11596a302a26..87e19804757b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1062,7 +1062,7 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	unsigned long *inuse;
 	struct net_device *d;
 
-	p = strnchr(name, IFNAMSIZ-1, '%');
+	p = strchr(name, '%');
 	if (p) {
 		/*
 		 * Verify the string as this thing may have come from
-- 
2.11.0

[PATCH 2/7] net: core: move dev_alloc_name_ns a little higher

[Rasmus Villemoes]

No functional change.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 87e19804757b..240ae6bc1097 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1105,6 +1105,19 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	return -ENFILE;
 }
 
+static int dev_alloc_name_ns(struct net *net,
+			     struct net_device *dev,
+			     const char *name)
+{
+	char buf[IFNAMSIZ];
+	int ret;
+
+	ret = __dev_alloc_name(net, name, buf);
+	if (ret >= 0)
+		strlcpy(dev->name, buf, IFNAMSIZ);
+	return ret;
+}
+
 /**
  *	dev_alloc_name - allocate a name for a device
  *	@dev: device
@@ -1134,19 +1147,6 @@ int dev_alloc_name(struct net_device *dev, const char *name)
 }
 EXPORT_SYMBOL(dev_alloc_name);
 
-static int dev_alloc_name_ns(struct net *net,
-			     struct net_device *dev,
-			     const char *name)
-{
-	char buf[IFNAMSIZ];
-	int ret;
-
-	ret = __dev_alloc_name(net, name, buf);
-	if (ret >= 0)
-		strlcpy(dev->name, buf, IFNAMSIZ);
-	return ret;
-}
-
 int dev_get_valid_name(struct net *net, struct net_device *dev,
 		       const char *name)
 {
-- 
2.11.0

[PATCH 3/7] net: core: eliminate dev_alloc_name{,_ns} code duplication

[Rasmus Villemoes]

dev_alloc_name contained a BUG_ON(), which I moved to dev_alloc_name_ns;
the only other caller of that already has the same BUG_ON.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 240ae6bc1097..1077bfe97bde 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1112,6 +1112,7 @@ static int dev_alloc_name_ns(struct net *net,
 	char buf[IFNAMSIZ];
 	int ret;
 
+	BUG_ON(!net);
 	ret = __dev_alloc_name(net, name, buf);
 	if (ret >= 0)
 		strlcpy(dev->name, buf, IFNAMSIZ);
@@ -1134,16 +1135,7 @@ static int dev_alloc_name_ns(struct net *net,
 
 int dev_alloc_name(struct net_device *dev, const char *name)
 {
-	char buf[IFNAMSIZ];
-	struct net *net;
-	int ret;
-
-	BUG_ON(!dev_net(dev));
-	net = dev_net(dev);
-	ret = __dev_alloc_name(net, name, buf);
-	if (ret >= 0)
-		strlcpy(dev->name, buf, IFNAMSIZ);
-	return ret;
+	return dev_alloc_name_ns(dev_net(dev), dev, name);
 }
 EXPORT_SYMBOL(dev_alloc_name);
 
-- 
2.11.0

[PATCH 4/7] net: core: drop pointless check in __dev_alloc_name

[Rasmus Villemoes]

The only caller passes a stack buffer as buf, so it won't equal the
passed-in name. Moreover, we're already using buf as a scratch buffer
inside the if (p) {} block, so if buf and name were the same, that
snprintf() call would be overwriting its own format string.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1077bfe97bde..14541b7a3195 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1093,8 +1093,7 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 		free_page((unsigned long) inuse);
 	}
 
-	if (buf != name)
-		snprintf(buf, IFNAMSIZ, name, i);
+	snprintf(buf, IFNAMSIZ, name, i);
 	if (!__dev_get_by_name(net, buf))
 		return i;
 
-- 
2.11.0

[PATCH 5/7] net: core: check dev_valid_name in __dev_alloc_name

[Rasmus Villemoes]

We currently only exclude non-sysfs-friendly names via
dev_get_valid_name; there doesn't seem to be a reason to allow such
names when we're called via dev_alloc_name.

This does duplicate the dev_valid_name check in the dev_get_valid_name()
case; we'll fix that shortly.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 14541b7a3195..c0a92cf27566 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1062,6 +1062,9 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	unsigned long *inuse;
 	struct net_device *d;
 
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
 	p = strchr(name, '%');
 	if (p) {
 		/*
-- 
2.11.0

[PATCH 6/7] net: core: maybe return -EEXIST in __dev_alloc_name

[Rasmus Villemoes]

If we're given format string with no %d, -EEXIST is a saner error code.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c0a92cf27566..7c08b4ca7b76 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1104,7 +1104,7 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
 	 * when the name is long and there isn't enough space left
 	 * for the digits, or if all bits are used.
 	 */
-	return -ENFILE;
+	return p ? -ENFILE : -EEXIST;
 }
 
 static int dev_alloc_name_ns(struct net *net,
-- 
2.11.0

[PATCH 7/7] net: core: dev_get_valid_name is now the same as dev_alloc_name_ns

[Rasmus Villemoes]

If name contains a %, it's easy to see that this patch doesn't change
anything (other than eliminate the duplicate dev_valid_name
call). Otherwise, we'll now just spend a little time in snprintf()
copying name to the stack buffer allocated in dev_alloc_name_ns, and do
the __dev_get_by_name using that buffer rather than name.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 net/core/dev.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7c08b4ca7b76..e29eea26f9c1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1144,19 +1144,7 @@ EXPORT_SYMBOL(dev_alloc_name);
 int dev_get_valid_name(struct net *net, struct net_device *dev,
 		       const char *name)
 {
-	BUG_ON(!net);
-
-	if (!dev_valid_name(name))
-		return -EINVAL;
-
-	if (strchr(name, '%'))
-		return dev_alloc_name_ns(net, dev, name);
-	else if (__dev_get_by_name(net, name))
-		return -EEXIST;
-	else if (dev->name != name)
-		strlcpy(dev->name, name, IFNAMSIZ);
-
-	return 0;
+	return dev_alloc_name_ns(net, dev, name);
 }
 EXPORT_SYMBOL(dev_get_valid_name);
 
-- 
2.11.0

Re: [PATCH 6/7] net: core: maybe return -EEXIST in __dev_alloc_name

[Stephen Hemminger]

On Mon, 13 Nov 2017 00:15:09 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> If we're given format string with no %d, -EEXIST is a saner error code.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c0a92cf27566..7c08b4ca7b76 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1104,7 +1104,7 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf)
>  	 * when the name is long and there isn't enough space left
>  	 * for the digits, or if all bits are used.
>  	 */
> -	return -ENFILE;
> +	return p ? -ENFILE : -EEXIST;
>  }
>  
>  static int dev_alloc_name_ns(struct net *net,

This is potentially a change to user ABI with no real advantage.

Re: [PATCH 0/7] net: core: devname allocation cleanups

[Stephen Hemminger]

On Mon, 13 Nov 2017 00:15:03 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> It's somewhat confusing to have both dev_alloc_name and
> dev_get_valid_name. I can't see why the former is less strict than the
> latter, so make them (or rather dev_alloc_name_ns and
> dev_get_valid_name) equivalent, hardening dev_alloc_name() a little.
> 
> Obvious follow-up patches would be to only export one function, and
> make dev_alloc_name a static inline wrapper for that (whichever name
> is chosen for the exported interface). But maybe there is a good
> reason the two exported interfaces do different checking, so I'll
> refrain from including the trivial but tree-wide renaming in this
> series.
> 
> Rasmus Villemoes (7):
>   net: core: improve sanity checking in __dev_alloc_name
>   net: core: move dev_alloc_name_ns a little higher
>   net: core: eliminate dev_alloc_name{,_ns} code duplication
>   net: core: drop pointless check in __dev_alloc_name
>   net: core: check dev_valid_name in __dev_alloc_name
>   net: core: maybe return -EEXIST in __dev_alloc_name
>   net: core: dev_get_valid_name is now the same as dev_alloc_name_ns
> 
>  net/core/dev.c | 62 +++++++++++++++++++++-------------------------------------
>  1 file changed, 22 insertions(+), 40 deletions(-)
> 

Looks good to me. Can't see anything obviously wrong with this.
I think the two functions started out heading in different directions.

Re: [PATCH 0/7] net: core: devname allocation cleanups

[David Miller]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: Mon, 13 Nov 2017 00:15:03 +0100

> It's somewhat confusing to have both dev_alloc_name and
> dev_get_valid_name. I can't see why the former is less strict than the
> latter, so make them (or rather dev_alloc_name_ns and
> dev_get_valid_name) equivalent, hardening dev_alloc_name() a little.
> 
> Obvious follow-up patches would be to only export one function, and
> make dev_alloc_name a static inline wrapper for that (whichever name
> is chosen for the exported interface). But maybe there is a good
> reason the two exported interfaces do different checking, so I'll
> refrain from including the trivial but tree-wide renaming in this
> series.

Series applied, thanks.

RE: [PATCH 3/7] net: core: eliminate dev_alloc_name{,_ns} code duplication

[David Laight]

From: Rasmus Villemoes
> Sent: 12 November 2017 23:15
> dev_alloc_name contained a BUG_ON(), which I moved to dev_alloc_name_ns;
> the only other caller of that already has the same BUG_ON.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  net/core/dev.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 240ae6bc1097..1077bfe97bde 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1112,6 +1112,7 @@ static int dev_alloc_name_ns(struct net *net,
>  	char buf[IFNAMSIZ];
>  	int ret;
> 
> +	BUG_ON(!net);
>  	ret = __dev_alloc_name(net, name, buf);

Just delete it.
The NULL pointer dereference is as easy to debug as the BUG().

	David