[PATCH 0/2] regmap: fix range checks in _regmap_raw_read/write()

[PATCH 0/2] regmap: fix range checks in _regmap_raw_read/write()

[Srinivas Kandagatla]

These two patches attempts to fix range checks in _regmap_raw_read()
and _regmap_raw_write() functions. Without these patches I hit kernel
crash when I attempt to dump the mmio regmap range using regmap_bulk_read().


Srinivas Kandagatla (2):
  regmap: Add range check in _regmap_raw_read()
  regmap: Add range check in _regmap_raw_write()

 drivers/base/regmap/regmap.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Srinivas Kandagatla]

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(-)

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, &reg, range,
-- 
1.9.1

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150219/11ae3d1f/attachment.sig>

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Srinivas Kandagatla]

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.


>

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Mark Brown]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150219/43da5572/attachment.sig>

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Srinivas Kandagatla]

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).



>

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150224/4ce5ec1d/attachment.sig>

[PATCH 1/2] regmap: Add range check in _regmap_raw_read()

[Srinivas Kandagatla]

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.
>

[PATCH 2/2] regmap: Add range check in _regmap_raw_write()

[Srinivas Kandagatla]

regmap_bulk_write() ends up using the path that invokes _regmap_raw_write(),
however _regmap_raw_write() never checks if the registers that are accessed
are actually within the accessible range. This results in kernel crashes when
trying to access registers beyond max_registers.

This patch just adds check before accessing the register range.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/base/regmap/regmap.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d480e49..32ce7b3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1202,16 +1202,14 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg,
 	void *buf;
 	int ret = -ENOTSUPP;
 	size_t len;
-	int i;
+	int i, count = val_len/map->format.val_bytes;
 
 	WARN_ON(!map->bus);
 
 	/* Check for unwritable registers before we start */
-	if (map->writeable_reg)
-		for (i = 0; i < val_len / map->format.val_bytes; i++)
-			if (!map->writeable_reg(map->dev,
-						reg + (i * map->reg_stride)))
-				return -EINVAL;
+	for (i = 0; i < count; i++)
+		if (!regmap_writeable(map, reg + (i * map->reg_stride)))
+			return -EINVAL;
 
 	if (!map->cache_bypass && map->format.parse_val) {
 		unsigned int ival;
-- 
1.9.1

[PATCH 2/2] regmap: Add range check in _regmap_raw_write()

[Mark Brown]

On Thu, Feb 19, 2015 at 08:40:55AM +0000, Srinivas Kandagatla wrote:
> regmap_bulk_write() ends up using the path that invokes _regmap_raw_write(),
> however _regmap_raw_write() never checks if the registers that are accessed
> are actually within the accessible range. This results in kernel crashes when
> trying to access registers beyond max_registers.
> 
> This patch just adds check before accessing the register range.

>  	/* Check for unwritable registers before we start */
> -	if (map->writeable_reg)
> -		for (i = 0; i < val_len / map->format.val_bytes; i++)
> -			if (!map->writeable_reg(map->dev,
> -						reg + (i * map->reg_stride)))
> -				return -EINVAL;
> +	for (i = 0; i < count; i++)
> +		if (!regmap_writeable(map, reg + (i * map->reg_stride)))
> +			return -EINVAL;

Your changelog doesn't correspond to what the code is actually doing
here...  what you're actually doing here is replacing an open coding of
regmap_writeable() with calls to the function.

The same papering over the cracks concerns do apply here as well, it's
not immediately obvious that this is a good fix for the issue you
describe.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150219/dd996c3e/attachment.sig>

[PATCH 2/2] regmap: Add range check in _regmap_raw_write()

[Srinivas Kandagatla]

On 19/02/15 10:31, Mark Brown wrote:
> On Thu, Feb 19, 2015 at 08:40:55AM +0000, Srinivas Kandagatla wrote:
>> regmap_bulk_write() ends up using the path that invokes _regmap_raw_write(),
>> however _regmap_raw_write() never checks if the registers that are accessed
>> are actually within the accessible range. This results in kernel crashes when
>> trying to access registers beyond max_registers.
>>
>> This patch just adds check before accessing the register range.
>
>>   	/* Check for unwritable registers before we start */
>> -	if (map->writeable_reg)
>> -		for (i = 0; i < val_len / map->format.val_bytes; i++)
>> -			if (!map->writeable_reg(map->dev,
>> -						reg + (i * map->reg_stride)))
>> -				return -EINVAL;
>> +	for (i = 0; i < count; i++)
>> +		if (!regmap_writeable(map, reg + (i * map->reg_stride)))
>> +			return -EINVAL;
>
> Your changelog doesn't correspond to what the code is actually doing
> here...  what you're actually doing here is replacing an open coding of
> regmap_writeable() with calls to the function.
>
> The same papering over the cracks concerns do apply here as well, it's
> not immediately obvious that this is a good fix for the issue you
> describe.
Only reason for me to send this patch was that fact that 
_regmap_raw_write() also suffers from same issue as _regmap_raw_read(), 
which is "access beyond max_register".

Should I drop this patch?
Or
Adding similar check of max_register before the writing makes sense?


--srini

[PATCH 2/2] regmap: Add range check in _regmap_raw_write()

[Mark Brown]

On Thu, Feb 19, 2015 at 11:11:58AM +0000, Srinivas Kandagatla wrote:
> On 19/02/15 10:31, Mark Brown wrote:

> >Your changelog doesn't correspond to what the code is actually doing
> >here...  what you're actually doing here is replacing an open coding of
> >regmap_writeable() with calls to the function.

> Only reason for me to send this patch was that fact that _regmap_raw_write()
> also suffers from same issue as _regmap_raw_read(), which is "access beyond
> max_register".

> Should I drop this patch?
> Or
> Adding similar check of max_register before the writing makes sense?

No, please go and reread your patch - neither your description above nor
the changelog match up with what it's actually doing.  It looks like a
perfectly sensible patch, it just doesn't do what you're saying it does.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150219/4a1a2b94/attachment.sig>