linux-kernel - Re: [PATCH] power: supply: register HWMON devices with valid names

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] [day] [month] [year] [list]

Date:   Fri, 23 Aug 2019 11:03:54 +0200
From:   Romain Izard <romain.izard.pro@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Sebastian Reichel <sre@...nel.org>,
        Andrey Smirnov <andrew.smirnov@...il.com>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: supply: register HWMON devices with valid names

On Thu, Aug 22, 2019 at 09:12:07AM -0700, Guenter Roeck wrote:
> On Thu, Aug 22, 2019 at 05:09:19PM +0200, Romain Izard wrote:
> > With the introduction of the HWMON compatibility layer to the power
> > supply framework in Linux 5.3, all power supply devices' names can be
> > used directly to create HWMON devices with the same names.
> > 
> > But HWMON has rules on allowable names that are different from the power
> > supply framework. The dash character is forbidden, as it is used by the
> > libsensors library in userspace as a separator, whereas this character
> > is used in the device names in more than half of the existing power
> > supply drivers. This last case is consistent with the typical naming
> > usage with MFD and Device Tree.
> > 
> > This leads to warnings in the kernel log, with the format:
> > 
> > power_supply gpio-charger: hwmon: \
> > 	'gpio-charger' is not a valid name attribute, please fix
> > 
> > Add a protection to power_supply_add_hwmon_sysfs() that replaces any
> > dash in the device name with an underscore when registering with the
> > HWMON framework. Other forbidden characters (star, slash, space, tab,
> > newline) are not replaced, as they are not in common use.
> > 
> > Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer")
> > Signed-off-by: Romain Izard <romain.izard.pro@...il.com>
> > ---
> >  drivers/power/supply/power_supply_hwmon.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> > index 51fe60440d12..ebe964bd512c 100644
> > --- a/drivers/power/supply/power_supply_hwmon.c
> > +++ b/drivers/power/supply/power_supply_hwmon.c
> > @@ -284,6 +284,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> >  	struct device *dev = &psy->dev;
> >  	struct device *hwmon;
> >  	int ret, i;
> > +	const char *name;
> >  
> >  	if (!devres_open_group(dev, power_supply_add_hwmon_sysfs,
> >  			       GFP_KERNEL))
> > @@ -334,7 +335,19 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> >  		}
> >  	}
> >  
> > -	hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name,
> > +	name = psy->desc->name;
> > +	if (strchr(name, '-')) {
> > +		char *new_name;
> > +
> > +		new_name = devm_kstrdup(dev, name, GFP_KERNEL);
> > +		if (!new_name) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +		strreplace(new_name, '-', '_');
> > +		name = (const char *) new_name;
> 
> Is that typecast necessary ?

Indeed, it is not. I'll remove it if a V2 is necessary.
> 
> Other than that,
> 
> Reviewed-by: Guenter Roeck <linux@...ck-us.net>
> 
> > +	}
> > +	hwmon = devm_hwmon_device_register_with_info(dev, name,
> >  						psyhw,
> >  						&power_supply_hwmon_chip_info,
> >  						NULL);

Thanks for your review.

Best regards,
-- 
Romain Izard