[BK PATCH] I2C fixes for 2.6.11-rc3

[BK PATCH] I2C fixes for 2.6.11-rc3

[Greg KH]

Hi,

Here are a few I2C bugfixes 2.6.11-rc3.  All of these patches have been
in the past few -mm releases.

Please pull from:
	bk://kernel.bkbits.net/gregkh/linux/2.6.11-rc3/i2c

Patches will be posted to linux-kernel and sensors as a follow-up thread
for those who want to see them.

thanks,

greg k-h

 drivers/i2c/busses/i2c-sis5595.c |   15 +++++++----
 drivers/i2c/busses/i2c-viapro.c  |   33 ++++++++++++++++++--------
 drivers/i2c/chips/ds1621.c       |   12 ++++++---
 drivers/i2c/chips/it87.c         |   10 +++----
 drivers/i2c/chips/pc87360.c      |   49 +++++++++++++++++++++++++++------------
 drivers/i2c/chips/via686a.c      |   25 +++++++++++++------
 drivers/i2c/chips/w83781d.c      |   20 ++-------------
 7 files changed, 100 insertions(+), 64 deletions(-)
-----


Aurelien Jarno:
  o I2C: Fix DS1621 detection

Jean Delvare:
  o I2C: Prevent buffer overflow on SMBus block read in
  o I2C: Do not show disabled pc87360 fans
  o I2C: Fix i2c-sis5595 pci configuration accesses
  o I2C: Reduce it87 i2c address range
  o I2C: Use standard temperature converters for as99127f
  o I2C: Resolve resource conflict between i2c-viapro and via686a

[PATCH] I2C: Fix DS1621 detection

[Greg KH]

ChangeSet 1.2041, 2005/02/03 00:28:34-08:00, aurelien@aurel32.net

[PATCH] I2C: Fix DS1621 detection

Dallas Semiconductors as recently changed the design of their DS1621
chips, including the bits that were checked in the kernel driver to
detect it.

The patch below fixes the detection by checking an other bit of the
configuration register instead.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/chips/ds1621.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)


diff -Nru a/drivers/i2c/chips/ds1621.c b/drivers/i2c/chips/ds1621.c
--- a/drivers/i2c/chips/ds1621.c	2005-02-03 09:35:23 -08:00
+++ b/drivers/i2c/chips/ds1621.c	2005-02-03 09:35:23 -08:00
@@ -42,9 +42,8 @@
 /* Many DS1621 constants specified below */
 /* Config register used for detection         */
 /*  7    6    5    4    3    2    1    0      */
-/* |Done|THF |TLF |NVB | 1  | 0  |POL |1SHOT| */
-#define DS1621_REG_CONFIG_MASK		0x0C
-#define DS1621_REG_CONFIG_VAL		0x08
+/* |Done|THF |TLF |NVB | X  | X  |POL |1SHOT| */
+#define DS1621_REG_CONFIG_NVB		0x10
 #define DS1621_REG_CONFIG_POLARITY	0x02
 #define DS1621_REG_CONFIG_1SHOT		0x01
 #define DS1621_REG_CONFIG_DONE		0x80
@@ -55,6 +54,7 @@
 #define DS1621_REG_TEMP_MAX		0xA2 /* word, RW */
 #define DS1621_REG_CONF			0xAC /* byte, RW */
 #define DS1621_COM_START		0xEE /* no data */
+#define DS1621_COM_STOP			0x22 /* no data */
 
 /* The DS1621 configuration register */
 #define DS1621_ALARM_TEMP_HIGH		0x40
