mfd: mt6370: add bounds checking to regmap_read/write functions

It looks like there are a potential out of bounds accesses in the read/write() functions. Also can "len" be negative? Let's check for that too.

Fixes ab9905c5e38e ("mfd: mt6370: Add MediaTek MT6370 support")

Signed-off-by Dan Carpenter <[email protected]>

---
This came from static analysis, but then I couldn't figure out the next day if it was actually required or not so we dropped it. But then ChiYuan Huang did some testing and it was required.

There was some debate around various style questions but honestly I'm pretty happy with the style the way I wrote it. I've written a long blog on why "unsigned int" is good at the user space boundary but should not be the default choice as a stack variable.

https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/

The other style issue was wether to use ARRAY_SIZE() or MT6370_MAX_I2C and I just think ARRAY_SIZE() is more obvious to the reader.

 drivers/mfd/mt6370.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/mfd/mt6370.c +6 -0

index cf19cce2fdc0..fd5e1d6a0272 100644
@@ -190,6 +190,9 @@ static int mt6370_regmap_read(void *context, const void *reg_buf,
 	bank_idx = u8_buf[0];
 	bank_addr = u8_buf[1];
 
+	if (bank_idx >= ARRAY_SIZE(info->i2c))
+		return -EINVAL;
+
 	ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr,
 					    val_size, val_buf);
 	if (ret < 0)

@@ -211,6 +214,9 @@ static int mt6370_regmap_write(void *context, const void *data, size_t count)
 	bank_idx = u8_buf[0];
 	bank_addr = u8_buf[1];
 
+	if (len < 0 || bank_idx >= ARRAY_SIZE(info->i2c))
+		return -EINVAL;
+
 	return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr,
 					      len, data + MT6370_MAX_ADDRLEN);
 }

Signature · 1 line

2.35.1

2) normal register access with negative length

Unable to handle kernel paging request at virtual address ffffffc009cefff2
pc : __memcpy+0x1dc/0x260
lr : _regmap_raw_write_impl+0x6d4/0x828
Call trace:
 __memcpy+0x1dc/0x260
 _regmap_raw_write+0xb4/0x130a
 regmap_raw_write+0x74/0xb0

After applying the patch, the first case is cleared.
But for the case 2, the root cause is not the mt6370_regmap_write() size check. It's in __memcpy() before mt6370_regmap_write().

I'm wondering 'is it reasonable to give the negative value as the size?'

Quoted 10 lines

2) normal register access with negative length

Unable to handle kernel paging request at virtual address ffffffc009cefff2
pc : __memcpy+0x1dc/0x260
lr : _regmap_raw_write_impl+0x6d4/0x828
Call trace:
 __memcpy+0x1dc/0x260
 _regmap_raw_write+0xb4/0x130a
 regmap_raw_write+0x74/0xb0

Quoted 6 lines

After applying the patch, the first case is cleared.
But for the case 2, the root cause is not the mt6370_regmap_write() size check. It's in __memcpy() before mt6370_regmap_write().

I'm wondering 'is it reasonable to give the negative value as the size?'

Thanks for testing!

I'm not sure I understand exactly which code you're talking about. Could you just create a diff with the check for negative just so I can understand where the issue is? We can re-work it into a proper patch from there.

regards,
dan carpenter

Dan Carpenter <[email protected]> 於 2022年10月26日 週三 下午4:51寫道：

Quoted 19 lines · ChiYuan Huang

Quoted 10 lines

2) normal register access with negative length

Unable to handle kernel paging request at virtual address ffffffc009cefff2
pc : __memcpy+0x1dc/0x260
lr : _regmap_raw_write_impl+0x6d4/0x828
Call trace:
 __memcpy+0x1dc/0x260
 _regmap_raw_write+0xb4/0x130a
 regmap_raw_write+0x74/0xb0

Quoted 6 lines

After applying the patch, the first case is cleared.
But for the case 2, the root cause is not the mt6370_regmap_write() size check. It's in __memcpy() before mt6370_regmap_write().

I'm wondering 'is it reasonable to give the negative value as the size?'

Quoted 7 lines

Thanks for testing!

