* [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
@ 2012-09-10 8:02 Kim, Milo
2012-09-10 9:16 ` Jonathan Cameron
2012-09-10 9:24 ` Lars-Peter Clausen
0 siblings, 2 replies; 6+ messages in thread
From: Kim, Milo @ 2012-09-10 8:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
The datasheet name is defined in the IIO driver.
On the other hand, the adc_channel_label is configured in
the platform machine side.
If the datasheet name is not matched with any adc_channel_label,
the iio_channel_get() should be returned as error for preventing
using invalid IIO channel data.
Moreover, this patch detects NULL pointer dereference problem at early time.
In general, the IIO driver just accesses to any member of the iio_chan_spec
in own xxx_read_raw() function.
If the iio_chan_spec is invalid pointer, NULL dereference problem may occur
such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
If the iio_channel_get() gets failed in the IIO consumer,
then no read_raw() operation proceeds.
Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
drivers/iio/inkern.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 1faa240..a5caf6b 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
channel->indio_dev = c->indio_dev;
- if (c->map->adc_channel_label)
+ if (c->map->adc_channel_label) {
channel->channel =
iio_chan_spec_from_name(channel->indio_dev,
c->map->adc_channel_label);
+ if (channel->channel == NULL)
+ return ERR_PTR(-ENODEV);
+ }
+
return channel;
}
EXPORT_SYMBOL_GPL(iio_channel_get);
--
1.7.9.5
Best Regards,
Milo
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
2012-09-10 8:02 [PATCH 2/2] iio: inkern: add error case in iio_channel_get() Kim, Milo
@ 2012-09-10 9:16 ` Jonathan Cameron
2012-09-14 0:59 ` Kim, Milo
2012-09-10 9:24 ` Lars-Peter Clausen
1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2012-09-10 9:16 UTC (permalink / raw)
To: Kim, Milo
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
On 10/09/12 09:02, Kim, Milo wrote:
> The datasheet name is defined in the IIO driver.
> On the other hand, the adc_channel_label is configured in
> the platform machine side.
> If the datasheet name is not matched with any adc_channel_label,
> the iio_channel_get() should be returned as error for preventing
> using invalid IIO channel data.
>
> Moreover, this patch detects NULL pointer dereference problem at early time.
> In general, the IIO driver just accesses to any member of the iio_chan_spec
> in own xxx_read_raw() function.
> If the iio_chan_spec is invalid pointer, NULL dereference problem may occur
> such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
> If the iio_channel_get() gets failed in the IIO consumer,
> then no read_raw() operation proceeds.
I guess this is marginally cleaner, but either way it is up to the
calling driver to check whether the call succeeded.
>
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
> drivers/iio/inkern.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 1faa240..a5caf6b 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>
> channel->indio_dev = c->indio_dev;
>
> - if (c->map->adc_channel_label)
> + if (c->map->adc_channel_label) {
> channel->channel =
> iio_chan_spec_from_name(channel->indio_dev,
> c->map->adc_channel_label);
>
> + if (channel->channel == NULL)
> + return ERR_PTR(-ENODEV);
> + }
> +
> return channel;
> }
> EXPORT_SYMBOL_GPL(iio_channel_get);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
2012-09-10 9:16 ` Jonathan Cameron
@ 2012-09-14 0:59 ` Kim, Milo
2012-09-14 8:24 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Kim, Milo @ 2012-09-14 0:59 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
> On 10/09/12 09:02, Kim, Milo wrote:
> > The datasheet name is defined in the IIO driver.
> > On the other hand, the adc_channel_label is configured in
> > the platform machine side.
> > If the datasheet name is not matched with any adc_channel_label,
> > the iio_channel_get() should be returned as error for preventing
> > using invalid IIO channel data.
> >
> > Moreover, this patch detects NULL pointer dereference problem at
> early time.
> > In general, the IIO driver just accesses to any member of the
> iio_chan_spec
> > in own xxx_read_raw() function.
> > If the iio_chan_spec is invalid pointer, NULL dereference problem
> may occur
> > such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
> > If the iio_channel_get() gets failed in the IIO consumer,
> > then no read_raw() operation proceeds.
> I guess this is marginally cleaner, but either way it is up to the
> calling driver to check whether the call succeeded.
IMO, it would be better if iio_channel_get() gets failed
when the consumer tries to get the channel rather than when using it.
The channel spec is retrieved when the channel is requested, - iio_channel_get()
So it's reasonable if error returns in case of invalid channel spcec.
And I just found similar handling in iio_channel_get_all().
But my patch needs releasing the memory as Lars-Peter noted.
Thanks !
Best Regards,
Milo
> >
> > Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> > ---
> > drivers/iio/inkern.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 1faa240..a5caf6b 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char
> *name, const char *channel_name)
> >
> > channel->indio_dev = c->indio_dev;
> >
> > - if (c->map->adc_channel_label)
> > + if (c->map->adc_channel_label) {
> > channel->channel =
> > iio_chan_spec_from_name(channel->indio_dev,
> > c->map->adc_channel_label);
> >
> > + if (channel->channel == NULL)
> > + return ERR_PTR(-ENODEV);
> > + }
> > +
> > return channel;
> > }
> > EXPORT_SYMBOL_GPL(iio_channel_get);
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
2012-09-14 0:59 ` Kim, Milo
@ 2012-09-14 8:24 ` Jonathan Cameron
2012-09-17 8:46 ` Kim, Milo
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2012-09-14 8:24 UTC (permalink / raw)
To: Kim, Milo
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
On 14/09/12 01:59, Kim, Milo wrote:
>> On 10/09/12 09:02, Kim, Milo wrote:
>>> The datasheet name is defined in the IIO driver.
>>> On the other hand, the adc_channel_label is configured in
>>> the platform machine side.
>>> If the datasheet name is not matched with any adc_channel_label,
>>> the iio_channel_get() should be returned as error for preventing
>>> using invalid IIO channel data.
>>>
>>> Moreover, this patch detects NULL pointer dereference problem at
>> early time.
>>> In general, the IIO driver just accesses to any member of the
>> iio_chan_spec
>>> in own xxx_read_raw() function.
>>> If the iio_chan_spec is invalid pointer, NULL dereference problem
>> may occur
>>> such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
>>> If the iio_channel_get() gets failed in the IIO consumer,
>>> then no read_raw() operation proceeds.
>
>> I guess this is marginally cleaner, but either way it is up to the
>> calling driver to check whether the call succeeded.
>
> IMO, it would be better if iio_channel_get() gets failed
> when the consumer tries to get the channel rather than when using it.
>
> The channel spec is retrieved when the channel is requested, - iio_channel_get()
> So it's reasonable if error returns in case of invalid channel spcec.
>
> And I just found similar handling in iio_channel_get_all().
> But my patch needs releasing the memory as Lars-Peter noted.
Fine, I'm convinced. Certainly less weird doing it your way ;)
Please do fix up the get all case as well.
>
> Thanks !
>
> Best Regards,
> Milo
>
>>>
>>> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
>>> ---
>>> drivers/iio/inkern.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index 1faa240..a5caf6b 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char
>> *name, const char *channel_name)
>>>
>>> channel->indio_dev = c->indio_dev;
>>>
>>> - if (c->map->adc_channel_label)
>>> + if (c->map->adc_channel_label) {
>>> channel->channel =
>>> iio_chan_spec_from_name(channel->indio_dev,
>>> c->map->adc_channel_label);
>>>
>>> + if (channel->channel == NULL)
>>> + return ERR_PTR(-ENODEV);
>>> + }
>>> +
>>> return channel;
>>> }
>>> EXPORT_SYMBOL_GPL(iio_channel_get);
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
2012-09-14 8:24 ` Jonathan Cameron
@ 2012-09-17 8:46 ` Kim, Milo
0 siblings, 0 replies; 6+ messages in thread
From: Kim, Milo @ 2012-09-17 8:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
> Fine, I'm convinced. Certainly less weird doing it your way ;)
> Please do fix up the get all case as well.
Please refer to chained patches.
[PATCH 1/2] iio: inkern: add error case in iio_channel_get()
[PATCH 2/2] iio: inkern: put the IIO device when mem alloc gets failed
I'd like to mention that these patches are dependent on another fix as below.
[PATCH RESEND] iio: inkern: allocate zeroed memory
Please let me know if any conflicts.
Thanks !
Best Regards,
Milo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()
2012-09-10 8:02 [PATCH 2/2] iio: inkern: add error case in iio_channel_get() Kim, Milo
2012-09-10 9:16 ` Jonathan Cameron
@ 2012-09-10 9:24 ` Lars-Peter Clausen
1 sibling, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2012-09-10 9:24 UTC (permalink / raw)
To: Kim, Milo
Cc: Jonathan Cameron, Jonathan Cameron, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
On 09/10/2012 10:02 AM, Kim, Milo wrote:
> The datasheet name is defined in the IIO driver.
> On the other hand, the adc_channel_label is configured in
> the platform machine side.
> If the datasheet name is not matched with any adc_channel_label,
> the iio_channel_get() should be returned as error for preventing
> using invalid IIO channel data.
>
> Moreover, this patch detects NULL pointer dereference problem at early time.
> In general, the IIO driver just accesses to any member of the iio_chan_spec
> in own xxx_read_raw() function.
> If the iio_chan_spec is invalid pointer, NULL dereference problem may occur
> such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
> If the iio_channel_get() gets failed in the IIO consumer,
> then no read_raw() operation proceeds.
>
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
> drivers/iio/inkern.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 1faa240..a5caf6b 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>
> channel->indio_dev = c->indio_dev;
>
> - if (c->map->adc_channel_label)
> + if (c->map->adc_channel_label) {
> channel->channel =
> iio_chan_spec_from_name(channel->indio_dev,
> c->map->adc_channel_label);
>
> + if (channel->channel == NULL)
> + return ERR_PTR(-ENODEV);
This introduces a memory leak. You need to free channel before returning.
> + }
> +
> return channel;
> }
> EXPORT_SYMBOL_GPL(iio_channel_get);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-17 8:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-10 8:02 [PATCH 2/2] iio: inkern: add error case in iio_channel_get() Kim, Milo
2012-09-10 9:16 ` Jonathan Cameron
2012-09-14 0:59 ` Kim, Milo
2012-09-14 8:24 ` Jonathan Cameron
2012-09-17 8:46 ` Kim, Milo
2012-09-10 9:24 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox