diff mbox series

[v2] mfd: mt6370: Add the out-of-bound check to prevent the null pointer

Message ID 1666318661-11777-1-git-send-email-u0084500@gmail.com
State New
Headers
Series [v2] mfd: mt6370: Add the out-of-bound check to prevent the null pointer |

Commit Message

ChiYuan Huang Oct. 21, 2022, 2:17 a.m. UTC 
  From: ChiYuan Huang <cy_huang@richtek.com>

This potential risk could happen at regmap_raw_read() or
regmap_raw_write() when accessing the over-bound register address.

For testing, I try to reproduce it with a testing attribute file.
Below's the issue trace log.

[41.314358] pc : i2c_smbus_xfer+0x58/0x120
[41.314371] lr : i2c_smbus_read_i2c_block_data+0x74/0xc0
[41.399677] Call trace:
[41.402153]  i2c_smbus_xfer+0x58/0x120
[41.405956]  i2c_smbus_read_i2c_block_data+0x74/0xc0
[41.410991]  mt6370_regmap_read+0x40/0x60 [mt6370]
[41.415855]  _regmap_raw_read+0xe4/0x278
[41.419834]  regmap_raw_read+0xec/0x240
[41.423721]  rg_bound_show+0xb0/0x120 [mt6370]
[41.428226]  dev_attr_show+0x3c/0x80
[41.431851]  sysfs_kf_seq_show+0xc4/0x150
[41.435916]  kernfs_seq_show+0x48/0x60
[41.439718]  seq_read_iter+0x11c/0x450
[41.443519]  kernfs_fop_read_iter+0x124/0x1c0
[41.447937]  vfs_read+0x1a8/0x288
[41.451296]  ksys_read+0x74/0x100
[41.454654]  __arm64_sys_read+0x24/0x30
[41.458541]  invoke_syscall+0x54/0x118
[41.462344]  el0_svc_common.constprop.4+0x94/0x128
[41.467202]  do_el0_svc+0x3c/0xd0
[41.470562]  el0_svc+0x20/0x60
[41.473658]  el0t_64_sync_handler+0x94/0xb8
[41.477899]  el0t_64_sync+0x15c/0x160
[41.481614] Code: 54000388 f9401262 aa1303e0 52800041 (f9400042)
[41.487793] ---[ end trace 0000000000000000 ]---

Fixes: b2adf788e603 ("mfd: mt6370: Add MediaTek MT6370 support")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
Dear reviewers:

In v2, I refined the patch title and commit message.

If there's still something improper, please kindly correct me.

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

Comments

Andy Shevchenko Oct. 21, 2022, 8:33 a.m. UTC | #1 
On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:
>
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> This potential risk could happen at regmap_raw_read() or
> regmap_raw_write() when accessing the over-bound register address.
>
> For testing, I try to reproduce it with a testing attribute file.
> Below's the issue trace log.

It looks like you randomly cut the trace.
It's not what I meant and documentation suggests.

> [41.314358] pc : i2c_smbus_xfer+0x58/0x120
> [41.314371] lr : i2c_smbus_read_i2c_block_data+0x74/0xc0
> [41.399677] Call trace:
> [41.402153]  i2c_smbus_xfer+0x58/0x120
> [41.405956]  i2c_smbus_read_i2c_block_data+0x74/0xc0
> [41.410991]  mt6370_regmap_read+0x40/0x60 [mt6370]
> [41.415855]  _regmap_raw_read+0xe4/0x278
> [41.419834]  regmap_raw_read+0xec/0x240
> [41.423721]  rg_bound_show+0xb0/0x120 [mt6370]
> [41.428226]  dev_attr_show+0x3c/0x80
> [41.431851]  sysfs_kf_seq_show+0xc4/0x150
> [41.435916]  kernfs_seq_show+0x48/0x60
> [41.439718]  seq_read_iter+0x11c/0x450
> [41.443519]  kernfs_fop_read_iter+0x124/0x1c0
> [41.447937]  vfs_read+0x1a8/0x288
> [41.451296]  ksys_read+0x74/0x100
> [41.454654]  __arm64_sys_read+0x24/0x30
> [41.458541]  invoke_syscall+0x54/0x118
> [41.462344]  el0_svc_common.constprop.4+0x94/0x128
> [41.467202]  do_el0_svc+0x3c/0xd0
> [41.470562]  el0_svc+0x20/0x60
> [41.473658]  el0t_64_sync_handler+0x94/0xb8
> [41.477899]  el0t_64_sync+0x15c/0x160
> [41.481614] Code: 54000388 f9401262 aa1303e0 52800041 (f9400042)
> [41.487793] ---[ end trace 0000000000000000 ]---

...

> If there's still something improper, please kindly correct me.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
ChiYuan Huang Oct. 21, 2022, 9:02 a.m. UTC | #2 
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午4:34寫道：
>
> On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:
> >
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > This potential risk could happen at regmap_raw_read() or
> > regmap_raw_write() when accessing the over-bound register address.
> >
> > For testing, I try to reproduce it with a testing attribute file.
> > Below's the issue trace log.
>
> It looks like you randomly cut the trace.
> It's not what I meant and documentation suggests.
>
I checked the submitting-patch.rst.
To satisfy the requirement for 70-75 chars per line, I only keep the
important log.

May I ask what you mean for the 'trim'?
Is it 'Still keep 70-75 per line and cut the characters that's over
the limit to the next line"?