I'm not sure I understand exactly which code you're talking about. Could you just create a diff with the check for negative just so I can understand where the issue is? We can re-work it into a proper patch from there.

Here.
https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/base/regmap/regmap.c#L1860

From my experiment, I try to access 0x00 reg for size (-1). Testing code is like as below regmap_raw_write(regmap, 0, &val, -1);

That's why I think if the size check is needed, it may put into regmap_raw_write() like as regmap_raw_read().

Quoted 3 lines

regards,
dan carpenter

ChiYuan Huang <[email protected]> 於 2022年10月26日 週三 下午5:05寫道：

Quoted 31 lines

Dan Carpenter <[email protected]> 於 2022年10月26日 週三 下午4:51寫道：

Quoted 19 lines · ChiYuan Huang

Quoted 10 lines

2) normal register access with negative length

Unable to handle kernel paging request at virtual address ffffffc009cefff2
pc : __memcpy+0x1dc/0x260
lr : _regmap_raw_write_impl+0x6d4/0x828
Call trace:
 __memcpy+0x1dc/0x260
 _regmap_raw_write+0xb4/0x130a
 regmap_raw_write+0x74/0xb0

Quoted 6 lines

After applying the patch, the first case is cleared.
But for the case 2, the root cause is not the mt6370_regmap_write() size check. It's in __memcpy() before mt6370_regmap_write().

I'm wondering 'is it reasonable to give the negative value as the size?'

Quoted 7 lines

Thanks for testing!

I'm not sure I understand exactly which code you're talking about. Could you just create a diff with the check for negative just so I can understand where the issue is? We can re-work it into a proper patch from there.

Here.
https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/base/regmap/regmap.c#L1860

Quoted 7 lines

From my experiment, I try to access 0x00 reg for size (-1). Testing code is like as below regmap_raw_write(regmap, 0, &val, -1);

That's why I think if the size check is needed, it may put into regmap_raw_write() like as regmap_raw_read().

It seems c99 already said size_t is an unsigned integer type. My experiment for (-1) size is not reasonable.
(-1) means it will be converted as the UINT_MAX or ULONG_MAX. This will cause any unknown error like as memory violation or stack protection,...etc.

let's check whether the negative size is reasonable or not. If this case dost not exist, to keep the boundary check is enough.

Quoted 45 lines

ChiYuan Huang <[email protected]> 於 2022年10月26日 週三 下午5:05寫道：

Quoted 31 lines

Dan Carpenter <[email protected]> 於 2022年10月26日 週三 下午4:51寫道：

Quoted 19 lines · ChiYuan Huang

Quoted 10 lines

2) normal register access with negative length

Unable to handle kernel paging request at virtual address ffffffc009cefff2
pc : __memcpy+0x1dc/0x260
lr : _regmap_raw_write_impl+0x6d4/0x828
Call trace:
 __memcpy+0x1dc/0x260
 _regmap_raw_write+0xb4/0x130a
 regmap_raw_write+0x74/0xb0

Quoted 6 lines

After applying the patch, the first case is cleared.
But for the case 2, the root cause is not the mt6370_regmap_write() size check. It's in __memcpy() before mt6370_regmap_write().

I'm wondering 'is it reasonable to give the negative value as the size?'

Quoted 7 lines

Thanks for testing!

I'm not sure I understand exactly which code you're talking about. Could you just create a diff with the check for negative just so I can understand where the issue is? We can re-work it into a proper patch from there.

Here.
https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/base/regmap/regmap.c#L1860

Quoted 7 lines

From my experiment, I try to access 0x00 reg for size (-1). Testing code is like as below regmap_raw_write(regmap, 0, &val, -1);

That's why I think if the size check is needed, it may put into regmap_raw_write() like as regmap_raw_read().

It seems c99 already said size_t is an unsigned integer type. My experiment for (-1) size is not reasonable.
(-1) means it will be converted as the UINT_MAX or ULONG_MAX. This will cause any unknown error like as memory violation or stack protection,...etc.

Quoted 2 lines

let's check whether the negative size is reasonable or not. If this case dost not exist, to keep the boundary check is enough.

I thought you were testing this from user space but it sounds like you're doing a unit test?

regards,
dan carpenter