[PATCH 1/2] iio: iio_alloc_device(): Free device correctly on error

[PATCH 1/2] iio: iio_alloc_device(): Free device correctly on error

[Lars-Peter Clausen]

Once device_initialize() has been called on a struct device the device must
be freed by decreasing the reference count rather than directly freeing the
underlying memory.

This is so that any additional state and resources associated with the
device get properly freed.

In this particular case there are no additional resources associated with
the device and no additional reference count. So there is no resource leak
or use-after-free by freeing the struct device directly

But in order to follow best practices and avoid accidental future breakage
use put_device() instead of kfree() to free the device when an error
occurs.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
No fixes tag since, while the code is wrong, it works. No leaks and no
use-after-free.
 drivers/iio/industrialio-core.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 463a63d5bf56..669218365277 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1615,7 +1615,8 @@ static void iio_dev_release(struct device *device)
 
 	iio_device_detach_buffers(indio_dev);
 
-	ida_simple_remove(&iio_ida, iio_dev_opaque->id);
+	if (iio_dev_opaque->id >= 0)
+		ida_simple_remove(&iio_ida, iio_dev_opaque->id);
 	kfree(iio_dev_opaque);
 }
 
@@ -1662,20 +1663,20 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	if (iio_dev_opaque->id < 0) {
 		/* cannot use a dev_err as the name isn't available */
 		pr_err("failed to get device id\n");
-		kfree(iio_dev_opaque);
-		return NULL;
+		goto err_put_device;
 	}
 
-	if (dev_set_name(&indio_dev->dev, "iio:device%d", iio_dev_opaque->id)) {
-		ida_simple_remove(&iio_ida, iio_dev_opaque->id);
-		kfree(iio_dev_opaque);
-		return NULL;
-	}
+	if (dev_set_name(&indio_dev->dev, "iio:device%d", iio_dev_opaque->id))
+		goto err_put_device;
 
 	INIT_LIST_HEAD(&iio_dev_opaque->buffer_list);
 	INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);
 
 	return indio_dev;
+
+err_put_device:
+	put_device(&indio_dev->dev);
+	return NULL;
 }
 EXPORT_SYMBOL(iio_device_alloc);
 
-- 
2.20.1

[PATCH 2/2] iio: viio_trigger_alloc(): Correctly free trigger on error

[Lars-Peter Clausen]

Once device_initialize() has been called on a struct device the device must
be freed by decreasing the reference count rather than directly freeing the
underlying memory.

This is so that any additional state and resources associated with the
device get properly freed.

In this particular case there are no additional resources associated with
the device and no additional reference count. So there is no resource leak
or use-after-free by freeing the struct device directly

But in order to follow best practices and avoid accidental future breakage
use put_device() instead of kfree() to free the device when an error
occurs.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-trigger.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 93990ff1dfe3..d566e8d4a14b 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -480,7 +480,7 @@ static void iio_trig_release(struct device *device)
 	struct iio_trigger *trig = to_iio_trigger(device);
 	int i;
 
