i2c interface bugs in dvb drivers

i2c interface bugs in dvb drivers

[matthieu castet]

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

Hi,

some dvb driver that export a i2c interface contains some mistakes because they mainly used by driver (frontend, tuner) wrote for them.
But the i2c interface is exposed to everybody.

One mistake is expect msg[i].addr with 8 bits address instead of 7 bits address. This make them use eeprom address at 0xa0 instead of 0x50. Also they shift tuner address (qt1010 tuner is likely to be at address 0x62, but some put it a 0xc4 (af9015, af9005, dtv5100)).

Other mistakes is in xfer callback. Often the controller support a limited i2c support (n bytes write then m bytes read). The driver try to convert the linux i2c msg to this pattern, but they often miss cases :
- msg[i].len can be null
- msg write are not always followed by msg read

And this can be dangerous if these interfaces are exported to userspace via i2c-dev :
- some scanning program avoid eeprom by filtering 0x5x range, but now it is at 0xax range (well that should happen because scan limit should be 0x77)
- some read only command can be interpreted as write command.


What should be done ?
Fix the drivers.
Have a mode where i2c interface are not exported to everybody.
Don't care.

First why does the i2c stack doesn't check that the address is on 7 bits (like the attached patch) ?

Also I believe a program for testing i2c interface corner case should catch most of these bugs :
- null msg[i].len
- different transactions on a device :
 - one write/read transaction
 - one write transaction then one read transaction
[...]

Does a such program exist ?


Matthieu

PS : please keep me in CC


[-- Attachment #2: i2c_check.diff --]
[-- Type: text/x-diff, Size: 701 bytes --]

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3202a86..91e63ea 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1150,6 +1150,17 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
 		}
 #endif
