linux-kernel - Re: [PATCH 04/12] iio: adc: max9611: Fix misuse of GENMASK macro

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Jul 2019 23:52:14 +0200
From:   Jacopo Mondi <jacopo@...ndi.org>
To:     Joe Perches <joe@...ches.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 04/12] iio: adc: max9611: Fix misuse of GENMASK macro

Hello,
  so I finally run some test and...

On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote:
> On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:
> > On Tue,  9 Jul 2019 22:04:17 -0700
> > Joe Perches <joe@...ches.com> wrote:
> >
> > > Arguments are supposed to be ordered high then low.
> > >
> > > Signed-off-by: Joe Perches <joe@...ches.com>
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable etc.

I don't see it in v5.3-rc2, has it been collected or are we still in
time for an additional fix?

>
> This mask is used in an init function called from a probe.
>
> I don't have this hardware but it looks as if it could
> never have worked so I doubt the driver and the hardware
> have ever been tested.
>
> Does anyone have this device in actual use?

Because it turns out this is 2 times embarrassing. The mask definition
is indeed wrong, as Joe reported and fixed, but also this line
>
> 	regval = ret & MAX9611_TEMP_MASK;

is very wrong as regval is read as:
        ret = max9611_read_single(max9611, CONF_TEMP, &regval);

So that should actually be:
        regval &= MAX9611_TEMP_MASK;
not
 	regval = ret & MAX9611_TEMP_MASK;
Ups...

Yes, it worked by chance, as regval was always 0, which is in the
range of acceptable temperatures :/

>
> 	if ((regval > MAX9611_TEMP_MAX_POS &&
> 	     regval < MAX9611_TEMP_MIN_NEG) ||
> 	     regval > MAX9611_TEMP_MAX_NEG) {

Also reading this condition and how I had defined the temperature
calculation formula makes me wonder if this it totally correct, but
for the moment:

1) if Joe's patch has been collected, I can send an additional patch to
fix how regval is computed.
2) If Joe's patch still have to be collected, the regval computation
might be fixed there.

Sorry for taking so long to get back to you and thanks for noticing.

Thanks
  j

> 		dev_err(max9611->dev,
> 			"Invalid value received from ADC 0x%4x: aborting\n",
> 			regval);
> 		return -EIO;
> 	}
>
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/adc/max9611.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > index 917223d5ff5b..0e3c6529fc4c 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -83,7 +83,7 @@
> > >  #define MAX9611_TEMP_MAX_POS		0x7f80
> > >  #define MAX9611_TEMP_MAX_NEG		0xff80
> > >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> > >  #define MAX9611_TEMP_SHIFT		0x07
> > >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> > >  #define MAX9611_TEMP_SCALE_NUM		1000000
>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)