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@xxxxxxxxx>
> >>>>>> To: Florian Fainelli <f.fainelli@xxxxxxxxx>, Andrew Lunn <andrew@xxxxxxx>, David Miller <davem@xxxxxxxxxxxxx>
> >>>>>> CC: netdev@xxxxxxxxxxxxxxx <netdev@xxxxxxxxxxxxxxx>
> >>>>>>
> >>>>>> 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