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@xxxxxxxxxx>
> ---
> 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