@@ -212,9 +212,13 @@
 
 	/* Now, we do the remaining detection. It is lousy. */
 	if (kind < 0) {
+		/* The NVB bit should be low if no EEPROM write has been 
+		   requested during the latest 10ms, which is highly 
+		   improbable in our case. */
 		conf = ds1621_read_value(new_client, DS1621_REG_CONF);
-		if ((conf & DS1621_REG_CONFIG_MASK) != DS1621_REG_CONFIG_VAL)
+		if (conf & DS1621_REG_CONFIG_NVB)
 			goto exit_free;
+		/* The 7 lowest bits of a temperature should always be 0. */
 		temp = ds1621_read_value(new_client, DS1621_REG_TEMP);
 		if (temp & 0x007f)
 			goto exit_free;

[PATCH] I2C: Resolve resource conflict between i2c-viapro and via686a

[Greg KH]

ChangeSet 1.2042, 2005/02/03 00:29:01-08:00, khali@linux-fr.org

[PATCH] I2C: Resolve resource conflict between i2c-viapro and via686a

Here comes the finalized version of our patch solving the PCI device
resource conflict between the i2c-viapro bus driver and and the via686a
chip driver. It is based on your original work and the IRC conversation
we had yesterday.

The retained solution is to not permanently register the PCI device in
either driver. This is legitimate since we only need it at init time to
retrieve the ISA address of a sub-device (SMBus master or integrated
sensors), and possibly change that address on user request. Once this is
done we can safely release the PCI device for others to use.

I am really glad to see this problem finally solved, as this was the
last remaining annoying issue left from the Linux 2.6 migration (missing
drivers left apart), and was generating many complaints both at our
level and at the distributions' support.


Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/busses/i2c-viapro.c |   27 +++++++++++++++++++--------
 drivers/i2c/chips/via686a.c     |   25 +++++++++++++++++--------
 2 files changed, 36 insertions(+), 16 deletions(-)


diff -Nru a/drivers/i2c/busses/i2c-viapro.c b/drivers/i2c/busses/i2c-viapro.c
--- a/drivers/i2c/busses/i2c-viapro.c	2005-02-03 09:35:16 -08:00
+++ b/drivers/i2c/busses/i2c-viapro.c	2005-02-03 09:35:16 -08:00
@@ -45,6 +45,8 @@
 #include <linux/init.h>
 #include <asm/io.h>
 
+static struct pci_dev *vt596_pdev;
+
 #define SMBBA1	   	 0x90
 #define SMBBA2     	 0x80
 #define SMBBA3     	 0xD0
@@ -381,19 +383,23 @@
 	snprintf(vt596_adapter.name, I2C_NAME_SIZE,
 			"SMBus Via Pro adapter at %04x", vt596_smba);
 	
-	return i2c_add_adapter(&vt596_adapter);
+	vt596_pdev = pci_dev_get(pdev);
+	if (i2c_add_adapter(&vt596_adapter)) {
+		pci_dev_put(vt596_pdev);
+		vt596_pdev = NULL;
+	}
+
+	/* Always return failure here.  This is to allow other drivers to bind
+	 * to this pci device.  We don't really want to have control over the
+	 * pci device, we only wanted to read as few register values from it.
+	 */
+	return -ENODEV;
 
  release_region:
 	release_region(vt596_smba, 8);
 	return error;
 }
 
-static void __devexit vt596_remove(struct pci_dev *pdev)
-{
-	i2c_del_adapter(&vt596_adapter);
-	release_region(vt596_smba, 8);
-}
-
 static struct pci_device_id vt596_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C596_3),
 	  .driver_data = SMBBA1 },
@@ -420,7 +426,6 @@
 	.name		= "vt596_smbus",
 	.id_table	= vt596_ids,
 	.probe		= vt596_probe,
-	.remove		= __devexit_p(vt596_remove),
 };
 
 static int __init i2c_vt596_init(void)
@@ -432,6 +437,12 @@
 static void __exit i2c_vt596_exit(void)
 {
 	pci_unregister_driver(&vt596_driver);
+	if (vt596_pdev != NULL) {
+		i2c_del_adapter(&vt596_adapter);
+		release_region(vt596_smba, 8);
+		pci_dev_put(vt596_pdev);
+		vt596_pdev = NULL;
+	}
 }
 
 MODULE_AUTHOR(
diff -Nru a/drivers/i2c/chips/via686a.c b/drivers/i2c/chips/via686a.c
--- a/drivers/i2c/chips/via686a.c	2005-02-03 09:35:16 -08:00
+++ b/drivers/i2c/chips/via686a.c	2005-02-03 09:35:16 -08:00
@@ -815,20 +815,24 @@
                return -ENODEV;
        }
        normal_isa[0] = addr;
-       s_bridge = dev;
-       return i2c_add_driver(&via686a_driver);
-}
 
