| Message ID | Y1aCiReTZDbPp/rS@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [resend] mfd: mt6370: add bounds checking to regmap_read/write functions | expand |
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
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
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 >
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 > >
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
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 >
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); }
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(+)