[PATCHv2 0/2] hwmon: couple of fixes on HWMON_C_REGISTER_TZ

[PATCHv2 0/2] hwmon: couple of fixes on HWMON_C_REGISTER_TZ

[Eduardo Valentin]

Hello Guenter,

I found these bugs in the error path of hwmon_device_register().
One related to calling of-thermal when no dev->of_node is present.
And another in the error path handling of it.

Only difference from V1 is that I changed patch 2/2 to remove
the device_unregister() before jumping into the new label.

Eduardo Valentin (2):
  hwmon: core: add thermal sensors only if dev->of_node is present
  hwmon: core: fix potential memory leak in *hwmon_device_register*

 drivers/hwmon/hwmon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.21.0

[PATCHv2 1/2] hwmon: core: add thermal sensors only if dev->of_node is present

[Eduardo Valentin]

Drivers may register to hwmon and request for also registering
with the thermal subsystem (HWMON_C_REGISTER_TZ). However,
some of these driver, e.g. marvell phy, may be probed from
Device Tree or being dynamically allocated, and in the later
case, it will not have a dev->of_node entry.

Registering with hwmon without the dev->of_node may result in
different outcomes depending on the device tree, which may
be a bit misleading. If the device tree blob has no 'thermal-zones'
node, the *hwmon_device_register*() family functions are going
to gracefully succeed, because of-thermal,
*thermal_zone_of_sensor_register() return -ENODEV in this case,
and the hwmon error path handles this error code as success to
cover for the case where CONFIG_THERMAL_OF is not set.
However, if the device tree blob has the 'thermal-zones'
entry, the *hwmon_device_register*() will always fail on callers
with no dev->of_node, propagating -EINVAL.

If dev->of_node is not present, calling of-thermal does not
make sense. For this reason, this patch checks first if the
device has a of_node before going over the process of registering
with the thermal subsystem of-thermal interface. And in this case,
when a caller of *hwmon_device_register*() with HWMON_C_REGISTER_TZ
and no dev->of_node will still register with hwmon, but not with
the thermal subsystem. If all the hwmon part bits are in place,
the registration will succeed.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
V1->V2: no change

 drivers/hwmon/hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index e694c46ff039..429784edd5ff 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -636,7 +636,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	if (err)
 		goto free_hwmon;
 
-	if (dev && chip && chip->ops->read &&
+	if (dev && dev->of_node && chip && chip->ops->read &&
 	    chip->info[0]->type == hwmon_chip &&
 	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
 		const struct hwmon_channel_info **info = chip->info;
-- 
2.21.0

[PATCHv2 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Eduardo Valentin]

When registering a hwmon device with HWMON_C_REGISTER_TZ flag
in place, the hwmon subsystem will attempt to register the device
also with the thermal subsystem. When the of-thermal registration
fails, __hwmon_device_register jumps to ida_remove, leaving
the locally allocated hwdev pointer.

This patch fixes the leak by jumping to a new label that
will first unregister hdev and then fall into the kfree of hwdev
to finally remove the idas and propagate the error code.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
V1->V2: removed the device_unregister() before jumping
into the new label, as suggested in the first review round.

 drivers/hwmon/hwmon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 429784edd5ff..620f05fc412a 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -652,10 +652,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 				if (info[i]->config[j] & HWMON_T_INPUT) {
 					err = hwmon_thermal_add_sensor(dev,
 								hwdev, j);
-					if (err) {
-						device_unregister(hdev);
-						goto ida_remove;
-					}
+					if (err)
+						goto device_unregister;
 				}
 			}
 		}
@@ -663,6 +661,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 
 	return hdev;
 
+device_unregister:
+	device_unregister(hdev);
 free_hwmon:
 	kfree(hwdev);
 ida_remove:
-- 
2.21.0

Re: [PATCHv2 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Guenter Roeck]

On Wed, May 29, 2019 at 07:56:05PM -0700, Eduardo Valentin wrote:
> When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> in place, the hwmon subsystem will attempt to register the device
> also with the thermal subsystem. When the of-thermal registration
> fails, __hwmon_device_register jumps to ida_remove, leaving
> the locally allocated hwdev pointer.
> 
> This patch fixes the leak by jumping to a new label that
> will first unregister hdev and then fall into the kfree of hwdev
> to finally remove the idas and propagate the error code.
> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>

Applied.

Thanks,
Guenter

> ---
> V1->V2: removed the device_unregister() before jumping
> into the new label, as suggested in the first review round.
> 
>  drivers/hwmon/hwmon.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 429784edd5ff..620f05fc412a 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -652,10 +652,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  				if (info[i]->config[j] & HWMON_T_INPUT) {
>  					err = hwmon_thermal_add_sensor(dev,
>  								hwdev, j);
> -					if (err) {
> -						device_unregister(hdev);
> -						goto ida_remove;
> -					}
> +					if (err)
> +						goto device_unregister;
>  				}
>  			}
>  		}
> @@ -663,6 +661,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  
>  	return hdev;
>  
> +device_unregister:
> +	device_unregister(hdev);
>  free_hwmon:
>  	kfree(hwdev);
>  ida_remove:

