| Message ID | 1666318661-11777-1-git-send-email-u0084500@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mfd: mt6370: Add the out-of-bound check to prevent the null pointer | expand |
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
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
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?
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
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).
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 --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); }