linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
@ 2021-02-23 19:38 Colin King
  2021-02-23 19:52 ` Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Colin King @ 2021-02-23 19:38 UTC (permalink / raw)
  To: Roger Quadros, Tony Lindgren, Krzysztof Kozlowski, linux-omap
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

Currently the array gpmc_cs is indexed by cs before it cs is range checked
and the pointer read from this out-of-index read is dereferenced. Fix this
by performing the range check on cs before the read and the following
pointer dereference.

Addresses-Coverity: ("Negative array index read")
Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/memory/omap-gpmc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index cfa730cfd145..f80c2ea39ca4 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
 
 void gpmc_cs_free(int cs)
 {
-	struct gpmc_cs_data *gpmc = &gpmc_cs[cs];
-	struct resource *res = &gpmc->mem;
+	struct gpmc_cs_data *gpmc;
+	struct resource *res;
 
 	spin_lock(&gpmc_mem_lock);
 	if (cs >= gpmc_cs_num || cs < 0 || !gpmc_cs_reserved(cs)) {
@@ -1018,6 +1018,9 @@ void gpmc_cs_free(int cs)
 		spin_unlock(&gpmc_mem_lock);
 		return;
 	}
+	gpmc = &gpmc_cs[cs];
+	res = &gpmc->mem;
+
 	gpmc_cs_disable_mem(cs);
 	if (res->flags)
 		release_resource(res);
-- 
2.30.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-23 19:38 [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[] Colin King
@ 2021-02-23 19:52 ` Krzysztof Kozlowski
  2021-02-24  7:55 ` Dan Carpenter
  2021-03-02  8:44 ` Krzysztof Kozlowski
  2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-02-23 19:52 UTC (permalink / raw)
  To: Colin King
  Cc: Roger Quadros, Tony Lindgren, linux-omap, kernel-janitors, linux-kernel

On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the array gpmc_cs is indexed by cs before it cs is range checked
> and the pointer read from this out-of-index read is dereferenced. Fix this
> by performing the range check on cs before the read and the following
> pointer dereference.
> 
> Addresses-Coverity: ("Negative array index read")
> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/memory/omap-gpmc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Thanks, looks good. I'll take it after merge window.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-23 19:38 [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[] Colin King
  2021-02-23 19:52 ` Krzysztof Kozlowski
@ 2021-02-24  7:55 ` Dan Carpenter
  2021-02-24  8:11   ` Krzysztof Kozlowski
  2021-02-24  8:46   ` Colin Ian King
  2021-03-02  8:44 ` Krzysztof Kozlowski
  2 siblings, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2021-02-24  7:55 UTC (permalink / raw)
  To: Colin King
  Cc: Roger Quadros, Tony Lindgren, Krzysztof Kozlowski, linux-omap,
	kernel-janitors, linux-kernel

On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the array gpmc_cs is indexed by cs before it cs is range checked
> and the pointer read from this out-of-index read is dereferenced. Fix this
> by performing the range check on cs before the read and the following
> pointer dereference.
> 
> Addresses-Coverity: ("Negative array index read")
> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/memory/omap-gpmc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index cfa730cfd145..f80c2ea39ca4 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
>  
>  void gpmc_cs_free(int cs)
>  {
> -	struct gpmc_cs_data *gpmc = &gpmc_cs[cs];
> -	struct resource *res = &gpmc->mem;

There is no actual dereferencing going on here, it just taking the
addresses.  But the patch is also harmless and improves readability.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-24  7:55 ` Dan Carpenter
@ 2021-02-24  8:11   ` Krzysztof Kozlowski
  2021-02-25 12:37     ` Tony Lindgren
  2021-02-24  8:46   ` Colin Ian King
  1 sibling, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-02-24  8:11 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Colin King, Roger Quadros, Tony Lindgren, linux-omap,
	kernel-janitors, linux-kernel

On Wed, Feb 24, 2021 at 10:55:52AM +0300, Dan Carpenter wrote:
> On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently the array gpmc_cs is indexed by cs before it cs is range checked
> > and the pointer read from this out-of-index read is dereferenced. Fix this
> > by performing the range check on cs before the read and the following
> > pointer dereference.
> > 
> > Addresses-Coverity: ("Negative array index read")
> > Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/memory/omap-gpmc.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > index cfa730cfd145..f80c2ea39ca4 100644
> > --- a/drivers/memory/omap-gpmc.c
> > +++ b/drivers/memory/omap-gpmc.c
> > @@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
> >  
> >  void gpmc_cs_free(int cs)
> >  {
> > -	struct gpmc_cs_data *gpmc = &gpmc_cs[cs];
> > -	struct resource *res = &gpmc->mem;
> 
> There is no actual dereferencing going on here, it just taking the
> addresses.  But the patch is also harmless and improves readability.

Hm, in the second line indeed the compiler will just calculate the
offset of "mem" field against the "gpmc_cs+cs" and return it's probable
address.

To me still the code is a little bit non-obvious or obfuscated - first
you play with the array[index] and then you check the validity of index.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-24  7:55 ` Dan Carpenter
  2021-02-24  8:11   ` Krzysztof Kozlowski
@ 2021-02-24  8:46   ` Colin Ian King
  1 sibling, 0 replies; 8+ messages in thread
From: Colin Ian King @ 2021-02-24  8:46 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Roger Quadros, Tony Lindgren, Krzysztof Kozlowski, linux-omap,
	kernel-janitors, linux-kernel

