2024-05-01 19:51:29

by Yun Levi

[permalink] [raw]
Subject: [PATCH] time/timgr: Fix wrong reference when level 0 group allocation failed

When tmigr_setup_groups() failed level 0 group allocation,
next do while loop refers wrong local stack array location.

Changing group init do while loop with while loop to fix this problem.

Signed-off-by: Levi Yun <[email protected]>
---
kernel/time/timer_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index ccba875d2234..84413114db5c 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)

} while (i < tmigr_hierarchy_levels);

- do {
+ while (i > 0) {
group = stack[--i];

if (err < 0) {
@@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
tmigr_connect_child_parent(child, group);
}
}
- } while (i > 0);
+ }

kfree(stack);

--
2.41.0



2024-05-04 04:47:40

by Yun Levi

[permalink] [raw]
Subject: Re: [PATCH] time/timgr: Fix wrong reference when level 0 group allocation failed

Gentle ping

On Wed, May 1, 2024 at 8:51 PM Levi Yun <[email protected]> wrote:
>
> When tmigr_setup_groups() failed level 0 group allocation,
> next do while loop refers wrong local stack array location.
>
> Changing group init do while loop with while loop to fix this problem.
>
> Signed-off-by: Levi Yun <[email protected]>
> ---
> kernel/time/timer_migration.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
> index ccba875d2234..84413114db5c 100644
> --- a/kernel/time/timer_migration.c
> +++ b/kernel/time/timer_migration.c
> @@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
>
> } while (i < tmigr_hierarchy_levels);
>
> - do {
> + while (i > 0) {
> group = stack[--i];
>
> if (err < 0) {
> @@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
> tmigr_connect_child_parent(child, group);
> }
> }
> - } while (i > 0);
> + }
>
> kfree(stack);
>
> --
> 2.41.0
>

2024-05-04 10:10:21

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH] time/timgr: Fix wrong reference when level 0 group allocation failed

> Changing group init do while loop with while loop to fix this problem.

* Please convert such a change description to an improved imperative wording.

* Would you like to add the tag “Fixes” accordingly?


Regards,
Markus

2024-05-05 08:57:22

by Yun Levi

[permalink] [raw]
Subject: [PATCH RESEND] time/timgr: Fix wrong reference when level 0 group allocation failed

When tmigr_setup_groups() failed level 0 group allocation,
wrong reference happens on local stack array while intializing timer hierarchy.

To prevent this, Check loop condition first before intializing timer hierarchy.


Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
Signed-off-by: Levi Yun <[email protected]>
---
kernel/time/timer_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index ccba875d2234..84413114db5c 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)

} while (i < tmigr_hierarchy_levels);

- do {
+ while (i > 0) {
group = stack[--i];

if (err < 0) {
@@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
tmigr_connect_child_parent(child, group);
}
}
- } while (i > 0);
+ }

kfree(stack);

--
2.41.0

2024-05-05 10:24:29

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH RESEND] time/timgr: Fix wrong reference when level 0 group allocation failed


> To prevent this, Check loop condition first before intializing timer hierarchy.

> Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")


Does this change approach represent a subsequent patch version instead of a “RESEND”?

How do you think about to add a patch changelog accordingly?

Regards,
Markus

2024-05-05 10:54:40

by Yun Levi

[permalink] [raw]
Subject: [PATCH v2] time/timgr: Fix wrong reference when level 0 group allocation failed

When tmigr_setup_groups() failed level 0 group allocation,
wrong reference happens on local stack array while intializing timer hierarchy.

To prevent this, Check loop condition first before intializing timer hierarchy.

Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
Signed-off-by: Levi Yun <[email protected]>
---

v2:
- Modify commit message.

kernel/time/timer_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index ccba875d2234..84413114db5c 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)

} while (i < tmigr_hierarchy_levels);

- do {
+ while (i > 0) {
group = stack[--i];

if (err < 0) {
@@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
tmigr_connect_child_parent(child, group);
}
}
- } while (i > 0);
+ }

kfree(stack);

--
2.41.0

2024-05-05 10:55:23

by Yun Levi

[permalink] [raw]
Subject: Re: [PATCH RESEND] time/timgr: Fix wrong reference when level 0 group allocation failed

Hi Markus :)


> Does this change approach represent a subsequent patch version instead of a “RESEND”?
>
> How do you think about to add a patch changelog accordingly?
>
Thanks I'll do

Thanks.

2024-05-05 17:07:49

by Markus Elfring

[permalink] [raw]
Subject: Re: [v2] time/timgr: Fix wrong reference when level 0 group allocation failed


> To prevent this, Check loop condition first before intializing timer hierarchy.

I suggest to avoid another typo (also in the email address for Frederic Weisbecker).

Regards,
Markus

2024-05-06 04:11:13

by Yun Levi

[permalink] [raw]
Subject: [PATCH v3] time/timgr: Fix wrong reference when level 0 group allocation failed

When tmigr_setup_groups() failed level 0 group allocation,
wrong reference happens on local stack array while intializing timer hierarchy.

To prevent this, Check loop condition first before initializing timer hierarchy.

Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
Signed-off-by: Levi Yun <[email protected]>
---
v3:
- Fix typo.

v2:
- Modify commit message.

kernel/time/timer_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index ccba875d2234..84413114db5c 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)

} while (i < tmigr_hierarchy_levels);