-	if (trig->subirq_base) {
+	if (trig->subirq_base > 0) {
 		for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
 			irq_modify_status(trig->subirq_base + i,
 					  IRQ_NOAUTOEN,
@@ -541,11 +541,11 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 					    CONFIG_IIO_CONSUMERS_PER_TRIGGER,
 					    0);
 	if (trig->subirq_base < 0)
-		goto free_trig;
+		goto err_put_trig;
 
 	trig->name = kvasprintf(GFP_KERNEL, fmt, vargs);
 	if (trig->name == NULL)
-		goto free_descs;
+		goto err_put_trig;
 
 	trig->subirq_chip.name = trig->name;
 	trig->subirq_chip.irq_mask = &iio_trig_subirqmask;
@@ -559,10 +559,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 
 	return trig;
 
-free_descs:
-	irq_free_descs(trig->subirq_base, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
-free_trig:
-	kfree(trig);
+err_put_trig:
+	put_device(&trig->dev);
 	return NULL;
 }
 
-- 
2.20.1

Re: [PATCH 2/2] iio: viio_trigger_alloc(): Correctly free trigger on error

[Lars-Peter Clausen]

On 10/31/21 9:54 AM, Andy Shevchenko wrote:
>
>
> On Sunday, October 31, 2021, Lars-Peter Clausen <lars@metafoo.de 
> <mailto:lars@metafoo.de>> wrote:
>
>     Once device_initialize() has been called on a struct device the
>     device must
>     be freed by decreasing the reference count rather than directly
>     freeing the
>     underlying memory.
>
>     This is so that any additional state and resources associated with the
>     device get properly freed.
>
>     In this particular case there are no additional resources
>     associated with
>     the device and no additional reference count. So there is no
>     resource leak
>     or use-after-free by freeing the struct device directly
>
>     But in order to follow best practices and avoid accidental future
>     breakage
>     use put_device() instead of kfree() to free the device when an error
>     occurs.
>
>     Signed-off-by: Lars-Peter Clausen <lars@metafoo.de
>     <mailto:lars@metafoo.de>>
>     ---
>      drivers/iio/industrialio-trigger.c | 12 +++++-------
>      1 file changed, 5 insertions(+), 7 deletions(-)
>
>     diff --git a/drivers/iio/industrialio-trigger.c
>     b/drivers/iio/industrialio-trigger.c
>     index 93990ff1dfe3..d566e8d4a14b 100644
>     --- a/drivers/iio/industrialio-trigger.c
>     +++ b/drivers/iio/industrialio-trigger.c
>     @@ -480,7 +480,7 @@ static void iio_trig_release(struct device
>     *device)
>             struct iio_trigger *trig = to_iio_trigger(device);
>             int i;
>
>     -       if (trig->subirq_base) {
>     +       if (trig->subirq_base > 0) {
>
>
>
> >= ?

I don't know. 0 is not supposed to be a valid irq number. And we 
kzalloc() the struct, so if it hasn't been explicitly initialized we'd 
get 0.

The way the code is at the moment we'd never end up here without calling 
irq_alloc_descs(), so it is either a valid irq or a negative error code 
and I can see why you might want to use >= for consistency and symmetry.

>
>                     for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER;
>     i++) {
>                             irq_modify_status(trig->subirq_base + i,
>                                               IRQ_NOAUTOEN,
>     @@ -541,11 +541,11 @@ struct iio_trigger
>     *viio_trigger_alloc(struct device *parent,
>     CONFIG_IIO_CONSUMERS_PER_TRIGGER,
>                                                 0);
>             if (trig->subirq_base < 0)
>     -               goto free_trig;
>     +               goto err_put_trig;
>
>             trig->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>             if (trig->name == NULL)
>     -               goto free_descs;
>     +               goto err_put_trig;
>
>             trig->subirq_chip.name <http://subirq_chip.name> = trig->name;
>             trig->subirq_chip.irq_mask = &iio_trig_subirqmask;
>     @@ -559,10 +559,8 @@ struct iio_trigger *viio_trigger_alloc(struct
>     device *parent,
>
>             return trig;
>
>     -free_descs:
>     -       irq_free_descs(trig->subirq_base,
>     CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>     -free_trig:
>     -       kfree(trig);
>     +err_put_trig:
>     +       put_device(&trig->dev);
>             return NULL;
>      }
>
>     -- 
>     2.20.1
>
>
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 2/2] iio: viio_trigger_alloc(): Correctly free trigger on error

[Andy Shevchenko]

On Sun, Oct 31, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 10/31/21 9:54 AM, Andy Shevchenko wrote:
> > On Sunday, October 31, 2021, Lars-Peter Clausen <lars@metafoo.de
> > <mailto:lars@metafoo.de>> wrote:

...

> >     -       if (trig->subirq_base) {
> >     +       if (trig->subirq_base > 0) {
> >
> > >= ?
>
> I don't know. 0 is not supposed to be a valid irq number. And we
> kzalloc() the struct, so if it hasn't been explicitly initialized we'd
> get 0.

But it will change the behaviour of the code.
>=0 is the opposite of replacing < 0.


> The way the code is at the moment we'd never end up here without calling
> irq_alloc_descs(), so it is either a valid irq or a negative error code
> and I can see why you might want to use >= for consistency and symmetry.

Right!

(But on some architectures and cases 0 might be a valid vIRQ)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: viio_trigger_alloc(): Correctly free trigger on error

[Jonathan Cameron]

On Sun, 31 Oct 2021 15:00:38 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Oct 31, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> > On 10/31/21 9:54 AM, Andy Shevchenko wrote:  
> > > On Sunday, October 31, 2021, Lars-Peter Clausen <lars@metafoo.de
> > > <mailto:lars@metafoo.de>> wrote:  
> 
> ...
> 
> > >     -       if (trig->subirq_base) {
> > >     +       if (trig->subirq_base > 0) {
> > >  
> > > >= ?  
> >
> > I don't know. 0 is not supposed to be a valid irq number. And we
> > kzalloc() the struct, so if it hasn't been explicitly initialized we'd
> > get 0.  
> 
> But it will change the behaviour of the code.
> >=0 is the opposite of replacing < 0.  
> 
> 
> > The way the code is at the moment we'd never end up here without calling
> > irq_alloc_descs(), so it is either a valid irq or a negative error code
> > and I can see why you might want to use >= for consistency and symmetry.  
> 
> Right!
> 
> (But on some architectures and cases 0 might be a valid vIRQ)
> 
Given I'm fairly sure this will be after any other irqs we should be fine
but I don't think it would be a problem to allow 0.

If that's fine with both of you I can just change it to >= 0 whilst
applying, or Lars can do a v2 when has time.

Thanks,

Jonathan