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
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
>
> 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
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
…
> 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
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
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.
…
> 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
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
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
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]>
…
>>> 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
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);
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