[PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Mike Chan]

Signed-off-by: Mike Chan <mike@android.com>
---
 arch/arm/mach-omap2/powerdomain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 0334609..6077629 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -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;
-- 
1.5.4.5

Re: [PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 0334609..6077629 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -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++) {

?

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

Re: [PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Mike Chan]

On Tue, Aug 11, 2009 at 7:38 AM, Kevin
Hilman<khilman@deeprootsystems.com> wrote:
> Mike Chan <mike@android.com> writes:
>
>> Signed-off-by: Mike Chan <mike@android.com>
>> ---
>>  arch/arm/mach-omap2/powerdomain.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>> index 0334609..6077629 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -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++) {
>

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

> ?
>
>>
>>               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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> On Tue, Aug 11, 2009 at 7:38 AM, Kevin
> Hilman<khilman@deeprootsystems.com> wrote:
>> Mike Chan <mike@android.com> writes:
>>
>>> Signed-off-by: Mike Chan <mike@android.com>
>>> ---
>>>  arch/arm/mach-omap2/powerdomain.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>>> index 0334609..6077629 100644
>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>> @@ -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++) {
>>
>
> 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.

> 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

> 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
>
>> ?
>>
>>>
>>>               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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1969 bytes --]

Hi Mike,

On Tue, 11 Aug 2009, Mike Chan wrote:

> On Tue, Aug 11, 2009 at 7:38 AM, Kevin
> Hilman<khilman@deeprootsystems.com> wrote:
> > Mike Chan <mike@android.com> writes:
> >
> >> Signed-off-by: Mike Chan <mike@android.com>
> >> ---
> >>  arch/arm/mach-omap2/powerdomain.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> >> index 0334609..6077629 100644
> >> --- a/arch/arm/mach-omap2/powerdomain.c
> >> +++ b/arch/arm/mach-omap2/powerdomain.c
> >> @@ -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++) {
> >
> 
> 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

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

Re: [PATCH] omap: pm: Fix overflow when doing powerdomain deps lookups.

[Mike Chan]

On Tue, Aug 11, 2009 at 11:32 PM, Paul Walmsley<paul@pwsan.com> wrote:
> Hi Mike,
>
> On Tue, 11 Aug 2009, Mike Chan wrote:
>
>> On Tue, Aug 11, 2009 at 7:38 AM, Kevin
>> Hilman<khilman@deeprootsystems.com> wrote:
>> > Mike Chan <mike@android.com> writes:
>> >
>> >> Signed-off-by: Mike Chan <mike@android.com>
>> >> ---
>> >>  arch/arm/mach-omap2/powerdomain.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>> >> index 0334609..6077629 100644
>> >> --- a/arch/arm/mach-omap2/powerdomain.c
>> >> +++ b/arch/arm/mach-omap2/powerdomain.c
>> >> @@ -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++) {
>> >
>>
>> 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
>
> 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
>

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

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