-static void __devexit via686a_pci_remove(struct pci_dev *dev)
-{
-       i2c_del_driver(&via686a_driver);
+	s_bridge = pci_dev_get(dev);
+	if (i2c_add_driver(&via686a_driver)) {
+		pci_dev_put(s_bridge);
+		s_bridge = NULL;
+	}
+
+	/* Always return failure here.  This is to allow other drivers to bind
+	 * to this pci device.  We don't really want to have control over the
+	 * pci device, we only wanted to read as few register values from it.
+	 */
+	return -ENODEV;
 }
 
 static struct pci_driver via686a_pci_driver = {
        .name		= "via686a",
        .id_table	= via686a_pci_ids,
        .probe		= via686a_pci_probe,
-       .remove		= __devexit_p(via686a_pci_remove),
 };
 
 static int __init sm_via686a_init(void)
@@ -838,7 +842,12 @@
 
 static void __exit sm_via686a_exit(void)
 {
-       pci_unregister_driver(&via686a_pci_driver);
+	pci_unregister_driver(&via686a_pci_driver);
+	if (s_bridge != NULL) {
+		i2c_del_driver(&via686a_driver);
+		pci_dev_put(s_bridge);
+		s_bridge = NULL;
+	}
 }
 
 MODULE_AUTHOR("Kyösti Mälkki <kmalkki@cc.hut.fi>, "

[PATCH] I2C: Use standard temperature converters for as99127f

[Greg KH]

ChangeSet 1.2043, 2005/02/03 00:29:27-08:00, khali@linux-fr.org

[PATCH] I2C: Use standard temperature converters for as99127f

When support for the Asus AS99127F chip was once added to the w83781d
driver, it was decided that we would treat temp2 and temp3 as having a
LSB of 0.25 degree C, as opposed to 0.5 degree C for the compatible
Winbond chips. The reason why this was done seems to be a couple of
users reporting that these temperatures were reading twice as high as it
should for them in the first place. We had much more feedback about the
A99127F chip since, and it turns out that the exact conversion required
for temp2 and temp3 depends on the motherboard model. For some models
(including my A7V133-C), we now have to multiply the readings by 2,
effectively negating the change that was once done in the driver. For
other models, a linear conversion formula is needed. The bottom line is
that the raw readings from the driver are correct for no known board,
while it would be for at least some of them if we had kept the same LSB
as the Winbond chips are known to have. Thus I believe that the standard
LSB of 0.5 degree C should be restored.

There is no datasheet available for the AS99127F chip, so whatever was
done was guess work (and still is). I see no reason why we would keep
additional code in the w83781d driver to handle this former supposed
difference, especially when the facts now tend to prove that this
difference doesn't exist.

The following patch drops the additional code and treats temp2 and temp3
the same way for all chips supported by the w83781d driver. A similar
change will be made to the 2.4 version of this driver, and the default
sensors.conf will be updated accordingly. Users will have to update
their configuration file, or their readings will of course read twice as
high as they should due to the old conversion formulae.


Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/chips/w83781d.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)


diff -Nru a/drivers/i2c/chips/w83781d.c b/drivers/i2c/chips/w83781d.c
--- a/drivers/i2c/chips/w83781d.c	2005-02-03 09:35:09 -08:00
+++ b/drivers/i2c/chips/w83781d.c	2005-02-03 09:35:09 -08:00
@@ -175,11 +175,6 @@
 						: (val)) / 1000, 0, 0xff))
 #define TEMP_FROM_REG(val)		(((val) & 0x80 ? (val)-0x100 : (val)) * 1000)
 
-#define AS99127_TEMP_ADD_TO_REG(val)	(SENSORS_LIMIT((((val) < 0 ? (val)+0x10000*250 \
-						: (val)) / 250) << 7, 0, 0xffff))
-#define AS99127_TEMP_ADD_FROM_REG(val)	((((val) & 0x8000 ? (val)-0x10000 : (val)) \
-						>> 7) * 250)
-
 #define ALARMS_FROM_REG(val)		(val)
 #define PWM_FROM_REG(val)		(val)
 #define PWM_TO_REG(val)			(SENSORS_LIMIT((val),0,255))
