netdev - Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa1f64d2-32a1-b8f9-0929-093fbd45d219@roeck-us.net>
Date:   Tue, 29 Mar 2022 20:46:48 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     David Laight <David.Laight@...LAB.COM>,
        'Michael Walle' <michael@...le.cc>,
        Xu Yilun <yilun.xu@...el.com>, Tom Rix <trix@...hat.com>,
        Jean Delvare <jdelvare@...e.com>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

On 3/29/22 19:57, David Laight wrote:
> From: Michael Walle
>> Sent: 29 March 2022 17:07
>>
>> More and more drivers will check for bad characters in the hwmon name
>> and all are using the same code snippet. Consolidate that code by adding
>> a new hwmon_sanitize_name() function.
> 
> I'm assuming these 'bad' hwmon names come from userspace?
> Like ethernet interface names??
> 
> Is silently changing the name of the hwmon entries the right
> thing to do at all?
> 
> What happens if the user tries to create both "foo_bar" and "foo-bar"?
> I'm sure that is going to go horribly wrong somewhere.
> 
> It would certainly make sense to have a function to verify the name
> is actually valid.
> Then bad names can be rejected earlier on.
> 
> I'm also intrigued about the list of invalid characters:
> 
> +static bool hwmon_is_bad_char(const char ch)
> +{
> +	switch (ch) {
> +	case '-':
> +	case '*':
> +	case ' ':
> +	case '\t':
> +	case '\n':
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> 
> If '\t' and '\n' are invalid why are all the other control characters
> allowed?
> I'm guessing '*' is disallowed because it is the shell wildcard?
> So what about '?'.
> Then I'd expect '/' to be invalid - but that isn't checked.
> Never mind all the values 0x80 to 0xff - they are probably worse
> than whitespace.
> 
> OTOH why are any characters invalid at all - except '/'?
> 

The name is supposed to reflect a driver name. Usually driver names
are not defined by userspace but by driver authors. The name is used
by libsensors to distinguish a driver from its instantiation.
libsensors uses wildcards in /etc/sensors3.conf. Duplicate names
are expected; there can be many instances of the same driver in
the system. For example, on the system I am typing this on, I have:

/sys/class/hwmon/hwmon0/name:nvme
/sys/class/hwmon/hwmon1/name:nvme
/sys/class/hwmon/hwmon2/name:nouveau
/sys/class/hwmon/hwmon3/name:nct6797
/sys/class/hwmon/hwmon4/name:jc42
/sys/class/hwmon/hwmon5/name:jc42
/sys/class/hwmon/hwmon6/name:jc42
/sys/class/hwmon/hwmon7/name:jc42
/sys/class/hwmon/hwmon8/name:k10temp

hwmon_is_bad_char() filters out characters which interfere with
libsensor's view of driver instances and the configuration data
in /etc/sensors3.conf. For example, again on my system, the
"sensors" command reports the following jc42 and nvme sensors.

jc42-i2c-0-1a
jc42-i2c-0-18
jc42-i2c-0-1b
jc42-i2c-0-19
nvme-pci-0100
nvme-pci-2500

In /etc/sensors3.conf, there might be entries for "jc42-*" or "nvme-*".
I don't think libsensors cares if a driver is named "this/is/my/driver".
That driver would then, assuming it is an i2c driver, show up
with the sensors command as "this/is/my/driver-i2c-0-25" or similar.
If it is named "this%is%my%driver", it would be something like
"this%is%my%driver-i2c-0-25". And so on. We can not permit "jc-42"
because libsensors would not be able to parse something like
"jc-42-*" or "jc-42-i2c-*".

Taking your example, if driver authors implement two drivers, one
named foo-bar and the other foo_bar, it would be the driver authors'
responsibility to provide valid driver names to the hwmon subsystem,
whatever those names might be. If both end up named "foo_bar" and can
as result not be distinguished from each other by libsensors,
or a user of the "sensors" command, that would be entirely the
responsibility of the driver authors. The only involvement of the
hwmon subsystem - and that is optional - would be to provide means
to the drivers to help them ensure that the names are valid, but
not that they are unique.

If there is ever a driver with a driver name that interferes with
libsensors' ability to distinguish the driver name from interface/port
information, we'll be happy to add the offending character(s)
to hwmon_is_bad_char(). Until then, being picky doesn't really
add any value and appears pointless.

Thanks,
Guenter