Re: [PATCHv2 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Guenter Roeck]

On Wed, May 29, 2019 at 07:56:05PM -0700, Eduardo Valentin wrote:
> When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> in place, the hwmon subsystem will attempt to register the device
> also with the thermal subsystem. When the of-thermal registration
> fails, __hwmon_device_register jumps to ida_remove, leaving
> the locally allocated hwdev pointer.
> 
> This patch fixes the leak by jumping to a new label that
> will first unregister hdev and then fall into the kfree of hwdev
> to finally remove the idas and propagate the error code.
> 

Hah, actually this is wrong. hwdev is freed indirectly with the
device_unregister() call. See commit 74e3512731bd ("hwmon: (core)
Fix double-free in __hwmon_device_register()").

It may make sense to add a respective comment to the code, though.

Guenter

> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
> V1->V2: removed the device_unregister() before jumping
> into the new label, as suggested in the first review round.
> 
>  drivers/hwmon/hwmon.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 429784edd5ff..620f05fc412a 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -652,10 +652,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  				if (info[i]->config[j] & HWMON_T_INPUT) {
>  					err = hwmon_thermal_add_sensor(dev,
>  								hwdev, j);
> -					if (err) {
> -						device_unregister(hdev);
> -						goto ida_remove;
> -					}
> +					if (err)
> +						goto device_unregister;
>  				}
>  			}
>  		}
> @@ -663,6 +661,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  
>  	return hdev;
>  
> +device_unregister:
> +	device_unregister(hdev);
>  free_hwmon:
>  	kfree(hwdev);
>  ida_remove:

Re: [PATCHv2 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Eduardo Valentin]

On Wed, Jun 05, 2019 at 01:38:38PM -0700, Guenter Roeck wrote:
> On Wed, May 29, 2019 at 07:56:05PM -0700, Eduardo Valentin wrote:
> > When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> > in place, the hwmon subsystem will attempt to register the device
> > also with the thermal subsystem. When the of-thermal registration
> > fails, __hwmon_device_register jumps to ida_remove, leaving
> > the locally allocated hwdev pointer.
> > 
> > This patch fixes the leak by jumping to a new label that
> > will first unregister hdev and then fall into the kfree of hwdev
> > to finally remove the idas and propagate the error code.
> > 
> 
> Hah, actually this is wrong. hwdev is freed indirectly with the
> device_unregister() call. See commit 74e3512731bd ("hwmon: (core)
> Fix double-free in __hwmon_device_register()").

heh.. I see it now. Well, it is not a straight catch though. 

> 
> It may make sense to add a respective comment to the code, though.
> 

I agree. Or a simple comment saying "dont worry about freeing hwdev
because hwmon_dev_release() takes care of it".

Are you patching it ?

> Guenter
> 
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-hwmon@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> > V1->V2: removed the device_unregister() before jumping
> > into the new label, as suggested in the first review round.
> > 
> >  drivers/hwmon/hwmon.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 429784edd5ff..620f05fc412a 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -652,10 +652,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> >  				if (info[i]->config[j] & HWMON_T_INPUT) {
> >  					err = hwmon_thermal_add_sensor(dev,
> >  								hwdev, j);
> > -					if (err) {
> > -						device_unregister(hdev);
> > -						goto ida_remove;
> > -					}
> > +					if (err)
> > +						goto device_unregister;
> >  				}
> >  			}
> >  		}
> > @@ -663,6 +661,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> >  
> >  	return hdev;
> >  
> > +device_unregister:
> > +	device_unregister(hdev);
> >  free_hwmon:
> >  	kfree(hwdev);
> >  ida_remove:

-- 
All the best,
Eduardo Valentin

Re: [PATCHv2 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Guenter Roeck]

On Thu, Jun 06, 2019 at 07:35:44AM -0700, Eduardo Valentin wrote:
> On Wed, Jun 05, 2019 at 01:38:38PM -0700, Guenter Roeck wrote:
> > On Wed, May 29, 2019 at 07:56:05PM -0700, Eduardo Valentin wrote:
> > > When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> > > in place, the hwmon subsystem will attempt to register the device
> > > also with the thermal subsystem. When the of-thermal registration
> > > fails, __hwmon_device_register jumps to ida_remove, leaving
> > > the locally allocated hwdev pointer.
> > > 
> > > This patch fixes the leak by jumping to a new label that
> > > will first unregister hdev and then fall into the kfree of hwdev
> > > to finally remove the idas and propagate the error code.
> > > 
> > 
> > Hah, actually this is wrong. hwdev is freed indirectly with the
> > device_unregister() call. See commit 74e3512731bd ("hwmon: (core)
> > Fix double-free in __hwmon_device_register()").
> 
> heh.. I see it now. Well, it is not a straight catch though. 
> 
> > 
> > It may make sense to add a respective comment to the code, though.
> > 
> 
> I agree. Or a simple comment saying "dont worry about freeing hwdev
> because hwmon_dev_release() takes care of it".
> 
> Are you patching it ?
> 

Will do. I'll send a patch in a minute.

Thanks,
Guenter