@@ -417,13 +412,8 @@
 { \
 	struct w83781d_data *data = w83781d_update_device(dev); \
 	if (nr >= 2) {	/* TEMP2 and TEMP3 */ \
-		if (data->type == as99127f) { \
-			return sprintf(buf,"%ld\n", \
-				(long)AS99127_TEMP_ADD_FROM_REG(data->reg##_add[nr-2])); \
-		} else { \
-			return sprintf(buf,"%d\n", \
-				LM75_TEMP_FROM_REG(data->reg##_add[nr-2])); \
-		} \
+		return sprintf(buf,"%d\n", \
+			LM75_TEMP_FROM_REG(data->reg##_add[nr-2])); \
 	} else {	/* TEMP1 */ \
 		return sprintf(buf,"%ld\n", (long)TEMP_FROM_REG(data->reg)); \
 	} \
@@ -442,11 +432,7 @@
 	val = simple_strtol(buf, NULL, 10); \
 	 \
 	if (nr >= 2) {	/* TEMP2 and TEMP3 */ \
-		if (data->type == as99127f) \
-			data->temp_##reg##_add[nr-2] = AS99127_TEMP_ADD_TO_REG(val); \
-		else \
-			data->temp_##reg##_add[nr-2] = LM75_TEMP_TO_REG(val); \
-		 \
+		data->temp_##reg##_add[nr-2] = LM75_TEMP_TO_REG(val); \
 		w83781d_write_value(client, W83781D_REG_TEMP_##REG(nr), \
 				data->temp_##reg##_add[nr-2]); \
 	} else {	/* TEMP1 */ \

[PATCH] I2C: Reduce it87 i2c address range

[Greg KH]

ChangeSet 1.2044, 2005/02/03 00:29:54-08:00, khali@linux-fr.org

[PATCH] I2C: Reduce it87 i2c address range

IT87xxF chips were never seen at any other I2C address than the default
(0x2d) so I think that we could safely reduce the range of addresses the
it87 drivers accepts. Currently it accepts 0x20-0x2f, I believe that
0x28-0x2f would already be more than sufficient.

(In theory, any address is possible, so whatever range we choose is
arbitrary anyway.)

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/chips/it87.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)


diff -Nru a/drivers/i2c/chips/it87.c b/drivers/i2c/chips/it87.c
--- a/drivers/i2c/chips/it87.c	2005-02-03 09:35:02 -08:00
+++ b/drivers/i2c/chips/it87.c	2005-02-03 09:35:02 -08:00
@@ -2,8 +2,8 @@
     it87.c - Part of lm_sensors, Linux kernel modules for hardware
              monitoring.
 
-    Supports: IT8705F  Super I/O chip w/LPC interface
-              IT8712F  Super I/O chip w/LPC interface & SMbus
+    Supports: IT8705F  Super I/O chip w/LPC interface & SMBus
+              IT8712F  Super I/O chip w/LPC interface & SMBus
               Sis950   A clone of the IT8705F
 
     Copyright (C) 2001 Chris Gauthron <chrisg@0-in.com> 
@@ -42,10 +42,8 @@
 
 
 /* Addresses to scan */
-static unsigned short normal_i2c[] = { 0x20, 0x21, 0x22, 0x23, 0x24,
-					0x25, 0x26, 0x27, 0x28, 0x29,
-					0x2a, 0x2b, 0x2c, 0x2d, 0x2e,
-					0x2f, I2C_CLIENT_END };
+static unsigned short normal_i2c[] = { 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d,
+					0x2e, 0x2f, I2C_CLIENT_END };
 static unsigned int normal_isa[] = { 0x0290, I2C_CLIENT_ISA_END };
 
 /* Insmod parameters */

[PATCH] I2C: Fix i2c-sis5595 pci configuration accesses

[Greg KH]

ChangeSet 1.2045, 2005/02/03 00:30:21-08:00, khali@linux-fr.org

[PATCH] I2C: Fix i2c-sis5595 pci configuration accesses

The i2c-sis5595 bus driver has logic errors on pci configuration
accesses. It returns an error on success and vice versa. The 2.4 kernel
version of the driver, as found in the lm_sensors CVS repository, is
correct, so the problem was introducted when the driver was ported to
the 2.6 kernel tree  (in 2.6.0-test6). As odd as it sounds, the driver
has been sitting here broken and unusable for 17 months and nobody ever
reported, until yesterday.

Credits go to Sebastian Hesselbarth for discovering and analyzing the
problem.

Here is a patch that fixes the problem, succesfully tested by Aurelien
Jarno and Sebastian Hesselbarth. Please apply.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/busses/i2c-sis5595.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)


diff -Nru a/drivers/i2c/busses/i2c-sis5595.c b/drivers/i2c/busses/i2c-sis5595.c
--- a/drivers/i2c/busses/i2c-sis5595.c	2005-02-03 09:34:55 -08:00
+++ b/drivers/i2c/busses/i2c-sis5595.c	2005-02-03 09:34:55 -08:00
@@ -181,9 +181,11 @@
 
 	if (force_addr) {
 		dev_info(&SIS5595_dev->dev, "forcing ISA address 0x%04X\n", sis5595_base);
-		if (!pci_write_config_word(SIS5595_dev, ACPI_BASE, sis5595_base))
+		if (pci_write_config_word(SIS5595_dev, ACPI_BASE, sis5595_base)
+		    != PCIBIOS_SUCCESSFUL)
 			goto error;
-		if (!pci_read_config_word(SIS5595_dev, ACPI_BASE, &a))
+		if (pci_read_config_word(SIS5595_dev, ACPI_BASE, &a)
+		    != PCIBIOS_SUCCESSFUL)
 			goto error;
 		if ((a & ~(SIS5595_EXTENT - 1)) != sis5595_base) {
 			/* doesn't work for some chips! */
@@ -192,13 +194,16 @@
 		}
 	}
 
-	if (!pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, &val))
+	if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, &val)
+	    != PCIBIOS_SUCCESSFUL)
 		goto error;
 	if ((val & 0x80) == 0) {
 		dev_info(&SIS5595_dev->dev, "enabling ACPI\n");
-		if (!pci_write_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, val | 0x80))
+		if (pci_write_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, val | 0x80)
+		    != PCIBIOS_SUCCESSFUL)
 			goto error;
-		if (!pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, &val))
+		if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, &val)
+		    != PCIBIOS_SUCCESSFUL)
 			goto error;
 		if ((val & 0x80) == 0) {
 			/* doesn't work for some chips? */

[PATCH] I2C: Do not show disabled pc87360 fans

[Greg KH]

ChangeSet 1.2046, 2005/02/03 00:30:49-08:00, khali@linux-fr.org

[PATCH] I2C: Do not show disabled pc87360 fans

The pc87360 driver create sysfs files even for disabled fans. Since data
won't ever be updated, it doesn't make much sense. The following patch
adds some tests to only create the interface files that are actually
needed.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/chips/pc87360.c |   49 +++++++++++++++++++++++++++++++-------------
 1 files changed, 35 insertions(+), 14 deletions(-)


diff -Nru a/drivers/i2c/chips/pc87360.c b/drivers/i2c/chips/pc87360.c
--- a/drivers/i2c/chips/pc87360.c	2005-02-03 09:34:48 -08:00
+++ b/drivers/i2c/chips/pc87360.c	2005-02-03 09:34:48 -08:00
@@ -795,8 +795,10 @@
 
 	/* Fan clock dividers may be needed before any data is read */
 	for (i = 0; i < data->fannr; i++) {
-		data->fan_status[i] = pc87360_read_value(data, LD_FAN,
-				      NO_BANK, PC87360_REG_FAN_STATUS(i));
+		if (FAN_CONFIG_MONITOR(data->fan_conf, i))
+			data->fan_status[i] = pc87360_read_value(data,
+					      LD_FAN, NO_BANK,
+					      PC87360_REG_FAN_STATUS(i));
 	}
 
 	if (init > 0) {
@@ -898,14 +900,27 @@
 	}
 
 	if (data->fannr) {
-		device_create_file(&new_client->dev, &dev_attr_fan1_input);
-		device_create_file(&new_client->dev, &dev_attr_fan2_input);
-		device_create_file(&new_client->dev, &dev_attr_fan1_min);
-		device_create_file(&new_client->dev, &dev_attr_fan2_min);
-		device_create_file(&new_client->dev, &dev_attr_fan1_div);
-		device_create_file(&new_client->dev, &dev_attr_fan2_div);
-		device_create_file(&new_client->dev, &dev_attr_fan1_status);
-		device_create_file(&new_client->dev, &dev_attr_fan2_status);
+		if (FAN_CONFIG_MONITOR(data->fan_conf, 0)) {
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan1_input);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan1_min);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan1_div);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan1_status);
+		}
+
+		if (FAN_CONFIG_MONITOR(data->fan_conf, 1)) {
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan2_input);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan2_min);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan2_div);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan2_status);
+		}
 
 		if (FAN_CONFIG_CONTROL(data->fan_conf, 0))
 			device_create_file(&new_client->dev, &dev_attr_pwm1);
