[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 at 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