PATCH i2c: smbus_read_block write out of buffer on noisy bus

Hi, all
On i2c_smbus_read_block my device sometimes returns FFs, so kernel
writes results out of buffer and halts. So it may be treated as
security hole too. This code exists since v2.6
Please mitigate me. :)
Pavel

On Thu, 21 Jun 2018 18:03:24 +0300, Pasha Orehov wrote:
> On i2c_smbus_read_block my device sometimes returns FFs, so kernel
> writes results out of buffer and halts. So it may be treated as
> security hole too. This code exists since v2.6
> Please mitigate me. :)
> 
> --- i2c-core-smbus-linus.c    2018-06-21 17:04:10.620609631 +0300
> +++ i2c-core-smbus.c    2018-06-21 17:22:23.145417235 +0300
> @@ -226,7 +226,7 @@
>      if (status)
>          return status;
> 
> -    memcpy(values, &data.block[1], data.block[0]);
> +    memcpy(values, &data.block[1], min(data.block[0], sizeof(data)));
>      return data.block[0];
>  }
>  EXPORT_SYMBOL(i2c_smbus_read_block_data);
> @@ -494,7 +494,7 @@
>              break;
>          case I2C_SMBUS_BLOCK_DATA:
>          case I2C_SMBUS_BLOCK_PROC_CALL:
> -            for (i = 0; i < msg[1].buf[0] + 1; i++)
> +            for (i = 0; i < min(sizeof(i2c_smbus_data), msg[1].buf[0]
> + 1); i++)
>                  data->block[i] = msg[1].buf[i];
>              break;
>          }

Please send patches in proper format (full path to files, diff option
-p to include function names, and no line wrapping by your email
client.)

I am also curious which i2c bus driver you are using. It is the bus
driver which should ensure that the block length returned by the I2C
slave device is within the specifications. If your bus driver receives
0xff as the length, blindly writes that to data.block[0] and keeps
processing, it is buggy, and you must be hitting some kind of buffer
overrun inside the bus driver already. Trying to fix that in i2c-core
is too late, and redundant for all bus drivers which do the right thing.

Excuse me for the delay, but right mail client for gmail is not trivial to 
find.
I hope the patch is formatted correctly now.
> Please send patches in proper format (full path to files, diff option
> -p to include function names, and no line wrapping by your email
> client.)

diff -rup linux-4.18.pre-orig/drivers/i2c/i2c-core-smbus.c linux-4.18.pre/drivers/i2c/i2c-core-smbus.c
--- linux-4.18.pre-orig/drivers/i2c/i2c-core-smbus.c	2018-07-03 18:53:21.328749069 +0300
+++ linux-4.18.pre/drivers/i2c/i2c-core-smbus.c	2018-07-03 19:16:56.426642993 +0300
@@ -226,7 +226,7 @@ s32 i2c_smbus_read_block_data(const stru
  	if (status)
  		return status;

-	memcpy(values, &data.block[1], data.block[0]);
+	memcpy(values, &data.block[1], min_t(size_t, data.block[0], sizeof(data)-1));
  	return data.block[0];
  }
  EXPORT_SYMBOL(i2c_smbus_read_block_data);
@@ -497,7 +497,7 @@ static s32 i2c_smbus_xfer_emulated(struc
  			break;
  		case I2C_SMBUS_BLOCK_DATA:
  		case I2C_SMBUS_BLOCK_PROC_CALL:
-			for (i = 0; i < msg[1].buf[0] + 1; i++)
+			for (i = 0; i < min_t(size_t, msg[1].buf[0] + 1, sizeof(data->block)); i++)
  				data->block[i] = msg[1].buf[i];
  			break;
  		}


i2c_smbus_read_i2c_block_data and co do not need to be checked because 
they use user's size, not returned.

> I am also curious which i2c bus driver you are using. It is the bus
> driver which should ensure that the block length returned by the I2C
I used i2c-usb-tiny. It use size given by user (maxsize) for returned 
object. It verified by 89c6efa and co.
So there is no problem here.

Also I checked some another bus drivers. No problem too.

> slave device is within the specifications. If your bus driver receives
> 0xff as the length, blindly writes that to data.block[0] and keeps
> processing, it is buggy, and you must be hitting some kind of buffer
> overrun inside the bus driver already. Trying to fix that in i2c-core
> is too late, and redundant for all bus drivers which do the right thing.
>
> -- 
> Jean Delvare
> SUSE L3 Support
>


Pavel