@@ -913,10 +928,16 @@
 			device_create_file(&new_client->dev, &dev_attr_pwm2);
 	}
 	if (data->fannr == 3) {
-		device_create_file(&new_client->dev, &dev_attr_fan3_input);
-		device_create_file(&new_client->dev, &dev_attr_fan3_min);
-		device_create_file(&new_client->dev, &dev_attr_fan3_div);
-		device_create_file(&new_client->dev, &dev_attr_fan3_status);
+		if (FAN_CONFIG_MONITOR(data->fan_conf, 2)) {
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan3_input);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan3_min);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan3_div);
+			device_create_file(&new_client->dev,
+					   &dev_attr_fan3_status);
+		}
 
 		if (FAN_CONFIG_CONTROL(data->fan_conf, 2))
 			device_create_file(&new_client->dev, &dev_attr_pwm3);

[PATCH] I2C: Prevent buffer overflow on SMBus block read in

[Greg KH]

ChangeSet 1.2047, 2005/02/03 00:31:16-08:00, khali@linux-fr.org

[PATCH] I2C: Prevent buffer overflow on SMBus block read in

Hi Greg, Linus, all,

I just hit a buffer overflow while playing around with i2cdump and
i2c-viapro through i2c-dev. This is caused by a missing length check on
a buffer operation when doing a SMBus block read in the i2c-viapro
driver. The problem was already known and had been fixed upon report by
Sergey Vlasov back in August 2003 in lm_sensors (2.4 kernel version of
the driver) but for some reason it was never ported to the 2.6 kernel
version.