On 24/02/2021 07:55, Dan Carpenter wrote:
> On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently the array gpmc_cs is indexed by cs before it cs is range checked
>> and the pointer read from this out-of-index read is dereferenced. Fix this
>> by performing the range check on cs before the read and the following
>> pointer dereference.
>>
>> Addresses-Coverity: ("Negative array index read")
>> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/memory/omap-gpmc.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>> index cfa730cfd145..f80c2ea39ca4 100644
>> --- a/drivers/memory/omap-gpmc.c
>> +++ b/drivers/memory/omap-gpmc.c
>> @@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
>>  
>>  void gpmc_cs_free(int cs)
>>  {
>> -	struct gpmc_cs_data *gpmc = &gpmc_cs[cs];
>> -	struct resource *res = &gpmc->mem;
> > There is no actual dereferencing going on here, it just taking the
> addresses.  But the patch is also harmless and improves readability.

Plus compilers are getting smarter with static analysis so some day in
the future they will warn about this.


> 
> regards,
> dan carpenter
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-24  8:11   ` Krzysztof Kozlowski
@ 2021-02-25 12:37     ` Tony Lindgren
  0 siblings, 0 replies; 8+ messages in thread
From: Tony Lindgren @ 2021-02-25 12:37 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Dan Carpenter, Colin King, Roger Quadros, linux-omap,
	kernel-janitors, linux-kernel

* Krzysztof Kozlowski <krzk@kernel.org> [210224 08:11]:
> On Wed, Feb 24, 2021 at 10:55:52AM +0300, Dan Carpenter wrote:
> > On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Currently the array gpmc_cs is indexed by cs before it cs is range checked
> > > and the pointer read from this out-of-index read is dereferenced. Fix this
> > > by performing the range check on cs before the read and the following
> > > pointer dereference.
> > > 
> > > Addresses-Coverity: ("Negative array index read")
> > > Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  drivers/memory/omap-gpmc.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > > index cfa730cfd145..f80c2ea39ca4 100644
> > > --- a/drivers/memory/omap-gpmc.c
> > > +++ b/drivers/memory/omap-gpmc.c
> > > @@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
> > >  
> > >  void gpmc_cs_free(int cs)
> > >  {
> > > -	struct gpmc_cs_data *gpmc = &gpmc_cs[cs];
> > > -	struct resource *res = &gpmc->mem;
> > 
> > There is no actual dereferencing going on here, it just taking the
> > addresses.  But the patch is also harmless and improves readability.
> 
> Hm, in the second line indeed the compiler will just calculate the
> offset of "mem" field against the "gpmc_cs+cs" and return it's probable
> address.
> 
> To me still the code is a little bit non-obvious or obfuscated - first
> you play with the array[index] and then you check the validity of index.

To me it seems the fixes tag is not ideal.. Seems this issue was there
earlier too before moving the code. In any case:

Reviewed-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-02-23 19:38 [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[] Colin King
  2021-02-23 19:52 ` Krzysztof Kozlowski
  2021-02-24  7:55 ` Dan Carpenter
@ 2021-03-02  8:44 ` Krzysztof Kozlowski
  2021-03-02  8:54   ` Colin Ian King
  2 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-03-02  8:44 UTC (permalink / raw)
  To: Colin King
  Cc: Roger Quadros, Tony Lindgren, linux-omap, kernel-janitors, linux-kernel

On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the array gpmc_cs is indexed by cs before it cs is range checked
> and the pointer read from this out-of-index read is dereferenced. Fix this
> by performing the range check on cs before the read and the following
> pointer dereference.
> 
> Addresses-Coverity: ("Negative array index read")
> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/memory/omap-gpmc.c | 7 +++++--

Thanks, applied with Tony's ack and changed Fixes to 9ed7a776eb50 ("ARM:
OMAP2+: Fix support for multiple devices on a GPMC chip select"). 

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
  2021-03-02  8:44 ` Krzysztof Kozlowski
@ 2021-03-02  8:54   ` Colin Ian King
  0 siblings, 0 replies; 8+ messages in thread
From: Colin Ian King @ 2021-03-02  8:54 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Roger Quadros, Tony Lindgren, linux-omap, kernel-janitors, linux-kernel

On 02/03/2021 08:44, Krzysztof Kozlowski wrote:
> On Tue, Feb 23, 2021 at 07:38:21PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently the array gpmc_cs is indexed by cs before it cs is range checked
>> and the pointer read from this out-of-index read is dereferenced. Fix this
>> by performing the range check on cs before the read and the following
>> pointer dereference.
>>
>> Addresses-Coverity: ("Negative array index read")
>> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under drivers")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/memory/omap-gpmc.c | 7 +++++--
> 
> Thanks, applied with Tony's ack and changed Fixes to 9ed7a776eb50 ("ARM:
> OMAP2+: Fix support for multiple devices on a GPMC chip select"). 


Thanks for correcting the Fixes line

> 
> Best regards,
> Krzysztof
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-03  1:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 19:38 [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[] Colin King
2021-02-23 19:52 ` Krzysztof Kozlowski
2021-02-24  7:55 ` Dan Carpenter
2021-02-24  8:11   ` Krzysztof Kozlowski
2021-02-25 12:37     ` Tony Lindgren
2021-02-24  8:46   ` Colin Ian King
2021-03-02  8:44 ` Krzysztof Kozlowski
2021-03-02  8:54   ` Colin Ian King

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox