[PATCH] cxd2820r: fix possible out-of-array lookup

[PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

In case of i2c write operation there is only one element in msg[] array.
Don't access msg[1] in that case.

Signed-off-by: Honza Petrous <jpetrous@smartimp.cz>

--

diff -uBbp cxd2820r_core.c.orig cxd2820r_core.c
--- cxd2820r_core.c.orig	2011-07-22 23:31:56.319168405 +0200
+++ cxd2820r_core.c	2011-07-22 23:35:02.508046078 +0200
@@ -750,8 +750,6 @@ static int cxd2820r_tuner_i2c_xfer(struc
 		}, {
 			.addr = priv->cfg.i2c_address,
 			.flags = I2C_M_RD,
-			.len = msg[1].len,
-			.buf = msg[1].buf,
 		}
 	};

@@ -760,6 +758,8 @@ static int cxd2820r_tuner_i2c_xfer(struc
 	if (num == 2) { /* I2C read */
 		obuf[1] = (msg[0].addr << 1) | I2C_M_RD; /* I2C RD flag */
 		msg2[0].len = sizeof(obuf) - 1; /* maybe HW bug ? */
+		msg2[1].len = msg[1].len;
+		msg2[1].buf = msg[1].buf;
 	}
 	memcpy(&obuf[2], msg[0].buf, msg[0].len);

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[Antti Palosaari]

On 07/23/2011 01:18 AM, HoP wrote:
> In case of i2c write operation there is only one element in msg[] array.
> Don't access msg[1] in that case.

NACK.
I suspect you confuse now local msg2 and msg that is passed as function 
parameter. Could you double check and explain?


regards
Antti

>
> Signed-off-by: Honza Petrous<jpetrous@smartimp.cz>
>
> --
>
> diff -uBbp cxd2820r_core.c.orig cxd2820r_core.c
> --- cxd2820r_core.c.orig	2011-07-22 23:31:56.319168405 +0200
> +++ cxd2820r_core.c	2011-07-22 23:35:02.508046078 +0200
> @@ -750,8 +750,6 @@ static int cxd2820r_tuner_i2c_xfer(struc
>   		}, {
>   			.addr = priv->cfg.i2c_address,
>   			.flags = I2C_M_RD,
> -			.len = msg[1].len,
> -			.buf = msg[1].buf,
>   		}
>   	};
>
> @@ -760,6 +758,8 @@ static int cxd2820r_tuner_i2c_xfer(struc
>   	if (num == 2) { /* I2C read */
>   		obuf[1] = (msg[0].addr<<  1) | I2C_M_RD; /* I2C RD flag */
>   		msg2[0].len = sizeof(obuf) - 1; /* maybe HW bug ? */
> +		msg2[1].len = msg[1].len;
> +		msg2[1].buf = msg[1].buf;
>   	}
>   	memcpy(&obuf[2], msg[0].buf, msg[0].len);


-- 
http://palosaari.fi/

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

2011/7/23 Antti Palosaari <crope@iki.fi>:
> On 07/23/2011 01:18 AM, HoP wrote:
>>
>> In case of i2c write operation there is only one element in msg[] array.
>> Don't access msg[1] in that case.
>
> NACK.
> I suspect you confuse now local msg2 and msg that is passed as function
> parameter. Could you double check and explain?
>

Ok, may I really understand it badly.

My intention was that in case of tda18271_write_regs() there is
i2c_transfer() called with msg[] array of one element only.
So am I wrong?

Thanks

Honza

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[Antti Palosaari]

On 07/23/2011 01:47 AM, HoP wrote:
> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>> On 07/23/2011 01:18 AM, HoP wrote:
>>>
>>> In case of i2c write operation there is only one element in msg[] array.
>>> Don't access msg[1] in that case.
>>
>> NACK.
>> I suspect you confuse now local msg2 and msg that is passed as function
>> parameter. Could you double check and explain?
>>
>
> Ok, may I really understand it badly.
>
> My intention was that in case of tda18271_write_regs() there is
> i2c_transfer() called with msg[] array of one element only.
> So am I wrong?

No. There is only one msg array in write and in case of reg read there 
is two elements, first one is write and second is read.

But now I see what you mean. msg2[1] is set as garbage fields in case of 
incoming msg len is 1. True, but it does not harm since it is not used 
in that case.

The idea of whole system is just add 2 bytes to incoming msg .buf and 
then forward that message.

regards
Antti

>
> Thanks
>
> Honza


-- 
http://palosaari.fi/

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

2011/7/23 Antti Palosaari <crope@iki.fi>:
> On 07/23/2011 01:47 AM, HoP wrote:
>>
>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>>
>>> On 07/23/2011 01:18 AM, HoP wrote:
>>>>
>>>> In case of i2c write operation there is only one element in msg[] array.
>>>> Don't access msg[1] in that case.
>>>
>>> NACK.
>>> I suspect you confuse now local msg2 and msg that is passed as function
>>> parameter. Could you double check and explain?
>>>
>>
>> Ok, may I really understand it badly.
>>
>> My intention was that in case of tda18271_write_regs() there is
>> i2c_transfer() called with msg[] array of one element only.
>> So am I wrong?
>
> No. There is only one msg array in write and in case of reg read there is
> two elements, first one is write and second is read.
>
> But now I see what you mean. msg2[1] is set as garbage fields in case of
> incoming msg len is 1. True, but it does not harm since it is not used in
> that case.

In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
with only one element, true? If so, then my patch is correct.

>
> The idea of whole system is just add 2 bytes to incoming msg .buf and then
> forward that message.
>

Yes. I just learnt it, very clever way. What I only don't understand is
why do you decrease msg2[0].len. Seems really like some hw bug.

Honza

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[Antti Palosaari]

On 07/23/2011 02:01 AM, HoP wrote:
> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>> On 07/23/2011 01:47 AM, HoP wrote:
>>>
>>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>>>
>>>> On 07/23/2011 01:18 AM, HoP wrote:
>>>>>
>>>>> In case of i2c write operation there is only one element in msg[] array.
>>>>> Don't access msg[1] in that case.
>>>>
>>>> NACK.
>>>> I suspect you confuse now local msg2 and msg that is passed as function
>>>> parameter. Could you double check and explain?
>>>>
>>>
>>> Ok, may I really understand it badly.
>>>
>>> My intention was that in case of tda18271_write_regs() there is
>>> i2c_transfer() called with msg[] array of one element only.
>>> So am I wrong?
>>
>> No. There is only one msg array in write and in case of reg read there is
>> two elements, first one is write and second is read.
>>
>> But now I see what you mean. msg2[1] is set as garbage fields in case of
>> incoming msg len is 1. True, but it does not harm since it is not used in
>> that case.
>
> In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
> with only one element, true? If so, then my patch is correct.

Yes it is true but nonsense. It is also wrong to make always msg2 as two 
element array too, but those are just simpler and generates most likely 
some code less. Could you see it can cause problem in some case?

If you want to fix that then please make it general enough to work for 
example when 3 or 4 messages are send in one I2C transaction (also 
rather nonsense since I don't know any driver using more than 2 msgs in 
I2C xfer).

And one point more for I2C implementations, i2c_transfer() should 
implement repeated start sequence to messages given. But I am almost 
sure there is rather none I2C adapter HW which can do that really. Whole 
i2c_transfer() usage is for read operation. Logically i2c_master_send() 
and i2c_master_recv() should be used instead since no repeated start but 
that's not possible (in normal two msg read) without caching writes in 
adapter.

>> The idea of whole system is just add 2 bytes to incoming msg .buf and then
>> forward that message.
>>
>
> Yes. I just learnt it, very clever way. What I only don't understand is
> why do you decrease msg2[0].len. Seems really like some hw bug.

NXP tuner driver used does not write any payload bytes to I2C, only 
device address, and that's upset I2C adapter. Many devices using NXP 
tuners just write as register 0 (have seen from sniffs) when reading to 
avoid that. For TDA18218 driver I added such functionality to the tuner 
driver since it should not make harm.

For short, you don't need to write register to read for NXP tuner, it is 
always starting from reg 0.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[Antti Palosaari]

On 07/23/2011 02:31 AM, Antti Palosaari wrote:
> On 07/23/2011 02:01 AM, HoP wrote:
>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>> But now I see what you mean. msg2[1] is set as garbage fields in case of
>>> incoming msg len is 1. True, but it does not harm since it is not
>>> used in
>>> that case.
>>
>> In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
>> with only one element, true? If so, then my patch is correct.
>
> Yes it is true but nonsense. It is also wrong to make always msg2 as two
> element array too, but those are just simpler and generates most likely
> some code less. Could you see it can cause problem in some case?

Now I thought it more, could it crash if it point out of memory area?


regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

2011/7/23 Antti Palosaari <crope@iki.fi>:
> On 07/23/2011 02:31 AM, Antti Palosaari wrote:
>>
>> On 07/23/2011 02:01 AM, HoP wrote:
>>>
>>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>>>
>>>> But now I see what you mean. msg2[1] is set as garbage fields in case of
>>>> incoming msg len is 1. True, but it does not harm since it is not
>>>> used in
>>>> that case.
>>>
>>> In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
>>> with only one element, true? If so, then my patch is correct.
>>
>> Yes it is true but nonsense. It is also wrong to make always msg2 as two
>> element array too, but those are just simpler and generates most likely
>> some code less. Could you see it can cause problem in some case?
>
> Now I thought it more, could it crash if it point out of memory area?

I see you finally understood what I wanted to do :-)