I am not a security expert but I would guess that such a buffer overflow
could possibly be used to run arbitrary code in kernel space from user
space through i2c-dev. The severity obviously depends on the permisions
set on the i2c device files in /dev. Maybe it wouldn't be a bad idea to
push this patch upstream rather sooner than later.

While I was at it, I also changed a similar size check (for SMBus block
write this time) in the same driver to use the correct constant
I2C_SMBUS_BLOCK_MAX instead of its current numerical value. This doesn't
change a thing at the moment but prevents another potential buffer
overflow in case the value of I2C_SMBUS_BLOCK_MAX were to be changed in
the future (admittedly unlikely though).

> Now if we have broken hardware, then we might have a problem here, but
> otherwise I don't see it as a security issue right now.

It doesn't take broken hardware.

(Warning: I am going technical at this point, people not interested in
the gory details of the I2C and SMBus protocols should better stop here
;))

It just depends on what part of the SMBus and I2C specifications a given
client chip supports. SMBus block reads are no different from SMBus byte
reads, except that the master (here the VIA Pro) goes on reading after
the first byte sent by the slave (which could be about anything, from
hardware monitoring chip to EEPROM). In that respect, it also doesn't
much differ from the I2C block read, which also starts in the exact same
way. The difference between SMBus block read and I2C block read is that
the first byte returned by the slave on SMBus block read is supposed to
be the remaining number of data byte to be sent, while this is simply
the first data byte for I2C block reads.

To make it clearer, here comes the detail of the byte read, SMBus block
read and I2C block read commands (-> means from master to slave, <-
means from slave to master). See the official specifications for I2C and
SMBus for nicer graphics and additional details.

Byte read:
-> client address, write mode
-> register address
-> client address, read mode
<- data byte

SMBus block read:
-> client address, write mode
-> register address
-> client address, read mode
<- length byte (1 <=3D N <=3D 32)
<- first byte
<- next byte
<- ...
<- last (Nth) byte

I2C block read:
-> client address, write mode
-> register address
-> client address, read mode
<- first byte
<- next byte
<- ...
<- last byte

