omap: pm: Fix overflow when doing powerdomain deps lookups.

PATCH In review Mike Chan 3 participants 6 messages Aug 11, 2009 – Aug 12, 2009

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Mike Chan

author

2009-08-11 02:00 UTC #0 lore raw

PATCH 1/1 omap: pm: Fix overflow when doing powerdomain deps lookups.

Signed-off-by Mike Chan <[email protected]>

---
 arch/arm/mach-omap2/powerdomain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-omap2/powerdomain.c +1 -1

index 0334609..6077629 100644
@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
 	if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
 		return ERR_PTR(-EINVAL);
 
-	for (pd = deps; pd; pd++) {
+	for (pd = deps; pd->pwrdm_name; pd++) {
 
 		if (!omap_chip_is(pd->omap_chip))
 			continue;

Signature · 1 line

1.5.4.5

Kevin Hilman

2009-08-11 14:38 UTC #1 lore raw

Quoted 10 lines · Mike Chan

Signed-off-by Mike Chan <[email protected]>
---
 arch/arm/mach-omap2/powerdomain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/powerdomain.c +0 -0
index 0334609..6077629 100644

arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 93

Quoted 5 lines

arch/arm/mach-omap2/powerdomain.c +1 -1

@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
 	if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
 		return ERR_PTR(-EINVAL);
 
-	for (pd = deps; pd; pd++) {
+	for (pd = deps; pd->pwrdm_name; pd++) {

Maybe should be:

	for (pd = deps; pd && pd->pwrdm_name; pd++) {

arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 96

Quoted 3 lines

arch/arm/mach-omap2/powerdomain.c +0 -0
 		if (!omap_chip_is(pd->omap_chip))
 			continue;

Also, a descriptive changelog would be helpful here describing the conditions where you saw overflow etc.

Kevin

Mike Chan

author

2009-08-11 18:11 UTC #2 lore raw

Quoted 15 lines · Kevin Hilman

Quoted 13 lines · Mike Chan

Signed-off-by Mike Chan <[email protected]>

---
arch/arm/mach-omap2/powerdomain.c | 2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-omap2/powerdomain.c +0 -0

index 0334609..6077629 100644
@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
       if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
               return ERR_PTR(-EINVAL);

Quoted 7 lines

arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 93
Quoted 2 lines
arch/arm/mach-omap2/powerdomain.c +1 -1
-     for (pd = deps; pd; pd++) {
+     for (pd = deps; pd->pwrdm_name; pd++) {
Maybe should be:
for (pd = deps; pd && pd->pwrdm_name; pd++) {

At the end of the list pd is a pointer to a NULL struct, so checking if the address == NULL doesn't help here. In fact the original code will just keep running past the struct to read who knows what in memory.

This case manifests itself when from clkdms_setup() when enabling auto idle for a clock domain and the clockdomain usecount is greater than
0. When _clkdm_add_autodeps() tries to add the a dependency that does not exist in the powerdomain->wkdep_srcs array the for loop will run past the wkdep_srcs array.

Currently in linux-omap you won't hit this because the not found case is never executed, unless you start modifying powerdomains and their wakeup/sleep deps.

--Mike

Quoted 11 lines

?
arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 96
Quoted 3 lines
arch/arm/mach-omap2/powerdomain.c +0 -0
               if (!omap_chip_is(pd->omap_chip))
                       continue;
Also, a descriptive changelog would be helpful here describing the conditions where you saw overflow etc.
Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Hilman

2009-08-12 00:14 UTC #3 lore raw

Quoted 22 lines · Mike Chan

Quoted 20 lines · Kevin Hilman
Quoted 10 lines · Mike Chan
Signed-off-by Mike Chan <[email protected]>
---
arch/arm/mach-omap2/powerdomain.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/powerdomain.c +0 -0
index 0334609..6077629 100644
arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 93
Quoted 5 lines
arch/arm/mach-omap2/powerdomain.c +1 -1
@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
       if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
               return ERR_PTR(-EINVAL);
 
-     for (pd = deps; pd; pd++) {
+     for (pd = deps; pd->pwrdm_name; pd++) {
Maybe should be:

Quoted 7 lines

Quoted 2 lines
for (pd = deps; pd && pd->pwrdm_name; pd++) {
At the end of the list pd is a pointer to a NULL struct, so checking if the address == NULL doesn't help here. In fact the original code will just keep running past the struct to read who knows what in memory.

I see that now, didn't notice the NULL struct terminators.

Quoted 5 lines

This case manifests itself when from clkdms_setup() when enabling auto idle for a clock domain and the clockdomain usecount is greater than
0. When _clkdm_add_autodeps() tries to add the a dependency that does not exist in the powerdomain->wkdep_srcs array the for loop will run past the wkdep_srcs array.

Thanks, will add this as changelog and push to PM branch as well as pm-2.6.29.

Kevin

Quoted 4 lines

Currently in linux-omap you won't hit this because the not found case is never executed, unless you start modifying powerdomains and their wakeup/sleep deps.

Quoted 13 lines

--Mike
Quoted 11 lines
?
arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 96
Quoted 3 lines
arch/arm/mach-omap2/powerdomain.c +0 -0
               if (!omap_chip_is(pd->omap_chip))
                       continue;
Also, a descriptive changelog would be helpful here describing the conditions where you saw overflow etc.
Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Paul Walmsley

2009-08-12 06:32 UTC #4 lore raw

In reply to Mike Chan

Hi Mike,

Quoted 36 lines · Mike Chan

Quoted 15 lines · Kevin Hilman
Quoted 13 lines · Mike Chan
Signed-off-by Mike Chan <[email protected]>
---
arch/arm/mach-omap2/powerdomain.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/powerdomain.c +0 -0
index 0334609..6077629 100644
@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
       if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
               return ERR_PTR(-EINVAL);
 
Quoted 7 lines
arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 93
Quoted 2 lines
arch/arm/mach-omap2/powerdomain.c +1 -1
-     for (pd = deps; pd; pd++) {
+     for (pd = deps; pd->pwrdm_name; pd++) {
Maybe should be:
for (pd = deps; pd && pd->pwrdm_name; pd++) {
At the end of the list pd is a pointer to a NULL struct, so checking if the address == NULL doesn't help here. In fact the original code will just keep running past the struct to read who knows what in memory.
This case manifests itself when from clkdms_setup() when enabling auto idle for a clock domain and the clockdomain usecount is greater than
0. When _clkdm_add_autodeps() tries to add the a dependency that does not exist in the powerdomain->wkdep_srcs array the for loop will run past the wkdep_srcs array.

Quoted 5 lines

Currently in linux-omap you won't hit this because the not found case is never executed, unless you start modifying powerdomains and their wakeup/sleep deps.
--Mike

Thanks for the patch, this is correct. But the patch also should modify:

        if (!pd)
                return ERR_PTR(-ENOENT);

after the loop to be

	if (!pd->pwrdm_name)
		...

Could you revise this and resend?

thanks

- Paul

Mike Chan

author

2009-08-12 17:58 UTC #5 lore raw

Quoted 51 lines · Paul Walmsley

Hi Mike,
Quoted 36 lines · Mike Chan
Quoted 15 lines · Kevin Hilman
Quoted 13 lines · Mike Chan
Signed-off-by Mike Chan <[email protected]>
---
arch/arm/mach-omap2/powerdomain.c | 2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/powerdomain.c +0 -0
index 0334609..6077629 100644
@@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm,
       if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip))
               return ERR_PTR(-EINVAL);
 
Quoted 7 lines
arch/arm/mach-omap2/powerdomain.c static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, · line 93
Quoted 2 lines
arch/arm/mach-omap2/powerdomain.c +1 -1
-     for (pd = deps; pd; pd++) {
+     for (pd = deps; pd->pwrdm_name; pd++) {
Maybe should be:
for (pd = deps; pd && pd->pwrdm_name; pd++) {
At the end of the list pd is a pointer to a NULL struct, so checking if the address == NULL doesn't help here. In fact the original code will just keep running past the struct to read who knows what in memory.
This case manifests itself when from clkdms_setup() when enabling auto idle for a clock domain and the clockdomain usecount is greater than
0. When _clkdm_add_autodeps() tries to add the a dependency that does not exist in the powerdomain->wkdep_srcs array the for loop will run past the wkdep_srcs array.
Quoted 5 lines
Currently in linux-omap you won't hit this because the not found case is never executed, unless you start modifying powerdomains and their wakeup/sleep deps.
--Mike
Thanks for the patch, this is correct. But the patch also should modify:
if (!pd)
return ERR_PTR(-ENOENT);

Quoted 9 lines

after the loop to be
if (!pd->pwrdm_name)
...
Could you revise this and resend?
thanks

Good catch, looks like this was already merged into the omap-pm branch :/ Sending a followup patch fine with everyone?

Quoted 2 lines

- Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html