i2c: busses: Fix out-of-bounds bug in mchp_corei2c_smbus_xfer

[PATCH] i2c: busses: Fix out-of-bounds bug in mchp_corei2c_smbus_xfer

Posted by Alex Guo 3 months, 3 weeks ago

The data->block[0] variable comes from user. Without proper check,
the variable may be very large to cause an out-of-bounds bug.

Fix this bug by checking the value of data->block[0] first.

Similar commit:
1. commit 39244cc7548 ("i2c: ismt: Fix an out-of-bounds bug in
ismt_access()")
2. commit 92fbb6d1296 ("i2c: xgene-slimpro: Fix out-of-bounds
bug in xgene_slimpro_i2c_xfer()")

Signed-off-by: Alex Guo <alexguo1023@gmail.com>
---
 drivers/i2c/busses/i2c-microchip-corei2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c
index 492bf4c34722..a79d4d327f20 100644
--- a/drivers/i2c/busses/i2c-microchip-corei2c.c
+++ b/drivers/i2c/busses/i2c-microchip-corei2c.c
@@ -492,6 +492,8 @@ static int mchp_corei2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned
 		if (read_write == I2C_SMBUS_WRITE) {
 			int data_len;
 
+			if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
+				return -EINVAL;
 			data_len = data->block[0];
 			msgs[CORE_I2C_SMBUS_MSG_WR].len = data_len + 2;
 			for (int i = 0; i <= data_len; i++)
-- 
2.34.1

Re: [PATCH] i2c: busses: Fix out-of-bounds bug in mchp_corei2c_smbus_xfer

Posted by Wolfram Sang 3 months, 1 week ago

On Sun, Jun 15, 2025 at 07:49:19PM -0400, Alex Guo wrote:
> The data->block[0] variable comes from user. Without proper check,
> the variable may be very large to cause an out-of-bounds bug.

Okay, okay, instead of adding these limits to all these drivers, let me
start adding SMBus3 support to the kernel which allows for bigger block
sizes. I probably won't have time to export this to user space yet, but
let's at least make sure the kernel, and thus the drivers, won't suffer
from buffer overflows anymore.

Re: [PATCH] i2c: busses: Fix out-of-bounds bug in mchp_corei2c_smbus_xfer

Posted by Andi Shyti 2 months, 3 weeks ago

Hi Wolfram,

On Sat, Jun 28, 2025 at 10:09:48PM +0200, Wolfram Sang wrote:
> On Sun, Jun 15, 2025 at 07:49:19PM -0400, Alex Guo wrote:
> > The data->block[0] variable comes from user. Without proper check,
> > the variable may be very large to cause an out-of-bounds bug.
> 
> Okay, okay, instead of adding these limits to all these drivers, let me
> start adding SMBus3 support to the kernel which allows for bigger block
> sizes. I probably won't have time to export this to user space yet, but
> let's at least make sure the kernel, and thus the drivers, won't suffer
> from buffer overflows anymore.

in the meantime, what should i do with all these patches? They
are not wrong and I agree that we need to work on the i2c core.
Besides having something like this might help all those drivers
using i2c by removing the need for their own specific
i2c_read/write methods.

Some times ago I started working on this and I have it still
laying around in one of my branches. If you haven'd done much
progress, i can bring it to life.

Andi

Re: [PATCH] i2c: busses: Fix out-of-bounds bug in mchp_corei2c_smbus_xfer

Posted by Conor Dooley 3 months, 3 weeks ago

On Sun, Jun 15, 2025 at 07:49:19PM -0400, Alex Guo wrote:
> The data->block[0] variable comes from user. Without proper check,
> the variable may be very large to cause an out-of-bounds bug.
> 
> Fix this bug by checking the value of data->block[0] first.
> 
> Similar commit:
> 1. commit 39244cc7548 ("i2c: ismt: Fix an out-of-bounds bug in
> ismt_access()")
> 2. commit 92fbb6d1296 ("i2c: xgene-slimpro: Fix out-of-bounds
> bug in xgene_slimpro_i2c_xfer()")
> 
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>
> ---
>  drivers/i2c/busses/i2c-microchip-corei2c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c
> index 492bf4c34722..a79d4d327f20 100644
> --- a/drivers/i2c/busses/i2c-microchip-corei2c.c
> +++ b/drivers/i2c/busses/i2c-microchip-corei2c.c
> @@ -492,6 +492,8 @@ static int mchp_corei2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned
>  		if (read_write == I2C_SMBUS_WRITE) {
>  			int data_len;
>  
> +			if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
> +				return -EINVAL;

Seems reasonable, but I'd like to see a blank line here after the
return. Maybe Andi can do it on application?

Acked-by: Conor Dooley <conor.dooley@microchip.com>

>  			data_len = data->block[0];
>  			msgs[CORE_I2C_SMBUS_MSG_WR].len = data_len + 2;
>  			for (int i = 0; i <= data_len; i++)
> -- 
> 2.34.1
>