> > [41.314358] pc : i2c_smbus_xfer+0x58/0x120
> > [41.314371] lr : i2c_smbus_read_i2c_block_data+0x74/0xc0
> > [41.399677] Call trace:
> > [41.402153]  i2c_smbus_xfer+0x58/0x120
> > [41.405956]  i2c_smbus_read_i2c_block_data+0x74/0xc0
> > [41.410991]  mt6370_regmap_read+0x40/0x60 [mt6370]
> > [41.415855]  _regmap_raw_read+0xe4/0x278
> > [41.419834]  regmap_raw_read+0xec/0x240
> > [41.423721]  rg_bound_show+0xb0/0x120 [mt6370]
> > [41.428226]  dev_attr_show+0x3c/0x80
> > [41.431851]  sysfs_kf_seq_show+0xc4/0x150
> > [41.435916]  kernfs_seq_show+0x48/0x60
> > [41.439718]  seq_read_iter+0x11c/0x450
> > [41.443519]  kernfs_fop_read_iter+0x124/0x1c0
> > [41.447937]  vfs_read+0x1a8/0x288
> > [41.451296]  ksys_read+0x74/0x100
> > [41.454654]  __arm64_sys_read+0x24/0x30
> > [41.458541]  invoke_syscall+0x54/0x118
> > [41.462344]  el0_svc_common.constprop.4+0x94/0x128
> > [41.467202]  do_el0_svc+0x3c/0xd0
> > [41.470562]  el0_svc+0x20/0x60
> > [41.473658]  el0t_64_sync_handler+0x94/0xb8
> > [41.477899]  el0t_64_sync+0x15c/0x160
> > [41.481614] Code: 54000388 f9401262 aa1303e0 52800041 (f9400042)
> > [41.487793] ---[ end trace 0000000000000000 ]---
>
> ...
>
> > If there's still something improper, please kindly correct me.
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
>
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko Oct. 21, 2022, 9:13 a.m. UTC | #3 
On Fri, Oct 21, 2022 at 12:02 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午4:34寫道：
> > On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:

> > It looks like you randomly cut the trace.
> > It's not what I meant and documentation suggests.
> >
> I checked the submitting-patch.rst.
> To satisfy the requirement for 70-75 chars per line, I only keep the
> important log.
>
> May I ask what you mean for the 'trim'?
> Is it 'Still keep 70-75 per line and cut the characters that's over
> the limit to the next line"?

> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages

Have you had a chance to read this section of the document?
ChiYuan Huang Oct. 21, 2022, 9:58 a.m. UTC | #4 
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午5:14寫道：
>
> On Fri, Oct 21, 2022 at 12:02 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午4:34寫道：
> > > On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:
>
> > > It looks like you randomly cut the trace.
> > > It's not what I meant and documentation suggests.
> > >
> > I checked the submitting-patch.rst.
> > To satisfy the requirement for 70-75 chars per line, I only keep the
> > important log.
> >
> > May I ask what you mean for the 'trim'?
> > Is it 'Still keep 70-75 per line and cut the characters that's over
> > the limit to the next line"?
>
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
>
> Have you had a chance to read this section of the document?
>
OK, get it.
Is the below text enough to express this problem?

Ex. Testing as below (mt6370 register range from 0 to 0x1ff)
   rg_bound_show()
        regmap_raw_read(regmap, 0x200, &val, sizeof(val));

pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
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
 _regmap_raw_read+0xe4/0x278
 regmap_raw_read+0xec/0x240
 rg_bound_show+0xb0/0x120
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko Oct. 21, 2022, 10:35 a.m. UTC | #5 
On Fri, Oct 21, 2022 at 12:58 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午5:14寫道：
> > On Fri, Oct 21, 2022 at 12:02 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午4:34寫道：
> > > > On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:

...

> > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> >
> > Have you had a chance to read this section of the document?
> >
> OK, get it.
> Is the below text enough to express this problem?

It is you who decides, because I don't know what exact problem this
will represent.

> Ex. Testing as below (mt6370 register range from 0 to 0x1ff)
>    rg_bound_show()
>         regmap_raw_read(regmap, 0x200, &val, sizeof(val));

> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)

For example, why is this line here?

> 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
>  _regmap_raw_read+0xe4/0x278
>  regmap_raw_read+0xec/0x240
>  rg_bound_show+0xb0/0x120

The rule of thumb is that it's okay to shrink to ~4-5 lines (in most cases).
ChiYuan Huang Oct. 26, 2022, 7:04 a.m. UTC | #6 
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午6:35寫道：
>
> On Fri, Oct 21, 2022 at 12:58 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午5:14寫道：
> > > On Fri, Oct 21, 2022 at 12:02 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年10月21日 週五 下午4:34寫道：
> > > > > On Fri, Oct 21, 2022 at 5:17 AM cy_huang <u0084500@gmail.com> wrote:
>
> ...
>
> > > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> > >
> > > Have you had a chance to read this section of the document?
> > >
> > OK, get it.
> > Is the below text enough to express this problem?
>
> It is you who decides, because I don't know what exact problem this
> will represent.
>
> > Ex. Testing as below (mt6370 register range from 0 to 0x1ff)
> >    rg_bound_show()
> >         regmap_raw_read(regmap, 0x200, &val, sizeof(val));
>
> > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>
> For example, why is this line here?
>
> > 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
> >  _regmap_raw_read+0xe4/0x278
> >  regmap_raw_read+0xec/0x240
> >  rg_bound_show+0xb0/0x120
>
> The rule of thumb is that it's okay to shrink to ~4-5 lines (in most cases).
>

At first, I start this thread due to  the original thread seems stopped
Since it already restarted, please forget this one.
Just follow the restaed thread for the discussion.
https://lore.kernel.org/all/Y1aCiReTZDbPp%2FrS@kadam/

> --
> With Best Regards,
> Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
index cf19cce..acbf960 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 >= MT6370_MAX_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 (bank_idx >= MT6370_MAX_I2C)
+		return -EINVAL;
+
 	return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr,
 					      len, data + MT6370_MAX_ADDRLEN);
 }