Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 1/3] iio: mxs-lradc: fix buffer overflow

1 view
Skip to first unread message

Alexandre Belloni

unread,
Jan 13, 2014, 11:10:05 AM1/13/14
to
Fixes:
drivers/staging/iio/adc/mxs-lradc.c:1556 mxs_lradc_probe() error: buffer
overflow 'iio->channels' 15 <= 15

The reported available scales for in_voltage15 were also wrong.

The realbits lookup is not necessary as all the channels of the LRADC have the
same resolution, use LRADC_RESOLUTION instead.

Reported-by: Dan Carpenter <dan.ca...@oracle.com>
Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
---
drivers/staging/iio/adc/mxs-lradc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index df71669bb60e..aa86849daeba 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -1613,7 +1613,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
* of the array.
*/
scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
- (iio->channels[i].scan_type.realbits - s);
+ (LRADC_RESOLUTION - s);
lradc->scale_avail[i][s].nano =
do_div(scale_uv, 100000000) * 10;
lradc->scale_avail[i][s].integer = scale_uv;
--
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alexandre Belloni

unread,
Jan 13, 2014, 12:00:02 PM1/13/14
to
Here are a few fixes to get in before 3.14.

Two of those are because I wasn't careful engouh when reordering my patch before
thos of Hector, it made me drop a part. and I forgot to reintroduce it when
rebasing.

The first patch fixes a buffer overflow found by smatch.

The second patch doesn't really have any impact as we control the input but is
there for correctness.

The last one removes useless scale_available files for channel 8 and 9 as they
are read has a temperature channel. I feel it is safe to do as no releases were
made with those so we are not really breaking the ABI.

Alexandre Belloni (3):
iio: mxs-lradc: fix buffer overflow
iio: mxs-lradc: fix invalid channel number detection
iio: mxs-lradc: remove useless scale_available files

drivers/staging/iio/adc/mxs-lradc.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

Alexandre Belloni

unread,
Jan 13, 2014, 12:00:04 PM1/13/14
to
16 would be accepted as a channel number but it is invalid. It doesn't really
have any effect as mxs_lradc_read_raw is called from a "controlled" environment
so it it only gets values going from 0 to 15.

Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
---
drivers/staging/iio/adc/mxs-lradc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index aa86849daeba..2289dc1bd928 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -898,7 +898,7 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
struct mxs_lradc *lradc = iio_priv(iio_dev);

/* Check for invalid channel */
- if (chan->channel > LRADC_MAX_TOTAL_CHANS)
+ if (chan->channel >= LRADC_MAX_TOTAL_CHANS)
return -EINVAL;

switch (m) {

Alexandre Belloni

unread,
Jan 13, 2014, 12:00:05 PM1/13/14
to
in_voltage8_scale_available and in_voltage9_scale_available are exposed to
userspace but useless as in_voltage8_raw and in_voltage9_raw are not available.

Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
---
drivers/staging/iio/adc/mxs-lradc.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 2289dc1bd928..cfd95d825702 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -1035,8 +1035,6 @@ SHOW_SCALE_AVAILABLE_ATTR(4);
SHOW_SCALE_AVAILABLE_ATTR(5);
SHOW_SCALE_AVAILABLE_ATTR(6);
SHOW_SCALE_AVAILABLE_ATTR(7);
-SHOW_SCALE_AVAILABLE_ATTR(8);
-SHOW_SCALE_AVAILABLE_ATTR(9);
SHOW_SCALE_AVAILABLE_ATTR(10);
SHOW_SCALE_AVAILABLE_ATTR(11);
SHOW_SCALE_AVAILABLE_ATTR(12);
@@ -1053,8 +1051,6 @@ static struct attribute *mxs_lradc_attributes[] = {
&iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
&iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
&iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
- &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
- &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
&iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
&iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
&iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,

Marek Vasut

unread,
Jan 13, 2014, 3:40:02 PM1/13/14
to
On Monday, January 13, 2014 at 05:02:02 PM, Alexandre Belloni wrote:
> 16 would be accepted as a channel number but it is invalid. It doesn't
> really have any effect as mxs_lradc_read_raw is called from a "controlled"
> environment so it it only gets values going from 0 to 15.
>
> Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>

Why don't you remove the check entirely then ?

Best regards,
Marek Vasut

Alexandre Belloni

unread,
Jan 13, 2014, 6:50:02 PM1/13/14
to
On 13/01/2014 21:25, Marek Vasut wrote:
> On Monday, January 13, 2014 at 05:02:02 PM, Alexandre Belloni wrote:
>> 16 would be accepted as a channel number but it is invalid. It doesn't
>> really have any effect as mxs_lradc_read_raw is called from a "controlled"
>> environment so it it only gets values going from 0 to 15.
>>
>> Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
>
> Why don't you remove the check entirely then ?
>

I'm not quite sure the inkernel API is sanitizing the input correctly
but maybe I didn't check enough. Maybe Jonathan can comment ?

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Jonathan Cameron

unread,
Jan 14, 2014, 2:00:01 PM1/14/14
to


On 13/01/14 23:43, Alexandre Belloni wrote:
> On 13/01/2014 21:25, Marek Vasut wrote:
>> On Monday, January 13, 2014 at 05:02:02 PM, Alexandre Belloni wrote:
>>> 16 would be accepted as a channel number but it is invalid. It doesn't
>>> really have any effect as mxs_lradc_read_raw is called from a "controlled"
>>> environment so it it only gets values going from 0 to 15.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
>>
>> Why don't you remove the check entirely then ?
>>
>
> I'm not quite sure the inkernel API is sanitizing the input correctly
> but maybe I didn't check enough. Maybe Jonathan can comment ?
>
Unless we have a bug (more than possible as this stuff isn't heavily used
yet), it should be impossible to get the required reference to a channel
that doesn't exist. Thus I don't 'think' the check is needed. Feel free
to write a test case to prove me wrong ;)

Jonathan

Jonathan Cameron

unread,
Jan 18, 2014, 6:40:01 AM1/18/14
to


On 13/01/14 16:02, Alexandre Belloni wrote:
> in_voltage8_scale_available and in_voltage9_scale_available are exposed to
> userspace but useless as in_voltage8_raw and in_voltage9_raw are not available.
>
> Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
Applied to the fixes-for-3.14new branch of iio.git

Thanks,

Jonathan Cameron

unread,
Jan 18, 2014, 6:40:02 AM1/18/14
to
On 13/01/14 16:02, Alexandre Belloni wrote:
> Fixes:
> drivers/staging/iio/adc/mxs-lradc.c:1556 mxs_lradc_probe() error: buffer
> overflow 'iio->channels' 15 <= 15
>
> The reported available scales for in_voltage15 were also wrong.
>
> The realbits lookup is not necessary as all the channels of the LRADC have the
> same resolution, use LRADC_RESOLUTION instead.
>
> Reported-by: Dan Carpenter <dan.ca...@oracle.com>
> Signed-off-by: Alexandre Belloni <alexandr...@free-electrons.com>
Applied to the fixes-for-3.14new branch of iio.git. This will go upstream
after 3.14-rc1 is tagged.

It's a little clunky having a simple array some of which isn't used for these
but I guess it does give fairly simple code.

Thanks,
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index df71669bb60e..aa86849daeba 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -1613,7 +1613,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
> * of the array.
> */
> scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
> - (iio->channels[i].scan_type.realbits - s);
> + (LRADC_RESOLUTION - s);
> lradc->scale_avail[i][s].nano =
> do_div(scale_uv, 100000000) * 10;
> lradc->scale_avail[i][s].integer = scale_uv;
>
--
0 new messages