[PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
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(+)

diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
index cf19cce2fdc0..fd5e1d6a0272 100644
--- a/drivers/mfd/mt6370.c
+++ b/drivers/mfd/mt6370.c
@@ -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);
 }
-- 
2.35.1

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[ChiYuan Huang]

On Mon, Oct 24, 2022 at 03:18:17PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> 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.
> 
Hi,

Before applying the patch, the test result is like as below
1) overbound register access
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
pc : i2c_smbus_xfer+0x58/0x120
lr : i2c_smbus_read_i2c_block_data+0x74/0xc0
Call trace:
 i2c_smbus_xfer+0x58/0x120
 i2c_smbus_read_i2c_block_data+0x74/0xc0
 mt6370_regmap_read+0x40/0x60 [mt6370]
 _regmap_raw_read+0xe4/0x278
 regmap_raw_read+0xec/0x240a

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?'


>  drivers/mfd/mt6370.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> index cf19cce2fdc0..fd5e1d6a0272 100644
> --- a/drivers/mfd/mt6370.c
> +++ b/drivers/mfd/mt6370.c
> @@ -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);
>  }
> -- 
> 2.35.1

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[Dan Carpenter]

On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> 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?'
> 

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

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[ChiYuan Huang]

Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月26日 週三 下午4:51寫道：
>
> On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> > 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?'
> >
>
> 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().

> regards,
> dan carpenter
>

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[ChiYuan Huang]

ChiYuan Huang <u0084500@gmail.com> 於 2022年10月26日 週三 下午5:05寫道：
>
> Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月26日 週三 下午4:51寫道：
> >
> > On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> > > 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?'
> > >
> >
> > 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().
>
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.
> > regards,
> > dan carpenter
> >

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[Dan Carpenter]

On Thu, Oct 27, 2022 at 09:59:46AM +0800, ChiYuan Huang wrote:
> ChiYuan Huang <u0084500@gmail.com> 於 2022年10月26日 週三 下午5:05寫道：
> >
> > Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月26日 週三 下午4:51寫道：
> > >
> > > On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> > > > 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?'
> > > >
> > >
> > > 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().
> >
> 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.

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

regards,
dan carpenter

Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

[ChiYuan Huang]

Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月27日 週四 晚上9:59寫道：
>
> On Thu, Oct 27, 2022 at 09:59:46AM +0800, ChiYuan Huang wrote:
> > ChiYuan Huang <u0084500@gmail.com> 於 2022年10月26日 週三 下午5:05寫道：
> > >
> > > Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月26日 週三 下午4:51寫道：
> > > >
> > > > On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> > > > > 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?'
> > > > >
> > > >
> > > > 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().
> > >
> > 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.
>
> I thought you were testing this from user space but it sounds like
> you're doing a unit test?
>
Yes, with the device attribute to test regmap_raw_read() and regmap_raw_write().
I think It should be the same as the usage in kernel space.
> regards,
> dan carpenter
>