Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

I submitted this through the netdev tree, maybe relevant for you as well.
See also here: https://marc.info/?t=155103900100003&r=1&w=2

-------- Forwarded Message --------
Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
Date: Sun, 3 Mar 2019 18:20:50 +0100
From: Heiner Kallweit <hkallweit1@gmail.com>
To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org <netdev@vger.kernel.org>

Add a new function strreplace_nonalnum that replaces all
non-alphanumeric characters. Such functionality is needed e.g. when a
string is supposed to be used in a sysfs file name. If '\0' is given
as new character then non-alphanumeric characters are cut. 

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/string.h |  1 +
 lib/string.c           | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f..d827b0b0f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
 #endif
 void *memchr_inv(const void *s, int c, size_t n);
 char *strreplace(char *s, char old, char new);
+char *strreplace_nonalnum(char *s, char new);
 
 extern void kfree_const(const void *x);
 
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e..f2b1baf96 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
 }
 EXPORT_SYMBOL(strreplace);
 
+/**
+ * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
+ * @s: The string to operate on.
+ * @new: The character non-alphanumeric characters are replaced with.
+ *
+ * If new is '\0' then non-alphanumeric characters are cut.
+ *
+ * Returns pointer to the nul byte at the end of the modified string.
+ */
+char *strreplace_nonalnum(char *s, char new)
+{
+	char *p = s;
+
+	for (; *s; ++s)
+		if (isalnum(*s)) {
+			if (p != s)
+				*p = *s;
+			++p;
+		} else if (new) {
+			*p++ = new;
+		}
+	*p = '\0';
+
+	return p;
+}
+EXPORT_SYMBOL(strreplace_nonalnum);
+
 void fortify_panic(const char *name)
 {
 	pr_emerg("detected buffer overflow in %s\n", name);
-- 
2.21.0

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> I submitted this through the netdev tree, maybe relevant for you as well.
> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> 
> -------- Forwarded Message --------
> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> Date: Sun, 3 Mar 2019 18:20:50 +0100
> From: Heiner Kallweit <hkallweit1@gmail.com>
> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> 
> Add a new function strreplace_nonalnum that replaces all
> non-alphanumeric characters. Such functionality is needed e.g. when a
> string is supposed to be used in a sysfs file name. If '\0' is given
> as new character then non-alphanumeric characters are cut. 

sysfs doesn't have any such requirements, it can use whatever you want
to give it for a filename.

So don't create a random kernel function for sysfs please.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/string.h |  1 +
>  lib/string.c           | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 7927b875f..d827b0b0f 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
>  #endif
>  void *memchr_inv(const void *s, int c, size_t n);
>  char *strreplace(char *s, char old, char new);
> +char *strreplace_nonalnum(char *s, char new);
>  
>  extern void kfree_const(const void *x);
>  
> diff --git a/lib/string.c b/lib/string.c
> index 38e4ca08e..f2b1baf96 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
>  }
>  EXPORT_SYMBOL(strreplace);
>  
> +/**
> + * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
> + * @s: The string to operate on.
> + * @new: The character non-alphanumeric characters are replaced with.
> + *
> + * If new is '\0' then non-alphanumeric characters are cut.
> + *
> + * Returns pointer to the nul byte at the end of the modified string.

Why do you need to point to the end of the string?


> + */
> +char *strreplace_nonalnum(char *s, char new)
> +{
> +	char *p = s;
> +
> +	for (; *s; ++s)
> +		if (isalnum(*s)) {
> +			if (p != s)
> +				*p = *s;
> +			++p;
> +		} else if (new) {
> +			*p++ = new;
> +		}
> +	*p = '\0';

No max length?  No error checking?  Surely we can do better, see the
long thread on the kernel-hardnening list about string functions please.

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>> I submitted this through the netdev tree, maybe relevant for you as well.
>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>
>> -------- Forwarded Message --------
>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>
>> Add a new function strreplace_nonalnum that replaces all
>> non-alphanumeric characters. Such functionality is needed e.g. when a
>> string is supposed to be used in a sysfs file name. If '\0' is given
>> as new character then non-alphanumeric characters are cut. 
> 
> sysfs doesn't have any such requirements, it can use whatever you want
> to give it for a filename.
> 
Even a slash?
HWMON drivers is an example where such functionality occurs open-coded.

> So don't create a random kernel function for sysfs please.
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/linux/string.h |  1 +
>>  lib/string.c           | 27 +++++++++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 7927b875f..d827b0b0f 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
>>  #endif
>>  void *memchr_inv(const void *s, int c, size_t n);
>>  char *strreplace(char *s, char old, char new);
>> +char *strreplace_nonalnum(char *s, char new);
>>  
>>  extern void kfree_const(const void *x);
>>  
>> diff --git a/lib/string.c b/lib/string.c
>> index 38e4ca08e..f2b1baf96 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
>>  }
>>  EXPORT_SYMBOL(strreplace);
>>  
>> +/**
>> + * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
>> + * @s: The string to operate on.
>> + * @new: The character non-alphanumeric characters are replaced with.
>> + *
>> + * If new is '\0' then non-alphanumeric characters are cut.
>> + *
>> + * Returns pointer to the nul byte at the end of the modified string.
> 
> Why do you need to point to the end of the string?
> 
I don't have to. I just tried to keep as close as possible to the original
strreplace().

> 
>> + */
>> +char *strreplace_nonalnum(char *s, char new)
>> +{
>> +	char *p = s;
>> +
>> +	for (; *s; ++s)
>> +		if (isalnum(*s)) {
>> +			if (p != s)
>> +				*p = *s;
>> +			++p;
>> +		} else if (new) {
>> +			*p++ = new;
>> +		}
>> +	*p = '\0';
> 
> No max length?  No error checking?  Surely we can do better, see the
> long thread on the kernel-hardnening list about string functions please.
> 
As for the review comment before, I basically used strreplace() as
template and extended it. What you say about hardening is right,
but it's open for quite a few functions, isn't it?
I'll have a look at the mail thread you mentioned.

> greg k-h
> 
Heiner

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >> I submitted this through the netdev tree, maybe relevant for you as well.
> >> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>
> >> -------- Forwarded Message --------
> >> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >> From: Heiner Kallweit <hkallweit1@gmail.com>
> >> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>
> >> Add a new function strreplace_nonalnum that replaces all
> >> non-alphanumeric characters. Such functionality is needed e.g. when a
> >> string is supposed to be used in a sysfs file name. If '\0' is given
> >> as new character then non-alphanumeric characters are cut. 
> > 
> > sysfs doesn't have any such requirements, it can use whatever you want
> > to give it for a filename.
> > 
> Even a slash?

Is a slash an illegal character for a file to have?  It's up to the vfs
to care about this, don't force random parts of the kernel to care :)

> HWMON drivers is an example where such functionality occurs open-coded.

Is that data coming from userspace or from a kernel driver?

thanks,

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>>>> I submitted this through the netdev tree, maybe relevant for you as well.
>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>>>
>>>> -------- Forwarded Message --------
>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>>>
>>>> Add a new function strreplace_nonalnum that replaces all
>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>>> string is supposed to be used in a sysfs file name. If '\0' is given
>>>> as new character then non-alphanumeric characters are cut. 
>>>
>>> sysfs doesn't have any such requirements, it can use whatever you want
>>> to give it for a filename.
>>>
>> Even a slash?
> 
> Is a slash an illegal character for a file to have?  It's up to the vfs
> to care about this, don't force random parts of the kernel to care :)
> 
>> HWMON drivers is an example where such functionality occurs open-coded.
> 
> Is that data coming from userspace or from a kernel driver?
> 
Usually from a kernel driver. That's what
Documentation/hwmon/hwmon-kernel-api.txt says:

All supported hwmon device registration functions only accept valid device
names. Device names including invalid characters (whitespace, '*', or '-')
will be rejected. The 'name' parameter is mandatory.

The hwmon subsystem has an own function to check for such characters:
hwmon_is_bad_char()


> thanks,
> 
> greg k-h
> 
Heiner

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> >> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> >>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >>>> I submitted this through the netdev tree, maybe relevant for you as well.
> >>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>>>
> >>>> -------- Forwarded Message --------
> >>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >>>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>>>
> >>>> Add a new function strreplace_nonalnum that replaces all
> >>>> non-alphanumeric characters. Such functionality is needed e.g. when a
> >>>> string is supposed to be used in a sysfs file name. If '\0' is given
> >>>> as new character then non-alphanumeric characters are cut. 
> >>>
> >>> sysfs doesn't have any such requirements, it can use whatever you want
> >>> to give it for a filename.
> >>>
> >> Even a slash?
> > 
> > Is a slash an illegal character for a file to have?  It's up to the vfs
> > to care about this, don't force random parts of the kernel to care :)
> > 
> >> HWMON drivers is an example where such functionality occurs open-coded.
> > 
> > Is that data coming from userspace or from a kernel driver?
> > 
> Usually from a kernel driver. That's what
> Documentation/hwmon/hwmon-kernel-api.txt says:

Usually?  So userspace can set the name?

> All supported hwmon device registration functions only accept valid device
> names. Device names including invalid characters (whitespace, '*', or '-')
> will be rejected. The 'name' parameter is mandatory.
> 
> The hwmon subsystem has an own function to check for such characters:
> hwmon_is_bad_char()

It looks like hwmon is the only thing that cares about this then, why
do you want to make this a common function?

thanks,

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 19:41, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
>> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
>>> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
>>>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
>>>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>>>>>> I submitted this through the netdev tree, maybe relevant for you as well.
>>>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>>>>>
>>>>>> -------- Forwarded Message --------
>>>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>>>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>>>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>>>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>>>>>
>>>>>> Add a new function strreplace_nonalnum that replaces all
>>>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>>>>> string is supposed to be used in a sysfs file name. If '\0' is given
>>>>>> as new character then non-alphanumeric characters are cut. 
>>>>>
>>>>> sysfs doesn't have any such requirements, it can use whatever you want
>>>>> to give it for a filename.
>>>>>
>>>> Even a slash?
>>>
>>> Is a slash an illegal character for a file to have?  It's up to the vfs
>>> to care about this, don't force random parts of the kernel to care :)
>>>
>>>> HWMON drivers is an example where such functionality occurs open-coded.
>>>
>>> Is that data coming from userspace or from a kernel driver?
>>>
>> Usually from a kernel driver. That's what
>> Documentation/hwmon/hwmon-kernel-api.txt says:
> 
> Usually?  So userspace can set the name?
> 
I'm not sure about that.

>> All supported hwmon device registration functions only accept valid device
>> names. Device names including invalid characters (whitespace, '*', or '-')
>> will be rejected. The 'name' parameter is mandatory.
>>
>> The hwmon subsystem has an own function to check for such characters:
>> hwmon_is_bad_char()
> 
> It looks like hwmon is the only thing that cares about this then, why
> do you want to make this a common function?
> 
In phylib we have a similar issue, sysfs is broken if a PHY driver name
includes a slash, typically if some driver author uses "10 / 100 Mbit"
or similar.
IIRC for HWMON there has been some longer discussion about how to deal
with naming, I'd be curious to hear Guenter's opinion on the topic.

> thanks,
> 
> greg k-h
> 
Heiner

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:47:29PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 19:41, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
> >> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> >>> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> >>>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> >>>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >>>>>> I submitted this through the netdev tree, maybe relevant for you as well.
> >>>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>>>>>
> >>>>>> -------- Forwarded Message --------
> >>>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >>>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >>>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >>>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>>>>>
> >>>>>> Add a new function strreplace_nonalnum that replaces all
> >>>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
> >>>>>> string is supposed to be used in a sysfs file name. If '\0' is given
> >>>>>> as new character then non-alphanumeric characters are cut. 
> >>>>>
> >>>>> sysfs doesn't have any such requirements, it can use whatever you want
> >>>>> to give it for a filename.
> >>>>>
> >>>> Even a slash?
> >>>
> >>> Is a slash an illegal character for a file to have?  It's up to the vfs
> >>> to care about this, don't force random parts of the kernel to care :)
> >>>
> >>>> HWMON drivers is an example where such functionality occurs open-coded.
> >>>
> >>> Is that data coming from userspace or from a kernel driver?
> >>>
> >> Usually from a kernel driver. That's what
> >> Documentation/hwmon/hwmon-kernel-api.txt says:
> > 
> > Usually?  So userspace can set the name?
> > 
> I'm not sure about that.

You might want to check :)

> >> All supported hwmon device registration functions only accept valid device
> >> names. Device names including invalid characters (whitespace, '*', or '-')
> >> will be rejected. The 'name' parameter is mandatory.
> >>
> >> The hwmon subsystem has an own function to check for such characters:
> >> hwmon_is_bad_char()
> > 
> > It looks like hwmon is the only thing that cares about this then, why
> > do you want to make this a common function?
> > 
> In phylib we have a similar issue, sysfs is broken if a PHY driver name
> includes a slash, typically if some driver author uses "10 / 100 Mbit"
> or similar.
> IIRC for HWMON there has been some longer discussion about how to deal
> with naming, I'd be curious to hear Guenter's opinion on the topic.

Making drivers "safe" from themselves is pretty foolish, as if they
don't know they are doing stupid things, they will continue to do those
stupid things :)

Having a driver name itself something bad like this is fine, nothing
breaks except no one can access any sysfs files from the driver, so the
driver needs to be fixed.

Now if userspace can set this string, that's a totaly different story,
that needs to be properly sanitized, because as we all know, "all input
is evil."

thanks,

greg k-h