--- /dev/null
+Date: Tue, 8 Mar 2005 20:15:04 +0100
+From: Jean Delvare <khali@linux-fr.org>
+To: "Randy.Dunlap" <rddunlap@osdl.org>
+Cc: Daniel Staaf <dst@bostream.nu>, LKML <linux-kernel@vger.kernel.org>,
+ Andrei Mikhailovsky <andrei@arhont.com>,
+ Ian Campbell <icampbell@arcom.com>,
+ Ronald Bultje <rbultje@ronald.bitfreak.net>,
+ Gerd Knorr <kraxel@bytesex.org>
+Subject: Re: amd64 2.6.11 oops on modprobe
+Lines: 182
+
+[Randy Dunlap]
+> I've been working on this with Daniel Staaf and Jean Delvare.
+> Jean enabled some more/different I2C bit banging code in
+> 2.6.11, and that causes callers to use it differently.
+
+Actually credits go to Ian Campbell for noticing that i2c-algo-bit was
+reporting less capabilities than it should have. Fixing this uncovered
+the bug in the saa7110 driver as a side effect but still was the right
+thing to do (this will let i2c chip drivers use faster i2c transfer
+commands wherever possible.)
+
+> Jean will be proposing a real patch for that obfuscated loop.
+
+I'd like to add details on what exactly the problems were in the
+original code, in the hope it can help make my patch easier to
+understand. As a side note, this was by far the crapiest code I read for
+the last few months.
+
+Here's the faulty code, with my comments:
+
+ u8 reg = *data++;
+ (...)
+ if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ struct saa7110 *decoder = i2c_get_clientdata(client);
+ struct i2c_msg msg;
+*1* u8 block_data[54];
+
+ msg.len = 0;
+ msg.buf = (char *) block_data;
+ msg.addr = client->addr;
+*2* msg.flags = client->flags;
+ while (len >= 1) {
+ msg.len = 0;
+ block_data[msg.len++] = reg;
+*3* while (len-- >= 1 && msg.len < 54)
+ block_data[msg.len++] =
+*4* decoder->reg[reg++] = *data++;
+*5* ret = i2c_transfer(client->adapter, &msg, 1);
+ }
+ } (...)
+
+1* This local buffer is not needed. The original buffer (pointed to by
+data) is already properly formated for an i2c_transfer message.
+
+2* i2c message flags and i2c client flags are essentially different
+things [1], so this line doesn't make any sense. In fact, it happens
+that client->flags contains I2C_CLIENT_ALLOW_USE (0x01) which will be
+interpreted as I2C_M_RD as a message flag, resulting in an I2C block
+*read* command instead of the expected I2C block write command. So, oops
+or not, there is no chance this code would do anything useful [2].
+
+3* The oops is here. When len == 0, the test will fail but len will
+still be decreased. Because len is unsigned, this will result in a huge
+positive value, so the outer while loop will not be exited as expected.
+One hundred iterations later the oops occurs. Mental note: mixing while
+loop condition with counter decrement sounds like a generally bad idea.
+The code here clearly yields for a clean for loop.
+
+4* Note that reg is not reset in the outer while loop, so it can take
+any arbitrarily large value, while decoder->reg is only 54 bytes long.
+So there is a potential buffer overrun here. More generally, the code
+makes it look like a message of an arbitrarily long length can be
+properly handled (split in 54-byte chunks), while several details would
+actually make it fail. Also note that sending messages longer than the
+number of registers makes no sense anyway, so not only the double while
+loop is complex and broken, but it is also totally useless as far as I
+can see.
+
+5* Return value is not properly handled.
+
+Considering that this code has been in there since 2003-08-21
+(2.6.0-test3-bk9), I find it quite frightening that nobody noticed how
+broken it was until yesterday. Another frightening fact is that the
+i2c-algo-bit change made the whole Zoran-based family of devices
+unusable for the past 3 months or so in -mm and maybe 1.5 month in
+-bk/-rc and nobody reported. As far as I know, these boards are rather
+popular. The fact that no user of such board actually tested -mm or -rc
+for this period of time points out a problem. (And I am not attempting
+to revive a flame war of any kind, nor am I blaming anyone in
+particular, merely pointing out what I think is a serious problem.)
+
+So here is my patch:
+
+Signed-off-by: Chris Wright <chrisw@osdl.org>
+
+--- linux-2.6.11-bk3/drivers/media/video/saa7110.c.orig Tue Mar 8 10:27:15 2005
++++ linux-2.6.11-bk3/drivers/media/video/saa7110.c Tue Mar 8 12:02:45 2005
+@@ -58,10 +58,12 @@
+ #define SAA7110_MAX_INPUT 9 /* 6 CVBS, 3 SVHS */
+ #define SAA7110_MAX_OUTPUT 0 /* its a decoder only */
+
+-#define I2C_SAA7110 0x9C /* or 0x9E */
++#define I2C_SAA7110 0x9C /* or 0x9E */
++
++#define SAA7110_NR_REG 0x35
+
+ struct saa7110 {
+- unsigned char reg[54];
++ unsigned char reg[SAA7110_NR_REG];
+
+ int norm;
+ int input;
+@@ -95,31 +97,28 @@
+ unsigned int len)
+ {
+ int ret = -1;
+- u8 reg = *data++;
++ u8 reg = *data; /* first register to write to */
+
+- len--;
++ /* Sanity check */
++ if (reg + (len - 1) > SAA7110_NR_REG)
++ return ret;
+
+ /* the saa7110 has an autoincrement function, use it if
+ * the adapter understands raw I2C */
+ if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ struct saa7110 *decoder = i2c_get_clientdata(client);
+ struct i2c_msg msg;
+- u8 block_data[54];
+
+- msg.len = 0;
+- msg.buf = (char *) block_data;
++ msg.len = len;
++ msg.buf = (char *) data;
+ msg.addr = client->addr;
+- msg.flags = client->flags;
+- while (len >= 1) {
+- msg.len = 0;
+- block_data[msg.len++] = reg;
+- while (len-- >= 1 && msg.len < 54)
+- block_data[msg.len++] =
+- decoder->reg[reg++] = *data++;
+- ret = i2c_transfer(client->adapter, &msg, 1);
+- }
++ msg.flags = 0;
++ ret = i2c_transfer(client->adapter, &msg, 1);
++
++ /* Cache the written data */
++ memcpy(decoder->reg + reg, data + 1, len - 1);
+ } else {
+- while (len-- >= 1) {
++ for (++data, --len; len; len--) {
+ if ((ret = saa7110_write(client, reg++,
+ *data++)) < 0)
+ break;
+@@ -192,7 +191,7 @@
+ return 0;
+ }
+
+-static const unsigned char initseq[] = {
++static const unsigned char initseq[1 + SAA7110_NR_REG] = {
+ 0, 0x4C, 0x3C, 0x0D, 0xEF, 0xBD, 0xF2, 0x03, 0x00,
+ /* 0x08 */ 0xF8, 0xF8, 0x60, 0x60, 0x00, 0x86, 0x18, 0x90,
+ /* 0x10 */ 0x00, 0x59, 0x40, 0x46, 0x42, 0x1A, 0xFF, 0xDA,
+
+
+I could load the modified driver on my own DC10+ board (just plugged-in
+for the tests) without oops. Unfortunately I am not able to test it any
+further (crappy motherboad and lack of video source and tv set).
+
+The idea of the patch is to keep everything simple because there is no
+reason to do otherwise, and add some checks to prevent buffer overruns.
+I would appreciate if people familiar with this video chip could check
+that my code does the correct thing. I would also appreciate if users of
+Zoran-based boards could test it and confirm it works OK.
+
+Thanks.
+
+[1] Actually I found that there is one common flag between client and
+message, but this is a bad idea IMHO and should never be relied upon
+outside of i2c-core.
+
+[2] I found 5 other drivers with the same problem: adv7170, adv7175,
+bt819, saa7114, saa7185, all used by Zoran-based boards. I'll post a
+separate patch for these drivers.
+
+--
+Jean Delvare
+-
+To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
+the body of a message to majordomo@vger.kernel.org
+More majordomo info at http://vger.kernel.org/majordomo-info.html
+Please read the FAQ at http://www.tux.org/lkml/
+
--- /dev/null
+Date: Tue, 8 Mar 2005 20:25:30 +0100
+From: Jean Delvare <khali@linux-fr.org>
+To: "Randy.Dunlap" <rddunlap@osdl.org>
+Cc: Daniel Staaf <dst@bostream.nu>, LKML <linux-kernel@vger.kernel.org>,
+ Andrei Mikhailovsky <andrei@arhont.com>,
+ Ian Campbell <icampbell@arcom.com>,
+ Ronald Bultje <rbultje@ronald.bitfreak.net>,
+ Gerd Knorr <kraxel@bytesex.org>
+Subject: [PATCH 2.6] Fix i2c messsage flags in video drivers
+Lines: 86
+
+While working on the saa7110 driver I found a problem with the way
+various video drivers (found on Zoran-based boards) prepare i2c messages
+to be used by i2c_transfer. The drivers improperly copy the i2c client
+flags as the message flags, while both sets are mostly unrelated. The
+net effect in this case is to trigger an I2C block read instead of the
+expected I2C block write. The fix is simply not to pass any flag,
+because none are needed.
+
+I think this patch qualifies hands down as a "critical bug fix" to be
+included in whatever bug-fix-only trees exist these days. As far as I
+can see, all Zoran-based boards are broken in 2.6.11 without this patch.
+
+Signed-off-by: Jean Delvare <khali@linux-fr.org>
+Signed-off-by: Chris Wright <chrisw@osdl.org>
+
+diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/adv7170.c linux-2.6.11-bk3/drivers/media/video/adv7170.c
+--- linux-2.6.11-bk3/drivers/media/video.orig/adv7170.c Tue Mar 8 10:27:14 2005
++++ linux-2.6.11-bk3/drivers/media/video/adv7170.c Tue Mar 8 12:19:04 2005
+@@ -130,7 +130,7 @@
+ u8 block_data[32];
+
+ msg.addr = client->addr;
+- msg.flags = client->flags;
++ msg.flags = 0;
+ while (len >= 2) {
+ msg.buf = (char *) block_data;
+ msg.len = 0;
+diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/adv7175.c linux-2.6.11-bk3/drivers/media/video/adv7175.c
+--- linux-2.6.11-bk3/drivers/media/video.orig/adv7175.c Tue Mar 8 10:27:14 2005
++++ linux-2.6.11-bk3/drivers/media/video/adv7175.c Tue Mar 8 12:18:57 2005
+@@ -126,7 +126,7 @@
+ u8 block_data[32];
+
+ msg.addr = client->addr;
+- msg.flags = client->flags;
++ msg.flags = 0;
+ while (len >= 2) {
+ msg.buf = (char *) block_data;
+ msg.len = 0;
+diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/bt819.c linux-2.6.11-bk3/drivers/media/video/bt819.c
+--- linux-2.6.11-bk3/drivers/media/video.orig/bt819.c Tue Mar 8 10:27:15 2005
++++ linux-2.6.11-bk3/drivers/media/video/bt819.c Tue Mar 8 12:18:51 2005
+@@ -146,7 +146,7 @@
+ u8 block_data[32];
+
+ msg.addr = client->addr;
+- msg.flags = client->flags;
++ msg.flags = 0;
+ while (len >= 2) {
+ msg.buf = (char *) block_data;
+ msg.len = 0;
+diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/saa7114.c linux-2.6.11-bk3/drivers/media/video/saa7114.c
+--- linux-2.6.11-bk3/drivers/media/video.orig/saa7114.c Tue Mar 8 10:27:15 2005
++++ linux-2.6.11-bk3/drivers/media/video/saa7114.c Tue Mar 8 12:18:20 2005
+@@ -163,7 +163,7 @@
+ u8 block_data[32];
+
+ msg.addr = client->addr;
+- msg.flags = client->flags;
++ msg.flags = 0;
+ while (len >= 2) {
+ msg.buf = (char *) block_data;
+ msg.len = 0;
+diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/saa7185.c linux-2.6.11-bk3/drivers/media/video/saa7185.c
+--- linux-2.6.11-bk3/drivers/media/video.orig/saa7185.c Tue Mar 8 10:27:15 2005
++++ linux-2.6.11-bk3/drivers/media/video/saa7185.c Tue Mar 8 12:18:12 2005
+@@ -118,7 +118,7 @@
+ u8 block_data[32];
+
+ msg.addr = client->addr;
+- msg.flags = client->flags;
++ msg.flags = 0;
+ while (len >= 2) {
+ msg.buf = (char *) block_data;
+ msg.len = 0;
+
+
+
+--
+Jean Delvare
+-
+To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
+the body of a message to majordomo@vger.kernel.org
+More majordomo info at http://vger.kernel.org/majordomo-info.html
+Please read the FAQ at http://www.tux.org/lkml/
+
projects / thirdparty / kernel / stable-queue.git / commitdiff