I'm surprised that it not crashed already. I thought I have to missed something.

Honza

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[Malcolm Priestley]

On Sat, 2011-07-23 at 01:47 +0200, HoP wrote:
> 2011/7/23 Antti Palosaari <crope@iki.fi>:
> > On 07/23/2011 02:31 AM, Antti Palosaari wrote:
> >>
> >> On 07/23/2011 02:01 AM, HoP wrote:
> >>>
> >>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
> >>>>
> >>>> But now I see what you mean. msg2[1] is set as garbage fields in case of
> >>>> incoming msg len is 1. True, but it does not harm since it is not
> >>>> used in
> >>>> that case.
> >>>
> >>> In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
> >>> with only one element, true? If so, then my patch is correct.
> >>
> >> Yes it is true but nonsense. It is also wrong to make always msg2 as two
> >> element array too, but those are just simpler and generates most likely
> >> some code less. Could you see it can cause problem in some case?
> >
> > Now I thought it more, could it crash if it point out of memory area?
Arrays are not fussy they will read anything, just don't poke them :-)
> 
> I see you finally understood what I wanted to do :-)
> 
> I'm surprised that it not crashed already. I thought I have to missed something.

It does not crash because num is constant throughout, when the number of
messages is one the second element isn't transferred.

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

2011/7/23 Malcolm Priestley <tvboxspy@gmail.com>:
> On Sat, 2011-07-23 at 01:47 +0200, HoP wrote:
>> 2011/7/23 Antti Palosaari <crope@iki.fi>:
>> > On 07/23/2011 02:31 AM, Antti Palosaari wrote:
>> >>
>> >> On 07/23/2011 02:01 AM, HoP wrote:
>> >>>
>> >>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>> >>>>
>> >>>> But now I see what you mean. msg2[1] is set as garbage fields in case of
>> >>>> incoming msg len is 1. True, but it does not harm since it is not
>> >>>> used in
>> >>>> that case.
>> >>>
>> >>> In case of write, cxd2820r_tuner_i2c_xfer() gets msg[] parameter
>> >>> with only one element, true? If so, then my patch is correct.
>> >>
>> >> Yes it is true but nonsense. It is also wrong to make always msg2 as two
>> >> element array too, but those are just simpler and generates most likely
>> >> some code less. Could you see it can cause problem in some case?
>> >
>> > Now I thought it more, could it crash if it point out of memory area?
> Arrays are not fussy they will read anything, just don't poke them :-)