In each case, the *master* decides when to stop the transfer, not the
slave.

There are two consequences for us here:

1* The client chip cannot differenciate between byte read and SMBus block
read until after it sent a first byte - which basically means that a
given register address is specified to be read with either command, not
both, and not using the correct one returns bogus results. i2c-dev
allows arbitrary commands so it is possible to ask for a SMBus block
read on a register that expects a simple byte read. The client
innocently will answer with the register value - which the master will
interpret as a length, and the master will then request that many
additional data bytes. If the client features autoincrement in this
register address range, it will most likely provide the value of the
next registers, if not it will dumbly return the same register value
again and again.

This illustrates the fact that it doesn't take a broken chip to cause a
buffer overflow. It only takes a SMBus block read command on a register
for which the client did not expect it (and almost no client actually
supports SMBus block reads at the moment). If it happens that the
register value was greater than 32, the buffer overflow will occur
(without Sergey's fix, that is). So, with write access to the i2c
device files, it is actually very easy to trigger the buffer overflow,
providing there is at least one chip on the VIA Pro SMBus.

2* A client chip can obviously only implement SMBus block read or I2C
block read for a given register address, since the sequence sent by the
master is exactly the same. Not a big deal since a client chip is
designed either as an I2C slave or as a SMBus slave. However the master
doesn't know this, and i2c-dev allows arbitrary commands, so it is
possible to use an SMBus block read on an I2C slave which expected
instead an I2C block read, causing weird results.

EEPROMs are such I2C slaves and they support I2C block reads. Now,
imagine that a non-write-protected EEPROM hangs on my VIA Pro SMBus (a
memory module SPD EEPROM would probably do), and for some reason i2c-dev
gives me access to it. I can write arbitrary bytes to the EEPROM using
simple byte writes. I could write the following bytes, in order, at some
location: 0x80, 34 null bytes, 94 bytes of nasty code. Then, still
through i2c-dev, I request a SMBus block read from the same location.
The EEPROM will answer as if it were an I2C block read (it can't
differenciate and doesn't support SMBus block reads anyway), i.e. it
will return as many bytes as requested, in order. The VIA Pro master
will however interpret the first byte (0x80) as a length, and will read
128 bytes from the EEPROM, 34 of which will fill the data buffer, and 94
will overflow. Providing I know how the kernel works, these 94 bytes
could be used for doing presumably bad things.

This illustrates the fact that the user may actually control the buffer
overflow, indirectly, depending on what hardware is present on the bus.
EEPROMs are the most obvious way to do it, but some hardware monitoring
chips have RAM arrays that could presumably be used in a similar way.

As a conclusion, I definitely agree that this buffer overflow isn't easy
to exploit, as it takes a particular combination of hardware and
non-standard permissions on i2c device files, and also requires very
good knowledge of the I2C and SMBus protocols; it is not impossible
though.


Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>


 drivers/i2c/busses/i2c-viapro.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)


diff -Nru a/drivers/i2c/busses/i2c-viapro.c b/drivers/i2c/busses/i2c-viapro.c
--- a/drivers/i2c/busses/i2c-viapro.c	2005-02-03 09:34:40 -08:00
+++ b/drivers/i2c/busses/i2c-viapro.c	2005-02-03 09:34:40 -08:00
@@ -233,8 +233,8 @@
 			len = data->block[0];
 			if (len < 0)
 				len = 0;
-			if (len > 32)
-				len = 32;
+			if (len > I2C_SMBUS_BLOCK_MAX)
+				len = I2C_SMBUS_BLOCK_MAX;
 			outb_p(len, SMBHSTDAT0);
 			i = inb_p(SMBHSTCNT);	/* Reset SMBBLKDAT */
 			for (i = 1; i <= len; i++)
@@ -268,6 +268,8 @@
 		break;
 	case VT596_BLOCK_DATA:
 		data->block[0] = inb_p(SMBHSTDAT0);
+		if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
+			data->block[0] = I2C_SMBUS_BLOCK_MAX;
 		i = inb_p(SMBHSTCNT);	/* Reset SMBBLKDAT */
 		for (i = 1; i <= data->block[0]; i++)
 			data->block[i] = inb_p(SMBBLKDAT);