From: chrisw@osdl.org Date: Thu, 10 Mar 2005 18:58:05 +0000 (-0800) Subject: [PATCH] amd64-oops-on-modprobe.patch: update with signed-off-by from Jean Delvare X-Git-Tag: v2.6.11.9~70 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=aca67fbf7e436589fbcb9c9b997398f0267fd5fa;p=thirdparty%2Fkernel%2Fstable-queue.git [PATCH] amd64-oops-on-modprobe.patch: update with signed-off-by from Jean Delvare --- diff --git a/amd64-oops-on-modprobe.patch b/amd64-oops-on-modprobe.patch index 5b8c7155564..b21cc13e0ba 100644 --- a/amd64-oops-on-modprobe.patch +++ b/amd64-oops-on-modprobe.patch @@ -1,97 +1,15 @@ -Date: Tue, 8 Mar 2005 20:15:04 +0100 +Date: Thu, 10 Mar 2005 19:43:42 +0100 From: Jean Delvare -To: "Randy.Dunlap" -Cc: Daniel Staaf , LKML , - Andrei Mikhailovsky , - Ian Campbell , - Ronald Bultje , - Gerd Knorr -Subject: Re: amd64 2.6.11 oops on modprobe -Lines: 182 +To: Chris Wright +Cc: Randy Dunlap , stable@kernel.org +Subject: [PATCH] fix amd64 2.6.11 oops on modprobe (saa7110) -[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: +This is a rewrite of the saa7110_write_block function, which was plain +broken in the case where the underlying adapter supports I2C_FUNC_I2C. +It also includes related fixes which ensure that different parts of the +driver agree on the number of registers the chip has. +Signed-off-by: Jean Delvare Signed-off-by: Chris Wright Signed-off-by: Greg Kroah-Hartman @@ -108,7 +26,7 @@ Signed-off-by: Greg Kroah-Hartman struct saa7110 { - unsigned char reg[54]; -+ unsigned char reg[SAA7110_NR_REG]; ++ u8 reg[SAA7110_NR_REG]; int norm; int input; @@ -165,33 +83,3 @@ Signed-off-by: Greg Kroah-Hartman 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/ -