linux-kernel - Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 4 Nov 2019 08:50:33 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>
CC:     "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if
 operation succeeded

On Sun, 2019-11-03 at 10:41 +0000, Jonathan Cameron wrote:
> On Fri, 1 Nov 2019 11:35:03 +0200
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> 
> > This was found only after the whole thing with the inline functions,
> > but
> > the compiler actually found something. The value of the `bias` (in
> > adis16480_get_calibbias()) should only be set if the read operation was
> > successful.
> > 
> > Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and
> > similar IMUs")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> 
> Hmm. It's not really a fix as such unless there is an in kernel consumer
> that is using this and not checking the return value.   I thought about
> dropping
> the fixes tag, but it is avoiding confusing the compiler so I suppose we
> might
> as well leave it marked as a fix.  I will add a note that this doesn't
> cause
> any known real problems and so probably isn't stable material.

Thanks.
No idea if it's worth having a Fixes tag or not.
I thought about adding it, since it seemed like a-bit-of-a-fix, and wasn't
sure, so I added. I guess after upstreaming things a bit, I started
wondering about these tags a bit more.

Alex

> 
> Applied.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/imu/adis16480.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index b9e2695bcfad..c0e7e768be41 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev
> > *indio_dev,
> >  	case IIO_MAGN:
> >  	case IIO_PRESSURE:
> >  		ret = adis_read_reg_16(&st->adis, reg, &val16);
> > -		*bias = sign_extend32(val16, 15);
> > +		if (ret == 0)
> > +			*bias = sign_extend32(val16, 15);
> >  		break;
> >  	case IIO_ANGL_VEL:
> >  	case IIO_ACCEL:
> >  		ret = adis_read_reg_32(&st->adis, reg, &val32);
> > -		*bias = sign_extend32(val32, 31);
> > +		if (ret == 0)
> > +			*bias = sign_extend32(val32, 31);
> >  		break;
> >  	default:
> >  		ret = -EINVAL;