+		for (ret = 0; ret < num; ret++) {
+			if (msgs[ret].flags & I2C_M_TEN) {
+				/* XXX what"s I2C_M_TEN range */
+				if (msgs[ret].addr < 0x03 || msgs[ret].addr > 0x377)
+					return -EINVAL;
+			}
+			else {
+				if (msgs[ret].addr < 0x03 || msgs[ret].addr > 0x77)
+					return -EINVAL;
+			}
+		}
 
 		if (in_atomic() || irqs_disabled()) {
 			ret = rt_mutex_trylock(&adap->bus_lock);

Re: i2c interface bugs in dvb drivers

[Jean Delvare]

Hi Matthieu,

On Sun, 21 Mar 2010 15:02:50 +0100, matthieu castet wrote:
> some dvb driver that export a i2c interface contains some mistakes
> because they mainly used by driver (frontend, tuner) wrote for them.
> But the i2c interface is exposed to everybody.
> 
> One mistake is expect msg[i].addr with 8 bits address instead of 7
> bits address. This make them use eeprom address at 0xa0 instead of
> 0x50. Also they shift tuner address (qt1010 tuner is likely to be
> at address 0x62, but some put it a 0xc4 (af9015, af9005, dtv5100)).
> 
> Other mistakes is in xfer callback. Often the controller support a
> limited i2c support (n bytes write then m bytes read). The driver
> try to convert the linux i2c msg to this pattern, but they often
> miss cases :
> - msg[i].len can be null

Zero, not null. This is a very rare case, I've never seen it in
multi-message transactions (wouldn't make much sense IMHO), only in
the single-message transaction known as SMBus quick command. Many
controllers don't support it, so I2C bus drivers don't have to support
it. It should be assumed by I2C chip drivers that an I2C adapter
without functionality bit I2C_FUNC_SMBUS_QUICK does not support 0-byte
messages.

> - msg write are not always followed by msg read
> 
> And this can be dangerous if these interfaces are exported to
> userspace via i2c-dev :

Even without that... We certainly hope to reuse client drivers for
other families of DVB cards, and for this to work, every driver must
stick to the standard.

> - some scanning program avoid eeprom by filtering 0x5x range, but
> now it is at 0xax range (well that should happen because scan limit
> should be 0x77)
> - some read only command can be interpreted as write command.

Very bad indeed.

> What should be done ?
> Fix the drivers.

Yes, definitely. The sooner, the better.

> Have a mode where i2c interface are not exported to everybody.

I have considered this for a moment. It might be fair to let I2C bus
drivers decide whether they want i2c-dev to expose their buses or not.
We could use a new class bit (I2C_CLASS_USER or such) to this purpose. I
didn't get to it yet though, as this doesn't seem to be urgent, and
i2c-dev has so many other problems...

That being said, this is hardly a valid answer to the problem you
discovered. Preventing user-space from triggering bugs is not fixing
them.

> Don't care.

No, that's not an option.

> First why does the i2c stack doesn't check that the address is on
> 7 bits (like the attached patch) ?

Performance reasons, I presume. Having to check this each time a
transaction is attempted would be quite costly.

Secondly, I suspect it was never thought that raw I2C messaging would
become so popular. The original intent was to have all I2C device
drivers (except maybe i2c-dev) register their clients. The legacy
method for this (i2c_detect) _does_ have an address check included, and
so do i2c_new_probed_device and i2c_sysfs_new_device. Probably we
should add it to i2c_new_device as well, for consistency. It is way
less expensive to check the address once and for all, than with each
transaction.

I certainly hope that DVB will move to client-based I2C device drivers
at some point in time.

Note though that the address check is in no way bullet-proof. If
addresses in the range 0x03-0x3b are handled as 8-bit entities instead
of 7-bit entities, they will appear to be in the range 0x06-0x76, which
is perfectly valid.

> Also I believe a program for testing i2c interface corner case
> should catch most of these bugs :
> - null msg[i].len
> - different transactions on a device :
>  - one write/read transaction
>  - one write transaction then one read transaction
> [...]
> 
> Does a such program exist ?

We have several programs in the i2c-tools package, which can be used to
this purpose:
* i2cdetect
* i2cdump
* i2cget
* i2cset

With these 4 tools, almost all SMBus transaction types are covered.
This is sufficient in most cases in my experience. If not, these tools
can certainly get extended, at least to cover all of SMBus.

-- 
Jean Delvare

Re: i2c interface bugs in dvb drivers

[Mauro Carvalho Chehab]

Jean Delvare wrote:
> Hi Matthieu,
> 
> On Sun, 21 Mar 2010 15:02:50 +0100, matthieu castet wrote:
>> some dvb driver that export a i2c interface contains some mistakes
>> because they mainly used by driver (frontend, tuner) wrote for them.
>> But the i2c interface is exposed to everybody.
>>
>> One mistake is expect msg[i].addr with 8 bits address instead of 7
>> bits address. This make them use eeprom address at 0xa0 instead of
>> 0x50. Also they shift tuner address (qt1010 tuner is likely to be
>> at address 0x62, but some put it a 0xc4 (af9015, af9005, dtv5100)).
>>
>> Other mistakes is in xfer callback. Often the controller support a
>> limited i2c support (n bytes write then m bytes read). The driver
>> try to convert the linux i2c msg to this pattern, but they often
>> miss cases :
>> - msg[i].len can be null
> 
> Zero, not null. This is a very rare case, I've never seen it in
> multi-message transactions (wouldn't make much sense IMHO), only in
> the single-message transaction known as SMBus quick command. Many
> controllers don't support it, so I2C bus drivers don't have to support
> it. It should be assumed by I2C chip drivers that an I2C adapter
> without functionality bit I2C_FUNC_SMBUS_QUICK does not support 0-byte
> messages.
> 
>> - msg write are not always followed by msg read
>>
>> And this can be dangerous if these interfaces are exported to
>> userspace via i2c-dev :
> 
> Even without that... We certainly hope to reuse client drivers for
> other families of DVB cards, and for this to work, every driver must
> stick to the standard.
> 
>> - some scanning program avoid eeprom by filtering 0x5x range, but
>> now it is at 0xax range (well that should happen because scan limit
>> should be 0x77)
>> - some read only command can be interpreted as write command.
> 
> Very bad indeed.
> 
>> What should be done ?
>> Fix the drivers.
> 
> Yes, definitely. The sooner, the better.

Agreed. Could you please propose some patches to fix them?

>> Have a mode where i2c interface are not exported to everybody.
> 
> I have considered this for a moment. It might be fair to let I2C bus
> drivers decide whether they want i2c-dev to expose their buses or not.
> We could use a new class bit (I2C_CLASS_USER or such) to this purpose. 

This is a good idea. Users shouldn't be playing with the internal interfaces
inside the DVB driver. This can be dangerous, as it may cause driver
miss-functioning, cause memory corruption or eventually hit some bug and
make the driver crash. Also, due to I2C bridges that are very common 
on DVB designs, this interface is broken anyway, as the bridge code, when needed,
is generally implemented outside i2c xfer routines.

Cheers,
Mauro