- do {
+ while (i > 0) {
group = stack[--i];

if (err < 0) {
@@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
tmigr_connect_child_parent(child, group);
}
}
- } while (i > 0);
+ }

kfree(stack);

--
2.41.0

2024-05-06 10:10:28

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH v3] time/timgr: Fix wrong reference when level 0 group allocation failed

Le Mon, May 06, 2024 at 05:10:59AM +0100, Levi Yun a ?crit :
> When tmigr_setup_groups() failed level 0 group allocation,
> wrong reference happens on local stack array while intializing timer hierarchy.
>
> To prevent this, Check loop condition first before initializing timer hierarchy.
>
> Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
> Signed-off-by: Levi Yun <[email protected]>
> ---
> v3:
> - Fix typo.
>
> v2:
> - Modify commit message.
>
> kernel/time/timer_migration.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
> index ccba875d2234..84413114db5c 100644
> --- a/kernel/time/timer_migration.c
> +++ b/kernel/time/timer_migration.c
> @@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
>
> } while (i < tmigr_hierarchy_levels);
>
> - do {
> + while (i > 0) {
> group = stack[--i];
>
> if (err < 0) {
> @@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
> tmigr_connect_child_parent(child, group);
> }
> }
> - } while (i > 0);
> + }

Looks good to me. But let's wait for Anna-Maria's second look. The group setup
is not my favourite area...

Thanks.

>
> kfree(stack);
>
> --
> 2.41.0

2024-05-08 06:16:22

by Anna-Maria Behnsen

[permalink] [raw]
Subject: Re: [PATCH v3] time/timgr: Fix wrong reference when level 0 group allocation failed

Frederic Weisbecker <[email protected]> writes:

> Le Mon, May 06, 2024 at 05:10:59AM +0100, Levi Yun a écrit :
>> When tmigr_setup_groups() failed level 0 group allocation,
>> wrong reference happens on local stack array while intializing timer hierarchy.
>>
>> To prevent this, Check loop condition first before initializing timer hierarchy.
>>
>> Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
>> Signed-off-by: Levi Yun <[email protected]>
>> ---
>> v3:
>> - Fix typo.
>>
>> v2:
>> - Modify commit message.
>>
>> kernel/time/timer_migration.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migrationc
>> index ccba875d2234..84413114db5c 100644
>> --- a/kernel/time/timer_migration.c
>> +++ b/kernel/time/timer_migration.c
>> @@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
>>
>> } while (i < tmigr_hierarchy_levels);
>>
>> - do {
>> + while (i > 0) {
>> group = stack[--i];
>>
>> if (err < 0) {
>> @@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
>> tmigr_connect_child_parent(child, group);
>> }
>> }
>> - } while (i > 0);
>> + }
>
> Looks good to me. But let's wait for Anna-Maria's second look. The group setup
> is not my favourite area...
>

Thanks for the fix!

Reviewed-by: Anna-Maria Behnsen <[email protected]>


2024-05-08 06:39:03

by Markus Elfring

[permalink] [raw]
Subject: Re: [v3] time/timgr: Fix wrong reference when level 0 group allocation failed


>>> To prevent this, Check loop condition first before initializing timer hierarchy.

>>> ---
>>> v3:
>>> - Fix typo.
>>>
>>> v2:
>>> - Modify commit message.
>>>
>>> kernel/time/timer_migration.c | 4 ++--

> Reviewed-by: Anna-Maria Behnsen <[email protected]>

Do you find any remaining wording concerns less relevant fur such a changelog?

Regards,
Markus

Subject: [tip: timers/urgent] timers/migration: Prevent out of bounds access on failure

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID: d7ad05c86e2191bd66e5b62fca8da53c4a53484f
Gitweb: https://git.kernel.org/tip/d7ad05c86e2191bd66e5b62fca8da53c4a53484f
Author: Levi Yun <[email protected]>
AuthorDate: Mon, 06 May 2024 05:10:59 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 08 May 2024 11:19:43 +02:00

timers/migration: Prevent out of bounds access on failure

When tmigr_setup_groups() fails the level 0 group allocation, then the
cleanup derefences index -1 of the local stack array.

Prevent this by checking the loop condition first.

Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
Signed-off-by: Levi Yun <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Anna-Maria Behnsen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/time/timer_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index ccba875..8441311 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -1596,7 +1596,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)

} while (i < tmigr_hierarchy_levels);

- do {
+ while (i > 0) {
group = stack[--i];

if (err < 0) {
@@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
tmigr_connect_child_parent(child, group);
}
}
- } while (i > 0);
+ }

kfree(stack);


2024-05-08 09:58:17

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [tip: timers/urgent] timers/migration: Prevent out of bounds access on failure

On Wed, May 08, 2024 at 09:45:41AM -0000, tip-bot2 for Levi Yun wrote:
> The following commit has been merged into the timers/urgent branch of tip:

(Yes, I noted above)

..

> - do {
> + while (i > 0) {
> group = stack[--i];

Looking at this and most used patterns for cleanup loops, I would amend
this to

while (i--) {
group = stack[i];

which seems to me an equivalent.

> if (err < 0) {
> @@ -1645,7 +1645,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
> tmigr_connect_child_parent(child, group);
> }
> }
> - } while (i > 0);
> + }

--
With Best Regards,
Andy Shevchenko