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
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 <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
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
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 <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
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
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 <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Eduardo Valentin <[email protected]>
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:
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 <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Eduardo Valentin <[email protected]>
> ---
> 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:
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 <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Eduardo Valentin <[email protected]>
> > ---
> > 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
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