iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
Subsystems (3)
PATCH 1/1 iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
The function returns a u8 so the -ENOMEM is truncated to a positive value. The caller tests for zero returns and treats them as an error so I have changed the -ENOMEM to 0.
drivers/iio/adc/at91_adc.c +1 -1
@@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, idev->id, triggers[i].name);if (!name) -return -ENOMEM ; +return 0 ;if (strcmp (trigger_name, name) =0 ) { value = triggers[i].value;
Am 11.06.2014 08:53, schrieb Dan Carpenter:
Quoted 11 lines
The function returns a u8 so the
-ENOMEMis truncated to a positive value. The caller tests for zero returns and treats them as an error so I have changed the-ENOMEMto 0.Signed-off-by Dan Carpenter <[email protected]>drivers/iio/adc/at91_adc.c +0
-0
drivers/iio/adc/at91_adc.c static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, · line 531
Quoted 8 lines
drivers/iio/adc/at91_adc.c +1
-1@@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, idev->id, triggers[i].name);if (!name) -return -ENOMEM ; +return 0 ;if (strcmp (trigger_name, name) =0 ) { value = triggers[i].value;
hi Dan,
LXR says there is only 1 caller (and he is checking for ENOMON) perhaps it is better to change the return value to int.
re,
wh
Quoted 20 lines · walter harms
Am 11.06.2014 08:53, schrieb Dan Carpenter:
Quoted 17 lines
The function returns a u8 so the
-ENOMEMis truncated to a positive value. The caller tests for zero returns and treats them as an error so I have changed the-ENOMEMto 0.Signed-off-by Dan Carpenter <[email protected]>drivers/iio/adc/at91_adc.c +1
-1@@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, idev->id, triggers[i].name);if (!name) -return -ENOMEM ; +return 0 ;
Quoted 6 lines
drivers/iio/adc/at91_adc.c static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, · line 531
Quoted 2 lines
drivers/iio/adc/at91_adc.c +0
-0if (strcmp (trigger_name, name) =0 ) { value = triggers[i].value;hi Dan,
LXR says there is only 1 caller (and he is checking for ENOMON)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No, it's not? It's checking for zero.
Quoted 1 line
perhaps it is better to change the return value to int.
In theory, it's always better to preserve the error code but in real life, this memory allocation is going to succeed. I'm not able to compile this code, and that change is quite a bit more involved, but I'll send it anyway.
regards,
dan carpenter
Am 11.06.2014 10:12, schrieb Dan Carpenter:
Quoted 21 lines · walter harms
Quoted 20 lines
Am 11.06.2014 08:53, schrieb Dan Carpenter:
Quoted 17 lines
The function returns a u8 so the
-ENOMEMis truncated to a positive value. The caller tests for zero returns and treats them as an error so I have changed the-ENOMEMto 0.Signed-off-by Dan Carpenter <[email protected]>drivers/iio/adc/at91_adc.c +1
-1@@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, idev->id, triggers[i].name);if (!name) -return -ENOMEM ; +return 0 ;
Quoted 9 lines
Quoted 6 lines
drivers/iio/adc/at91_adc.c static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev, · line 531
Quoted 2 lines
drivers/iio/adc/at91_adc.c +0
-0if (strcmp (trigger_name, name) =0 ) { value = triggers[i].value;hi Dan,
LXR says there is only 1 caller (and he is checking for ENOMON)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^No, it's not? It's checking for zero.
i as looking at at91_adc_configure_trigger() may it has changed lately
363
364 value = at91_adc_get_trigger_value_by_name(idev,
365 st->trigger_list,
366 idev->trig->name);
367 if (value = 0)
368 return -EINVAL;
Quoted 6 lines
Quoted 1 line
perhaps it is better to change the return value to int.
In theory, it's always better to preserve the error code but in real life, this memory allocation is going to succeed. I'm not able to compile this code, and that change is quite a bit more involved, but I'll send it anyway.
no problem with me, it was only a hint, maybe the maintainer will pick it up. In practice I found it is better to stay with int than to thing about "optimisation" and using char or friends, it does not save anything most times instead it has unintended consequences later.
re,
wh
Quoted 4 lines
regards,
dan carpenter
PATCH v2 1/1 iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
In reply to walter harmsat91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to a positive u8 and that doesn't work. I've changed it to int and refactored it to preserve the error code.
---
I'm not able to compile this code.
v2: Preserve the -ENOMEM
drivers/iio/adc/at91_adc.c +8 -8
@@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)return idev->num_channels; } -static u8 at91_adc_get_trigger_value_by_name (struct iio_dev *idev, +static int at91_adc_get_trigger_value_by_name (struct iio_dev *idev,struct at91_adc_trigger *triggers,const char *trigger_name) {struct at91_adc_state *st =iio_priv (idev); -u8 value =0 ;int i;for (i =0 ; i < st->trigger_number; i++) {
@@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,return -ENOMEM ;if (strcmp (trigger_name, name) =0 ) { - value = triggers[i].value;kfree (name); -break ; +if (triggers[i].value =0 ) +return -EINVAL ; +return triggers[i].value; }kfree (name); } -return value; +return -EINVAL ; }static int at91_adc_configure_trigger (struct iio_trigger *trig,bool state)
@@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)struct iio_buffer *buffer = idev->buffer;struct at91_adc_reg_desc *reg = st->registers;u32 status =at91_adc_readl (st, reg->trigger_register); -u8 value; +int value;u8 bit; value =at91_adc_get_trigger_value_by_name (idev, st->trigger_list, idev->trig->name); -if (value =0 ) -return -EINVAL ; +if (value <0 ) +return value;if (state) { st->buffer =kmalloc (idev->scan_bytes,GFP_KERNEL );
Quoted 5 lines · Dan Carpenter
at91_adc_get_trigger_value_by_name()was returning-ENOMEMtruncated to a positive u8 and that doesn't work. I've changed it to int and refactored it to preserve the error code.Signed-off-by Dan Carpenter <[email protected]>
Also, tested on an at91sam9g45ek.
Quoted 61 lines
---
I'm not able to compile this code.v2: Preserve the
-ENOMEMdrivers/iio/adc/at91_adc.c +8
-8@@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)return idev->num_channels; } -static u8 at91_adc_get_trigger_value_by_name (struct iio_dev *idev, +static int at91_adc_get_trigger_value_by_name (struct iio_dev *idev,struct at91_adc_trigger *triggers,const char *trigger_name) {struct at91_adc_state *st =iio_priv (idev); -u8 value =0 ;int i;for (i =0 ; i < st->trigger_number; i++) {@@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,return -ENOMEM ;if (strcmp (trigger_name, name) =0 ) { - value = triggers[i].value;kfree (name); -break ; +if (triggers[i].value =0 ) +return -EINVAL ; +return triggers[i].value; }kfree (name); } -return value; +return -EINVAL ; }static int at91_adc_configure_trigger (struct iio_trigger *trig,bool state)@@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)struct iio_buffer *buffer = idev->buffer;struct at91_adc_reg_desc *reg = st->registers;u32 status =at91_adc_readl (st, reg->trigger_register); -u8 value; +int value;u8 bit; value =at91_adc_get_trigger_value_by_name (idev, st->trigger_list, idev->trig->name); -if (value =0 ) -return -EINVAL ; +if (value <0 ) +return value;if (state) { st->buffer =kmalloc (idev->scan_bytes,GFP_KERNEL );
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Quoted 10 lines · Alexandre Belloni
Quoted 5 lines · Dan Carpenter
at91_adc_get_trigger_value_by_name()was returning-ENOMEMtruncated to a positive u8 and that doesn't work. I've changed it to int and refactored it to preserve the error code.Signed-off-by Dan Carpenter <[email protected]>Also, tested on an at91sam9g45ek.
I've added a tested-by as in many ways that has more weight than an Ack (and can sit alongside it just fine)
Dan, thanks as ever!
Applied to the fixes-togreg branch of iio.git and cc'd to stable.
Jonathan
Quoted 3 lines
Quoted 3 lines
---
I'm not able to compile this code.
Me neither - which makes that tested by rather handy ;)
Quoted 60 lines
Quoted 59 lines
v2: Preserve the
-ENOMEMdrivers/iio/adc/at91_adc.c +8
-8@@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)return idev->num_channels; } -static u8 at91_adc_get_trigger_value_by_name (struct iio_dev *idev, +static int at91_adc_get_trigger_value_by_name (struct iio_dev *idev,struct at91_adc_trigger *triggers,const char *trigger_name) {struct at91_adc_state *st =iio_priv (idev); -u8 value =0 ;int i;for (i =0 ; i < st->trigger_number; i++) {@@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,return -ENOMEM ;if (strcmp (trigger_name, name) ==0 ) { - value = triggers[i].value;kfree (name); -break ; +if (triggers[i].value ==0 ) +return -EINVAL ; +return triggers[i].value; }kfree (name); } -return value; +return -EINVAL ; }static int at91_adc_configure_trigger (struct iio_trigger *trig,bool state)@@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)struct iio_buffer *buffer = idev->buffer;struct at91_adc_reg_desc *reg = st->registers;u32 status =at91_adc_readl (st, reg->trigger_register); -u8 value; +int value;u8 bit; value =at91_adc_get_trigger_value_by_name (idev, st->trigger_list, idev->trig->name); -if (value ==0 ) -return -EINVAL ; +if (value <0 ) +return value;if (state) { st->buffer =kmalloc (idev->scan_bytes,GFP_KERNEL );