Are you sure about not crashing? On every architecture on which linux
can run?

Even if it not crash kernel, I hope we can agree that it is incorrect
and need to be fixed.

>>
>> I see you finally understood what I wanted to do :-)
>>
>> I'm surprised that it not crashed already. I thought I have to missed something.
>
> It does not crash because num is constant throughout, when the number of
> messages is one the second element isn't transferred.

Sure, that is evident. My fix was about not do read access outside
of input array msg[]. I still don't understand the NACK.

/Honza

Re: [PATCH] cxd2820r: fix possible out-of-array lookup

[HoP]

Hi Antti

2011/7/23 Antti Palosaari <crope@iki.fi>:
> On 07/23/2011 02:01 AM, HoP wrote:
>>
>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>>
>>> On 07/23/2011 01:47 AM, HoP wrote:
>>>>
>>>> 2011/7/23 Antti Palosaari<crope@iki.fi>:
>>>>>
>>>>> On 07/23/2011 01:18 AM, HoP wrote:
>>>>>>
>>>>>> In case of i2c write operation there is only one element in msg[]
>>>>>> array.
>>>>>> Don't access msg[1] in that case.
>>>>>
>>>>> NACK.
>>>>> I suspect you confuse now local msg2 and msg that is passed as function
>>>>> parameter. Could you double check and explain?

Can you confirm your NACK?

As I wrote before, my patch was about read access out of msg[] array
parameter of function cxd2820r_tuner_i2c_xfer() in case when msg[]
array has only one element (what should be case when using
tda18271_write_regs() for example). Or am I still missed something?

[snip]

> And one point more for I2C implementations, i2c_transfer() should implement
> repeated start sequence to messages given. But I am almost sure there is
> rather none I2C adapter HW which can do that really. Whole i2c_transfer()

Strange enought. Or may better say that linux/i2c.h must fool if you are right,
because there you can read:

--- linux/i2c.h ---
 * An i2c_msg is the low level representation of one segment of an I2C
 * transaction.  It is visible to drivers in the @i2c_transfer() procedure,
 * to userspace from i2c-dev, and to I2C adapter drivers through the
 * @i2c_adapter.@master_xfer() method.
 *
 * Except when I2C "protocol mangling" is used, all I2C adapters implement
 * the standard rules for I2C transactions.  Each transaction begins with a
 * START.  That is followed by the slave address, and a bit encoding read
 * versus write.  Then follow all the data bytes, possibly including a byte
 * with SMBus PEC.  The transfer terminates with a NAK, or when all those
 * bytes have been transferred and ACKed.  If this is the last message in a
 * group, it is followed by a STOP.  Otherwise it is followed by the next
 * @i2c_msg transaction segment, beginning with a (repeated) START.
---

It says quite the reverse - all multimessage transfers have using
repeated START.
Very annoying. At least for kernel newbies.

Regards

Honza