re: STAGING:iio:light: fix ISL29018 init to handle brownout

re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Dan Carpenter]

Hi Grant,

There is a memory corruption bug in 176f9f29cec9 "STAGING:iio:light:
fix ISL29018 init to handle brownout".

In isl29018_chip_init() we call:
        status = isl29018_write_data(client, ISL29018_REG_TEST, 0,               
                                ISL29018_TEST_MASK, ISL29018_TEST_SHIFT);        

where ISL29018_REG_TEST is 8.

In isl29018_write_data() it uses reg (ISL29018_REG_TEST) as the
offset into the ->reg_cache[] array:
	chip->reg_cache[reg] = regval;

But ->reg_cache[] only has 3 elements, so we're past the end of the
array.

I don't know the code well enough to fix this.

regards,
dan carpenter

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Grant Grundler]

On Thu, Aug 25, 2011 at 6:15 PM, Dan Carpenter <error27@gmail.com> wrot=
e:
> Hi Grant,
>
> There is a memory corruption bug in 176f9f29cec9 "STAGING:iio:light:
> fix ISL29018 init to handle brownout".
>
> In isl29018_chip_init() we call:
> =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D isl29018_write_data(client, ISL=
29018_REG_TEST, 0,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ISL29018_TEST_MASK, ISL29018_=
TEST_SHIFT);
>
> where ISL29018_REG_TEST is 8.
>
> In isl29018_write_data() it uses reg (ISL29018_REG_TEST) as the
> offset into the ->reg_cache[] array:
> =C2=A0 =C2=A0 =C2=A0 =C2=A0chip->reg_cache[reg] =3D regval;
>
> But ->reg_cache[] only has 3 elements, so we're past the end of the
> array.

Wow! Thanks! I'll look at the code in the morning and suggest a fix.


> I don't know the code well enough to fix this.

No problem - I'm happy you spotted this.

My initial suggestion for a fix is to just not reference reg_cache if
"reg" exceeds the size of reg_cache. In other words, don't cache those
values. This should normally work well since we don't other touch that
register in the driver AFAICT.  But I'll review the code some more
tomorrow before submitting a fix.

cheers,
grant

>
> regards,
> dan carpenter
>

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On Thu, Aug 25, 2011 at 6:15 PM, Dan Carpenter <error27@gmail.com> wrote:
...
> In isl29018_write_data() it uses reg (ISL29018_REG_TEST) as the
> offset into the ->reg_cache[] array:
>        chip->reg_cache[reg] = regval;
>
> But ->reg_cache[] only has 3 elements, so we're past the end of the
> array.

Dan,
I've attached a preliminary patch to fix this that applies on top of
176f9f29cec9 "STAGING:iio:light:
fix ISL29018 init to handle brownout".  This patch applies cleanly to
the 2.6.38-based chromium.org tree.

In a nutshell, the attached patch implements what I was thinking of
last night: don't cache REG_TEST.

I did change one basic behavior that I think was also broken: cache
the value regardless of if the transaction completed successfully or
not. For registers where we are frobbing bits, the later write to the
same register  might succeed and things will work anyway despite the
transient failure. If I'm wrong, please comment and I'll repost with
original behavior.

I also noticed the original code had an "off-by-one" error in the
allocation of reg_cache[] (allocated 3 but indexed up to offset +3).

I'll submit a cleaned up version that should cleanly apply to gregkh's
staging-2.6 tree.

thanks again!
grant

[-- Attachment #2: 2.6.38-isl29018_reg_cache-01 --]
[-- Type: application/octet-stream, Size: 1801 bytes --]

Fix out-of-bounds reference to reg_cache[]

Simple fix is to just not cache REG_TEST (offset 8). It doesn't help anyway
since we write all 8 bits exactly once (at resume/init time).

Also fix an "off-by-one" allocation of reg_cache[] array size that
was in the original code before I touched it.

Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Grant Grundler <grundler@chromium.org>

----
Thanks again to Dan Carpenter for spotting the out-of-bounds array reference.

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 0f97734..a5259ff 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -51,7 +51,7 @@
 
 #define ISL29018_REG_ADD_DATA_LSB	0x02
 #define ISL29018_REG_ADD_DATA_MSB	0x03
-#define ISL29018_MAX_REGS		ISL29018_REG_ADD_DATA_MSB
+#define ISL29018_MAX_REGS		ISL29018_REG_ADD_DATA_MSB+1
 
 #define ISL29018_REG_TEST		0x08
 #define ISL29018_TEST_SHIFT		0
@@ -71,22 +71,23 @@ struct isl29018_chip {
 static int isl29018_write_data(struct i2c_client *client, u8 reg,
 			u8 val, u8 mask, u8 shift)
 {
-	u8 regval;
-	int ret = 0;
+	u8 regval = val;
+	int ret;
 	struct isl29018_chip *chip = i2c_get_clientdata(client);
 
-	regval = chip->reg_cache[reg];
-	regval &= ~mask;
-	regval |= val << shift;
+	/* don't cache REG_TEST */
+	if (reg < ISL29018_MAX_REGS) {
+		regval = chip->reg_cache[reg];
+		regval &= ~mask;
+		regval |= val << shift;
+		chip->reg_cache[reg] = regval;
+	}
 
 	ret = i2c_smbus_write_byte_data(client, reg, regval);
-	if (ret) {
+	if (ret)
 		dev_err(&client->dev, "Write to device fails status %x\n", ret);
-		return ret;
-	}
-	chip->reg_cache[reg] = regval;
 
-	return 0;
+	return ret;
 }
 
 static int isl29018_set_range(struct i2c_client *client, unsigned long range,

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Dan Carpenter]

It looks good to me, but I don't know anything about the iio code.
Jonathan will let you know if there is a problem.  :)

This is minor thing, but checkpatch complains that:
+#define ISL29018_MAX_REGS              ISL29018_REG_ADD_DATA_MSB+1
should be written as:
+#define ISL29018_MAX_REGS              (ISL29018_REG_ADD_DATA_MSB + 1)

Thanks Grant.

regards,
dan carpenter

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Grant Grundler]

