[PATCH] mfd: mt6370: add bounds checking to regmap_read/write functions

[ChiYuan Huang]

On Fri, Sep 09, 2022 at 07:59:49AM +0100, Lee Jones wrote:
> On Thu, 08 Sep 2022, Dan Carpenter wrote:
> 
> > On Thu, Sep 08, 2022 at 07:57:16AM +0100, Lee Jones wrote:
> > > On Mon, 22 Aug 2022, Dan Carpenter wrote:
> > > 
> > > > On Fri, Aug 19, 2022 at 09:27:13AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Aug 19, 2022 at 8:25 AM Dan Carpenter <dan.carpenter at oracle.com> 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")
> > > > > 
> > > > > > From static analysis.  This code is obviously harmless however it may
> > > > > > not be required.  The regmap range checking is slightly complicated and
> > > > > > I haven't remembered where all it's done.
> > > > > 
> > > > > Exactement! I do not think this Fixes anything, I believe you are
> > > > > adding a dead code. So, can you do deeper analysis?
> > > > 
> > > > I spent a long time looking at this code before I sent it and I've
> > > > spent a long time looking at it today.
> > > > 
> > > > Smatch said that these values come from the user, but now it seems
> > > > less clear to me and I have rebuilt the DB so I don't have the same
> > > > information I was looking at earlier.
> > > > 
> > > > So I can't see if these come from the user but neither can I find any
> > > > bounds checking.
> > > 
> > > What's the consensus please?
> > 
> > Let's drop it.  I think it's not required.
> 
> Dropped.
> 
Hi, all:

  I have reproduced this case.
If register access over the bound, it'll cause the NULL pointer.
For the MT6370, the register bank layout is 0 -> TCPC, 1 -> PMU.

>From my experiment, I try to access 0x200, it means bank 2 -> empty

[   41.301385] Hardware name: Raspberry Pi 4 Model B Rev 1.2 (DT)
[   41.307296] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   41.314358] pc : i2c_smbus_xfer+0x58/0x120
[   41.314371] lr : i2c_smbus_read_i2c_block_data+0x74/0xc0
[   41.314376] sp : ffffffc008ceb9a0
[   41.327261] x29: ffffffc008ceb9a0 x28: 0000000000000000 x27: ffffffc008cebb94
[   41.334505] x26: ffffffe529116000 x25: ffffffc008ceba30 x24: 0000000000000008
[   41.334513] x23: 0000000000000000 x22: 0000000000000001 x21: 0000000000000000
[   41.334520] x20: 0000000000000000 x19: ffffff804e1e8010 x18: ffffffe529116538
[   41.348994] x17: 0000000000000000 x16: ffffffe528ae11f0 x15: ffffff804e1be01e
[   41.349002] x14: 0000000000000000 x13: ffffff804e1bd03c x12: 7220303032783020
[   41.370710] x11: 0000000000000000 x10: 000000000000000a x9 : ffffffc008cebb60
[   41.377953] x8 : 0000000000000000 x7 : ffffffe529116538 x6 : ffffffc008ceba30
[   41.385196] x5 : 0000000000000008 x4 : 0000000000000000 x3 : 0000000000000001
[   41.392436] x2 : 0000000000000000 x1 : 0000000000000002 x0 : ffffff804e1e8010
[   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 ]---

I check the APIs regmap_read/write and regmap_raw_read/write.

regmap_read/write -> blocked by boundary check in regmap_readable/writeable
regmap_raw_read/write -> no additional boundary check.

I guess the result for regmap_bulk_read/write is the same as
regmap_raw_read/write.

For this case, it seems mt6360 regmap will also cause this issue.

I'll submit one to fix this.

Hi, Dan:
  Your patch really fix this.
Just one thing need to be confirmed, but it depends on what Lee prefers.

In mt6370.h, we already defiend the enum for the bank boundary
"MT6370_MAX_I2C".

Instead of ARRAY_SIZE(info->i2c), you can also use the enum define.

Anyway, many thanks to report this bug.

ChiYuan Huang.


> -- 
> Lee Jones [李琼斯]
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel