| Message ID | 1424335239-7475-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 19, 2015 at 08:40:39AM +0000, Srinivas Kandagatla wrote: > + /* Check for readable registers before we start */ > + for (i = 0; i < count; i++) > + if (!regmap_readable(map, reg + (i * map->reg_stride))) > + return -EINVAL; That's starting to look pretty expensive especially if what we're looking for is just max_register really... This is one of the reasons we're not religious about checking for readability everywhere, and obviously even if we avoid triggering this particular thing we still have to cope with both the caller and devices that didn't specify readability. A cheaper check for just max_register would be less concerning but it feels like we're trying to paper over a symptom with this rather than fix a problem.
On 19/02/15 10:27, Mark Brown wrote: > On Thu, Feb 19, 2015 at 08:40:39AM +0000, Srinivas Kandagatla wrote: > >> + /* Check for readable registers before we start */ >> + for (i = 0; i < count; i++) >> + if (!regmap_readable(map, reg + (i * map->reg_stride))) >> + return -EINVAL; > > That's starting to look pretty expensive especially if what we're > looking for is just max_register really... This is one of the reasons Yes, I totally agree, this call would be expensive. Initially I had some thing like this, and it works for me. + if (map->max_register && + (reg > map->max_register || + ((reg + (count - 1) * map->reg_stride) > map->max_register))) + return -EINVAL; > we're not religious about checking for readability everywhere, and > obviously even if we avoid triggering this particular thing we still > have to cope with both the caller and devices that didn't specify > readability. A cheaper check for just max_register would be less > concerning but it feels like we're trying to paper over a symptom with > this rather than fix a problem. Yes, just checking max_register would solve the issue for me, I think I over done the patch.. I will resend with just max_register check. >
On Thu, Feb 19, 2015 at 11:04:39AM +0000, Srinivas Kandagatla wrote: > On 19/02/15 10:27, Mark Brown wrote: > >readability. A cheaper check for just max_register would be less > >concerning but it feels like we're trying to paper over a symptom with > >this rather than fix a problem. > Yes, just checking max_register would solve the issue for me, I think I over > done the patch.. I will resend with just max_register check. I'm still not happy with that, it still seems like we're just papering over some other problem here which we should understand before we do anything else. Why are we generating out of bounds accesses in the first place?
On 19/02/15 12:21, Mark Brown wrote: > On Thu, Feb 19, 2015 at 11:04:39AM +0000, Srinivas Kandagatla wrote: >> On 19/02/15 10:27, Mark Brown wrote: > >>> readability. A cheaper check for just max_register would be less >>> concerning but it feels like we're trying to paper over a symptom with >>> this rather than fix a problem. > >> Yes, just checking max_register would solve the issue for me, I think I over >> done the patch.. I will resend with just max_register check. > > I'm still not happy with that, it still seems like we're just papering > over some other problem here which we should understand before we do > anything else. Why are we generating out of bounds accesses in the > first place? The culprit was in my test code, which I eventually fixed. However I would have expected regmap to do some out of bound check before it tries to access the register space. If I try to do an out of bound access via regmap_read()/write() it throws up an error, which is not the same with regmap_bulk_read/write() apis. I was lucky that I got a page fault as the register range was just at page boundary, but in cases where the range is not at page boundary, Its highly likely that it could silently corrupt other memory location( specially in write cases). >
On Thu, Feb 19, 2015 at 01:02:03PM +0000, Srinivas Kandagatla wrote: > The culprit was in my test code, which I eventually fixed. However I would > have expected regmap to do some out of bound check before it tries to access > the register space. > If I try to do an out of bound access via regmap_read()/write() it throws up > an error, which is not the same with regmap_bulk_read/write() apis. > I was lucky that I got a page fault as the register range was just at page > boundary, but in cases where the range is not at page boundary, Its highly > likely that it could silently corrupt other memory location( specially in > write cases). The risk of page faults mostly only applies to memory mapped register maps - most register maps are on other buses where things are a bit less clear, we do often have writes to undocumented registers which aren't included in the readability checks (indeed it's rare for anything to actually give us writability information for the write side). As covered in earlier messages a part of this is a performance tradeoff, it's potentially expensive for us to do the checks on bulk I/O but for single register access it's much cheaper relative to the operation as a whole. It's particularly interesting for MMIO actually as these devices are by far the most performance intensive, we don't have all the costs of the bus to mask what regmap is doing.
On 24/02/15 08:55, Mark Brown wrote: > On Thu, Feb 19, 2015 at 01:02:03PM +0000, Srinivas Kandagatla wrote: > >> The culprit was in my test code, which I eventually fixed. However I would >> have expected regmap to do some out of bound check before it tries to access >> the register space. > >> If I try to do an out of bound access via regmap_read()/write() it throws up >> an error, which is not the same with regmap_bulk_read/write() apis. > >> I was lucky that I got a page fault as the register range was just at page >> boundary, but in cases where the range is not at page boundary, Its highly >> likely that it could silently corrupt other memory location( specially in >> write cases). > > The risk of page faults mostly only applies to memory mapped register > maps - most register maps are on other buses where things are a bit less > clear, we do often have writes to undocumented registers which aren't Yes, my test was on memory mapped registers. > included in the readability checks (indeed it's rare for anything to > actually give us writability information for the write side). As > covered in earlier messages a part of this is a performance tradeoff, > it's potentially expensive for us to do the checks on bulk I/O but for > single register access it's much cheaper relative to the operation as a > whole. I totally agree with you on the performance overhead of checking every read/write, But on the other hand adding a single range check is better than no check with less/nil performance overhead. > > It's particularly interesting for MMIO actually as these devices are by > far the most performance intensive, we don't have all the costs of the > bus to mask what regmap is doing. >
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index d2f8a81..d480e49 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2036,10 +2036,15 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val, { struct regmap_range_node *range; u8 *u8 = map->work_buf; - int ret; + int ret, i, count = val_len/map->format.val_bytes; WARN_ON(!map->bus); + /* Check for readable registers before we start */ + for (i = 0; i < count; i++) + if (!regmap_readable(map, reg + (i * map->reg_stride))) + return -EINVAL; + range = _regmap_range_lookup(map, reg); if (range) { ret = _regmap_select_page(map, ®, range,
regmap_bulk_read() ends up using the path that invokes _regmap_raw_read(), however _regmap_raw_read() never checks if the registers that are accessed are actually readable or within the accessible range. This results in kernel crashes when trying to access registers beyond max_registers. Without this patch I hit below kernel crash: Unable to handle kernel paging request at virtual address f0167000 pgd = ecea0000 [f0167000] *pgd=ad822811, *pte=00000000, *ppte=00000000 Internal error: Oops: 7 [#1] SMP ARM Modules linked in: CPU: 1 PID: 739 Comm: cat Tainted: G L 3.19.0-00008-g1efb3d7-dirty #915 Hardware name: Qualcomm (Flattened Device Tree) task: ecbbd0c0 ti: ec9fc000 task.ti: ec9fc000 PC is at regmap_mmio_read+0xf8/0x138 LR is at irq_work_queue+0x14/0x98 pc : [<c068ab14>] lr : [<c02ed970>] psr: 600f0093 sp : ec9fdd90 ip : 00000001 fp : ec9fddb4 r10: 00001000 r9 : c115a2a8 r8 : edae3940 r7 : edae38c0 r6 : ed9d5000 r5 : 00001000 r4 : 00001000 r3 : f0166000 r2 : 00000007 r1 : 00000000 r0 : 00000019 Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c5787d Table: acea006a DAC: 00000015 Process cat (pid: 739, stack limit = 0xec9fc248) Stack: (0xec9fdd90 to 0xec9fe000) dd80: ed9d5000 edacc400 00000000 eda0a210 dda0: 00001000 00001000 ec9fddfc ec9fddb8 c06854d4 c068aa28 00001000 c109f26c ddc0: c1160148 600f0013 ed9d5000 00001000 ec9fde3c edacc400 00002000 00001000 dde0: 00000001 00001000 00001002 00001000 ec9fde3c ec9fde00 c0685714 c068541c de00: 00000000 ed9d5000 00000000 00000000 ec9fde3c 00001000 edacc400 00000001 de20: ed9d5000 00001000 00000001 00001000 ec9fde7c ec9fde40 c06858b4 c06855ec de40: c0e12d3c ec9fde74 ec9fde6c ec9fde58 c0ade1a8 edacc608 00001000 ed9d5000 de60: 00001000 edacc400 00000000 ed9d6000 ec9fdeb4 ec9fde80 c0900ec8 c0685744 de80: 00001000 00000000 ed9d6000 ed9d5000 00000000 00000000 00001000 00000000 dea0: 00001000 00001000 ec9fdef4 ec9fdeb8 c03a9b60 c0900e58 00001000 00000000 dec0: 00001000 0000a46f ec9fdf44 edfb1900 ec9fdf78 ed9d6000 0001c000 00001000 dee0: 00000000 edfb190c ec9fdf2c ec9fdef8 c03a92c0 c03a9b04 00001000 00000000 df00: 00000000 c0b00d70 0001c000 00010000 ec9fdf78 00010000 ec9fc000 0001c000 df20: ec9fdf44 ec9fdf30 c0347024 c03a9230 ec9fdf78 ec823c00 ec9fdf74 ec9fdf48 df40: c03470e4 c0347008 c03624fc c036246c 00001000 00000000 ec823c00 ec823c00 df60: 00010000 0001c000 ec9fdfa4 ec9fdf78 c03471b4 c0347064 00001000 00000000 df80: 00010000 00001000 0001c000 00000003 c020f2e4 00000000 00000000 ec9fdfa8 dfa0: c020f140 c0347174 00010000 00001000 00000003 0001c000 00010000 0001c000 dfc0: 00010000 00001000 0001c000 00000003 7fffe000 00000001 00000000 00000000 dfe0: 00000000 bef1e5bc 0000b649 b6f29916 600f0030 00000003 00000000 00000000 [<c068ab14>] (regmap_mmio_read) from [<c06854d4>] (_regmap_raw_read+0xc4/0x1d0) [<c06854d4>] (_regmap_raw_read) from [<c0685714>] (regmap_raw_read+0x134/0x158) [<c0685714>] (regmap_raw_read) from [<c06858b4>] (regmap_bulk_read+0x17c/0x1c4) [<c06858b4>] (regmap_bulk_read) from [<c0900ec8>] (bin_attr_eeprom_read+0x7c/0xb4) [<c0900ec8>] (bin_attr_eeprom_read) from [<c03a9b60>] (sysfs_kf_bin_read+0x68/0xa0) [<c03a9b60>] (sysfs_kf_bin_read) from [<c03a92c0>] (kernfs_fop_read+0x9c/0x16c) [<c03a92c0>] (kernfs_fop_read) from [<c0347024>] (__vfs_read+0x28/0x5c) [<c0347024>] (__vfs_read) from [<c03470e4>] (vfs_read+0x8c/0x110) [<c03470e4>] (vfs_read) from [<c03471b4>] (SyS_read+0x4c/0x98) [<c03471b4>] (SyS_read) from [<c020f140>] (ret_fast_syscall+0x0/0x34) Code: eb091d03 e3a00000 e89da9f8 e5973000 (e7d32004) Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/base/regmap/regmap.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)