| Message ID | Yv8ezribLQbq5ggv@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mfd: mt6370: add bounds checking to regmap_read/write functions | expand |
On Fri, Aug 19, 2022 at 8:25 AM Dan Carpenter <dan.carpenter@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?
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@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. regards, dan carpenter
On Fri, Aug 19, 2022 at 8:25 AM Dan Carpenter <dan.carpenter@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. ... > + if (bank_idx >= ARRAY_SIZE(info->i2c)) Okay, the index of the bank comes from arbitrary data and here you want to prevent it from overflowing. > + return -EINVAL; ... > + if (len < 0 || bank_idx >= ARRAY_SIZE(info->i2c)) > + return -EINVAL; Ditto here. But what I would do differently is a check for len. Instead split the assignment and do a check beforehand. unsigned int len; if (count < MT6370_MAX_ADDRLEN) return -EINVAL; len = count - MT6370_MAX_ADDRLEN;
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@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?
On Thu, Sep 8, 2022 at 9:57 AM Lee Jones <lee@kernel.org> wrote: > On Mon, 22 Aug 2022, Dan Carpenter wrote: > > On Fri, Aug 19, 2022 at 09:27:13AM +0300, Andy Shevchenko wrote: ... > > 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? From my point of view it's not needed, and we may fix it later when a real problem happens / revised analysis done. But OTOH it is harmless, if you think it worth applying. I have no objections to the way Dan and you choose.
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@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. regards, dan carpenter
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@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.
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@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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> --- 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. drivers/mfd/mt6370.c | 6 ++++++ 1 file changed, 6 insertions(+)