All of lore.kernel.org
 help / color / mirror / Atom feed
From: srinivas.kandagatla@linaro.org (Srinivas Kandagatla)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/2] regmap: Add range check in _regmap_raw_write()
Date: Thu, 19 Feb 2015 11:11:58 +0000	[thread overview]
Message-ID: <54E5C4FE.5080604@linaro.org> (raw)
In-Reply-To: <20150219103156.GD3198@finisterre.sirena.org.uk>



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

WARNING: multiple messages have this Message-ID (diff)
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Mark Brown <broonie@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] regmap: Add range check in _regmap_raw_write()
Date: Thu, 19 Feb 2015 11:11:58 +0000	[thread overview]
Message-ID: <54E5C4FE.5080604@linaro.org> (raw)
In-Reply-To: <20150219103156.GD3198@finisterre.sirena.org.uk>



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

  reply	other threads:[~2015-02-19 11:11 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-19  8:39 [PATCH 0/2] regmap: fix range checks in _regmap_raw_read/write() Srinivas Kandagatla
2015-02-19  8:39 ` Srinivas Kandagatla
2015-02-19  8:40 ` [PATCH 1/2] regmap: Add range check in _regmap_raw_read() Srinivas Kandagatla
2015-02-19  8:40   ` Srinivas Kandagatla
2015-02-19 10:27   ` Mark Brown
2015-02-19 10:27     ` Mark Brown
2015-02-19 11:04     ` Srinivas Kandagatla
2015-02-19 11:04       ` Srinivas Kandagatla
2015-02-19 12:21       ` Mark Brown
2015-02-19 12:21         ` Mark Brown
2015-02-19 13:02         ` Srinivas Kandagatla
2015-02-19 13:02           ` Srinivas Kandagatla
2015-02-24  8:55           ` Mark Brown
2015-02-24  8:55             ` Mark Brown
2015-02-24  9:12             ` Srinivas Kandagatla
2015-02-24  9:12               ` Srinivas Kandagatla
2015-02-19  8:40 ` [PATCH 2/2] regmap: Add range check in _regmap_raw_write() Srinivas Kandagatla
2015-02-19  8:40   ` Srinivas Kandagatla
2015-02-19 10:31   ` Mark Brown
2015-02-19 10:31     ` Mark Brown
2015-02-19 11:11     ` Srinivas Kandagatla [this message]
2015-02-19 11:11       ` Srinivas Kandagatla
2015-02-19 11:55       ` Mark Brown
2015-02-19 11:55         ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54E5C4FE.5080604@linaro.org \
    --to=srinivas.kandagatla@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.