[PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Alex Guo]

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-rtl9300.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index e064e8a4a1f0..568495720810 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -281,6 +281,10 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
 		if (ret)
 			goto out_unlock;
+		if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) {
+			ret = -EINVAL;
+			goto out_unlock;
+		}
 		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
 		if (ret)
 			goto out_unlock;
-- 
2.34.1

Re: [PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Chris Packham]

Hi Alex,

On 16/06/2025 11:52, 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>

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Thanks

> ---
>   drivers/i2c/busses/i2c-rtl9300.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index e064e8a4a1f0..568495720810 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -281,6 +281,10 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>   		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
>   		if (ret)
>   			goto out_unlock;
> +		if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
>   		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
>   		if (ret)
>   			goto out_unlock;

Re: [PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Monday, 16 June 2025 01:52:48 CEST 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()")
[...]

Please correct me but it looks like this fix was not yet applied to the tree. 
But Chris Packham pointed out that this conflicts with my fixes for SMBUS/
SMBUS_I2C.

I would like to add my patchset on top of this (to avoid problems with stable 
submission) and add the Fixes: and Cc: stable@vger.kernel.org.

I hope it is ok for you when I would pick this up. I would resubmit the fixes 
patchset this evening (GMT+2).

You can preview it at 
https://git.open-mesh.org/linux-merge.git/log/?h=b4/i2c-rtl9300-multi-byte

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Mon, Aug 04, 2025 at 10:18:53AM +0200, Sven Eckelmann wrote:
> On Monday, 16 June 2025 01:52:48 CEST 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()")
> [...]
> 
> Please correct me but it looks like this fix was not yet applied to the tree. 
> But Chris Packham pointed out that this conflicts with my fixes for SMBUS/
> SMBUS_I2C.
> 
> I would like to add my patchset on top of this (to avoid problems with stable 
> submission) and add the Fixes: and Cc: stable@vger.kernel.org.
> 
> I hope it is ok for you when I would pick this up. I would resubmit the fixes 
> patchset this evening (GMT+2).
> 
> You can preview it at 
> https://git.open-mesh.org/linux-merge.git/log/?h=b4/i2c-rtl9300-multi-byte

Yes, we can do that. In general, it doesn't make sense to add this check
when the ultimate goal is to support SMBus v3 which doesn't need the
check anymore. But if it is blocking further development, we can apply
this. The check will be removed when SMBus v3 support comes in.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On Monday, 4 August 2025 11:17:47 CEST Wolfram Sang wrote:
> Yes, we can do that. In general, it doesn't make sense to add this check
> when the ultimate goal is to support SMBus v3 which doesn't need the
> check anymore. But if it is blocking further development, we can apply
> this. The check will be removed when SMBus v3 support comes in.

Yes, when I2C_SMBUS_BLOCK_MAX becomes >= 255 bytes, such a check would not 
be necessary. But this driver is already in Linux 6.13 - and in this version, 
I2C_SMBUS_BLOCK_MAX is just 32 bytes. So, just from the code perspective, it 
would be interesting for Linux stable to get this fixed in longterm kernel 
6.15 (and also the most recent Linux 6.16.y).

If you want to have an argument against this patch then it would be the the 
hardware limitation of this i2c host controller. It only allows to transfer up 
to 16 bytes. But then you could also argue that there might be variants which 
will not have this limitation anymore. And Jonas is already trying to make the 
driver more flexible [1] - the future will only show whether this will ever be 
a relevant check (before I2C_SMBUS_BLOCK_MAX is large enough to make this 
check obsolete).

Btw. I've already picked it up in my patchset [2] to avoid conflicts with this 
patch. And since they (2-4) fix broken functionality in Linux 6.13, this patch 
becomes a requirement for backporting those fixes to the stable kernels.

Kind regards,
	Sven

[1] https://lore.kernel.org/r/20250729075145.2972-1-jelonek.jonas@gmail.com
[2] https://lore.kernel.org/r/20250804-i2c-rtl9300-multi-byte-v3-0-e20607e1b28c@narfation.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]


> Btw. I've already picked it up in my patchset [2] to avoid conflicts with this 
> patch. And since they (2-4) fix broken functionality in Linux 6.13, this patch 
> becomes a requirement for backporting those fixes to the stable kernels.

Thanks for the heads up. As I already said, in your case we can surely
include this patch.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]