On Fri, Aug 26, 2011 at 3:18 PM, Dan Carpenter <error27@gmail.com> wrot=
e:
> It looks good to me, but I don't know anything about the iio code.
> Jonathan will let you know if there is a problem. =C2=A0:)

I'm hoping so. :)  Thanks for reviewing.

>
> This is minor thing, but checkpatch complains that:
> +#define ISL29018_MAX_REGS =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0ISL29018_REG_ADD_DATA_MSB+1
> should be written as:
> +#define ISL29018_MAX_REGS =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0(ISL29018_REG_ADD_DATA_MSB + 1)

ah...I didn't run check_patch. /o\
I'll include that in the version I send to gregkh.

cheers,
grant

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Jonathan Cameron]

On 08/26/11 22:58, Grant Grundler wrote:
> On Thu, Aug 25, 2011 at 6:15 PM, Dan Carpenter <error27@gmail.com> wrote:
> ...
>> In isl29018_write_data() it uses reg (ISL29018_REG_TEST) as the
>> offset into the ->reg_cache[] array:
>>        chip->reg_cache[reg] = regval;
>>
>> But ->reg_cache[] only has 3 elements, so we're past the end of the
>> array.
> 
> Dan,
> I've attached a preliminary patch to fix this that applies on top of
> 176f9f29cec9 "STAGING:iio:light:
> fix ISL29018 init to handle brownout".  This patch applies cleanly to
> the 2.6.38-based chromium.org tree.
> 
> In a nutshell, the attached patch implements what I was thinking of
> last night: don't cache REG_TEST.
> 
> I did change one basic behavior that I think was also broken: cache
> the value regardless of if the transaction completed successfully or
> not. 
Don't do that.  That means userspace will get an invalid value if it reads
in the meantime. If you have an error on a hardware bus - tell userspace about
it and don't 'guess' what is in the register.

Otherwise patch looks fine to me.
>For registers where we are frobbing bits, the later write to the
> same register  might succeed and things will work anyway despite the
> transient failure. If I'm wrong, please comment and I'll repost with
> original behavior.
> 
> I also noticed the original code had an "off-by-one" error in the
> allocation of reg_cache[] (allocated 3 but indexed up to offset +3).
> 
> I'll submit a cleaned up version that should cleanly apply to gregkh's
> staging-2.6 tree.
> 
> thanks again!
> grant

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Grant Grundler]

On Tue, Aug 30, 2011 at 3:05 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
....
>> I did change one basic behavior that I think was also broken: cache
>> the value regardless of if the transaction completed successfully or
>> not.
> Don't do that. =C2=A0That means userspace will get an invalid value if it=
 reads
> in the meantime. If you have an error on a hardware bus - tell userspace =
about
> it and don't 'guess' what is in the register.

Ah ok - I didn't know this was part of the path to/from user space.

>
> Otherwise patch looks fine to me.

OK - I'll restore previous behavior and submit another patch.

cheers!
grant

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Jonathan Cameron]

On 08/30/11 17:14, Grant Grundler wrote:
> On Tue, Aug 30, 2011 at 3:05 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> ....
>>> I did change one basic behavior that I think was also broken: cache
>>> the value regardless of if the transaction completed successfully or
>>> not.
>> Don't do that.  That means userspace will get an invalid value if it reads
>> in the meantime. If you have an error on a hardware bus - tell userspace about
>> it and don't 'guess' what is in the register.
> 
> Ah ok - I didn't know this was part of the path to/from user space.
It plausibly isn't, I didn't go over it closely enough to be sure.
Even if not, it's a nasty complexity that will bite someone at some stage.
Mostly comes down to code doing what other code does in the same situation
rather than trying to be clever.


> 
>>
>> Otherwise patch looks fine to me.
> 
> OK - I'll restore previous behavior and submit another patch.
> 
> cheers!
> grant
> 

Re: STAGING:iio:light: fix ISL29018 init to handle brownout

[Grant Grundler]

On Tue, Aug 30, 2011 at 9:31 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 08/30/11 17:14, Grant Grundler wrote:
>> On Tue, Aug 30, 2011 at 3:05 AM, Jonathan Cameron <jic23@cam.ac.uk> wrot=
e:
>> ....
>>>> I did change one basic behavior that I think was also broken: cache
>>>> the value regardless of if the transaction completed successfully or
>>>> not.
>>> Don't do that. =C2=A0That means userspace will get an invalid value if =
it reads
>>> in the meantime. If you have an error on a hardware bus - tell userspac=
e about
>>> it and don't 'guess' what is in the register.
>>
>> Ah ok - I didn't know this was part of the path to/from user space.
> It plausibly isn't, I didn't go over it closely enough to be sure.

Ok. After thinking about it more, it might not matter anyway.

> Even if not, it's a nasty complexity that will bite someone at some stage=
.
> Mostly comes down to code doing what other code does in the same situatio=
n
> rather than trying to be clever.

The whole error handling path one ugly bit of "nasty complexity". If a regi=
ster
write fails, the register contents will be unknown. We will have no idea wh=
at
value is in the register or if the device is even still alive.
The cached value can't represent "known HW state" at that point and should
probably be marked "invalid" until either the value is succesfully
read or the next
write succeeds.

BTW, I didn't consider this particularly clever - just the code was
easier to read. :)

I don't think it really matters how the "cached value" is handled as
long as the error gets returned.  I'm just going to implement whatever
you prefer. :)

cheers,
grant