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

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

[Serhey Popovych]

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 <serhe.popovych@gmail.com>
---
 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

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

[David Miller]

From: Serhey Popovych <serhe.popovych@gmail.com>
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 <serhe.popovych@gmail.com>

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.

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

[Serhey Popovich]

[-- Attachment #1.1: Type: text/plain, Size: 1639 bytes --]

David Miller wrote:
> From: Serhey Popovych <serhe.popovych@gmail.com>
> 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 <serhe.popovych@gmail.com>
> 
> 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.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[David Miller]

From: Serhey Popovich <serhe.popovych@gmail.com>
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.