linux-kernel - Re: iio:adc:ad7280a: Move out of staging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YiLLsQmnK+f6hjvi@marsc.168.1.7>
Date:   Fri, 4 Mar 2022 23:32:17 -0300
From:   Marcelo Schmitt <marcelo.schmitt1@...il.com>
To:     "Colin King (gmail)" <colin.i.king@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: iio:adc:ad7280a: Move out of staging

On 02/24, Colin King (gmail) wrote:
> Hi,
> 
> Static analysis with clang scan picked up a potential issue with
> drivers/iio/adc/ad7280a.c in function ad7280a_write_thresh, the analysis is
> as follows:
> 
>         switch (chan->type) {
>         case IIO_VOLTAGE:
>                 value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
>                 value = clamp(value, 0L, 0xFFL);
> 
>                 ^^
> Note: variable value is being assigned a value
> 
>                 switch (dir) {
>                 case IIO_EV_DIR_RISING:
>                         addr = AD7280A_CELL_OVERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->cell_threshhigh = value;
> 
> ..and value is being used here ^^
> 
>                         break;
>                 case IIO_EV_DIR_FALLING:
>                         addr = AD7280A_CELL_UNDERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->cell_threshlow = value;
> 
> and value is being used here ^^
> 
>                         break;
>                 default:
>                         ret = -EINVAL;
>                         goto err_unlock;
>                 }
>                 break;
> 
> However for the IIO_TEMP case:
> 
>         case IIO_TEMP:
>                 value = (val * 10) / 196; /* LSB 19.6mV */
>                 value = clamp(value, 0L, 0xFFL);
> 
>                 ^^
> Note: variable value is being assigned a value
> 
>                 switch (dir) {
>                 case IIO_EV_DIR_RISING:
>                         addr = AD7280A_AUX_ADC_OVERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->aux_threshhigh = val;
>                                              ^^
> But val is being used here rather than value
> 
>                         break;
>                 case IIO_EV_DIR_FALLING:
>                         addr = AD7280A_AUX_ADC_UNDERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->aux_threshlow = val;
>                                             ^^
> and val us being used here rather than value too
> 
> 
> So for the IIO_TEMP case either the assignment to value is redundant or the
> setting of st->aux_threshhigh or st->auxthreashlow is incorrect.

Good catch. This has really slipped through my review.
It's impressive how we can discuss changes at one specific part of the
code and miss inconsistencies a few lines below it.

Thanks,
Marcelo

> 
> Colin