[PATCH net-next] dev: Correctly get length of alias string in dev_set_alias()

We supply number of bytes available in @alias via @len
parameter to dev_set_alias() which is not the same
as zero terminated string length that can be shorter.

Both dev_set_alias() users (rtnetlink and sysfs) can
submit number of bytes up to IFALIASZ with actual string
length slightly shorter by putting '\0' not at @len - 1.

Use strnlen() to get length of zero terminated string
and not access beyond @len. Correct comment about @len
and explain how to unset alias (i.e. use zero for @len).

Signed-off-by: Serhey Popovych <redacted>
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b0eee49..d362fe6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1243,7 +1243,7 @@ int dev_change_name(struct net_device *dev, const char *newname)
  *	dev_set_alias - change ifalias of a device
  *	@dev: device
  *	@alias: name up to IFALIASZ
- *	@len: limit of bytes to copy from info
+ *	@len: number of bytes available in @alias, zero to unset current alias
  *
  *	Set ifalias for a device,
  */

@@ -1255,6 +1255,8 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
 		return -EINVAL;
 
 	if (len) {
+		len = strnlen(alias, len);
+
 		new_alias = kmalloc(sizeof(*new_alias) + len + 1, GFP_KERNEL);
 		if (!new_alias)
 			return -ENOMEM;

-- 
1.8.3.1

From: Serhey Popovych <redacted>
Date: Mon, 18 Dec 2017 23:38:35 +0200

We supply number of bytes available in @alias via @len
parameter to dev_set_alias() which is not the same
as zero terminated string length that can be shorter.

Both dev_set_alias() users (rtnetlink and sysfs) can
submit number of bytes up to IFALIASZ with actual string
length slightly shorter by putting '\0' not at @len - 1.

Use strnlen() to get length of zero terminated string
and not access beyond @len. Correct comment about @len
and explain how to unset alias (i.e. use zero for @len).

Signed-off-by: Serhey Popovych <redacted>

I don't really see this as useful, really.

In the sysfs case, we are not presented with a NULL terminated string.
Instead, the net sysfs code gives us a length that goes up until the
trailing newline character.  The sysfs case is never larger than the
actual string size + 1.

The netlink attribute is usually sized appropriately for whatever the
string length actually is.

This therefore just seems to add an new strnlen() unnecessarily to
this code path, which rarely does anything helpful.

Thanks.

David Miller wrote:

From: Serhey Popovych <redacted>
Date: Mon, 18 Dec 2017 23:38:35 +0200

quoted

We supply number of bytes available in @alias via @len
parameter to dev_set_alias() which is not the same
as zero terminated string length that can be shorter.

Both dev_set_alias() users (rtnetlink and sysfs) can
submit number of bytes up to IFALIASZ with actual string
length slightly shorter by putting '\0' not at @len - 1.

Use strnlen() to get length of zero terminated string
and not access beyond @len. Correct comment about @len
and explain how to unset alias (i.e. use zero for @len).

Signed-off-by: Serhey Popovych <redacted>

I don't really see this as useful, really.

In the sysfs case, we are not presented with a NULL terminated string.
Instead, the net sysfs code gives us a length that goes up until the
trailing newline character.  The sysfs case is never larger than the
actual string size + 1.

The netlink attribute is usually sized appropriately for whatever the
string length actually is.

Sorry but I do not mean larger. I mean shorter. When nla_len() >
strlen() we allocate extra space up to IFALIASZ - 1.

This is definitely fix nothing: we never get above the bounds, but
in case if NULL terminator is in the middle of string with nla_len()
we might allocate unused extra space.

Sorry again if I'm not correct with above assumption.

This therefore just seems to add an new strnlen() unnecessarily to
this code path, which rarely does anything helpful.

Thanks.

From: Serhey Popovich <redacted>
Date: Wed, 20 Dec 2017 21:44:46 +0200

Sorry but I do not mean larger. I mean shorter. When nla_len() >
strlen() we allocate extra space up to IFALIASZ - 1.

But the user typically does not do that.

This is definitely fix nothing: we never get above the bounds, but
in case if NULL terminator is in the middle of string with nla_len()
we might allocate unused extra space.

See above, not worth optimizing for.