| Message ID | 20240401194315.362090-1-kiryushin@ancud.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] rcu: Fix buffer overlow in print_cpu_stall_info() | expand |
On Mon, 1 Apr 2024 22:43:15 +0300 Nikita Kiryushin <kiryushin@ancud.ru> wrote: > rcuc info output in print_cpu_stall_info() contains > posiible buffer overflow in the case of huge jiffies > difference. The situation seems improbable, but, buffer > overflow, still. > > Also, unsigned jiffies difference printed as (signed) > %ld. This is intentional for debugging purposes, but > it is not obvious from the code. > > Change sprintf to snprintf and add clarifying comment > about intention of %ld format. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 245a62982502 ("rcu: Dump rcuc kthread status for CPUs not reporting quiescent state") > Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru> > --- > v3: Change intention comment wording as > Steven Rostedt <rostedt@goodmis.org> suggested > v2: Remove signed to unsigned print format change as > Paul E. McKenney <paulmck@kernel.org> suggested, add format > intention clarification comment > kernel/rcu/tree_stall.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h > index 5d666428546b..320440b8384e 100644 > --- a/kernel/rcu/tree_stall.h > +++ b/kernel/rcu/tree_stall.h > @@ -504,7 +504,8 @@ static void print_cpu_stall_info(int cpu) > rcu_dynticks_in_eqs(rcu_dynticks_snap(cpu)); > rcuc_starved = rcu_is_rcuc_kthread_starving(rdp, &j); > if (rcuc_starved) > - sprintf(buf, " rcuc=%ld jiffies(starved)", j); > + /* Print signed value, as negative means it is likely a bug */ > + snprintf(buf, sizeof(buf), " rcuc=%ld jiffies(starved)", j); > pr_err("\t%d-%c%c%c%c: (%lu %s) idle=%04x/%ld/%#lx softirq=%u/%u fqs=%ld%s%s\n", > cpu, > "O."[!!cpu_online(cpu)], Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On Mon, Apr 01, 2024 at 04:03:12PM -0400, Steven Rostedt wrote: > On Mon, 1 Apr 2024 22:43:15 +0300 > Nikita Kiryushin <kiryushin@ancud.ru> wrote: > > > rcuc info output in print_cpu_stall_info() contains > > posiible buffer overflow in the case of huge jiffies > > difference. The situation seems improbable, but, buffer > > overflow, still. > > > > Also, unsigned jiffies difference printed as (signed) > > %ld. This is intentional for debugging purposes, but > > it is not obvious from the code. > > > > Change sprintf to snprintf and add clarifying comment > > about intention of %ld format. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Fixes: 245a62982502 ("rcu: Dump rcuc kthread status for CPUs not reporting quiescent state") > > Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru> > > --- > > v3: Change intention comment wording as > > Steven Rostedt <rostedt@goodmis.org> suggested > > v2: Remove signed to unsigned print format change as > > Paul E. McKenney <paulmck@kernel.org> suggested, add format > > intention clarification comment > > kernel/rcu/tree_stall.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h > > index 5d666428546b..320440b8384e 100644 > > --- a/kernel/rcu/tree_stall.h > > +++ b/kernel/rcu/tree_stall.h > > @@ -504,7 +504,8 @@ static void print_cpu_stall_info(int cpu) > > rcu_dynticks_in_eqs(rcu_dynticks_snap(cpu)); > > rcuc_starved = rcu_is_rcuc_kthread_starving(rdp, &j); > > if (rcuc_starved) > > - sprintf(buf, " rcuc=%ld jiffies(starved)", j); > > + /* Print signed value, as negative means it is likely a bug */ > > + snprintf(buf, sizeof(buf), " rcuc=%ld jiffies(starved)", j); > > pr_err("\t%d-%c%c%c%c: (%lu %s) idle=%04x/%ld/%#lx softirq=%u/%u fqs=%ld%s%s\n", > > cpu, > > "O."[!!cpu_online(cpu)], > > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> Queued for v6.10, thank you both! Thanx, Paul
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5d666428546b..320440b8384e 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -504,7 +504,8 @@ static void print_cpu_stall_info(int cpu) rcu_dynticks_in_eqs(rcu_dynticks_snap(cpu)); rcuc_starved = rcu_is_rcuc_kthread_starving(rdp, &j); if (rcuc_starved) - sprintf(buf, " rcuc=%ld jiffies(starved)", j); + /* Print signed value, as negative means it is likely a bug */ + snprintf(buf, sizeof(buf), " rcuc=%ld jiffies(starved)", j); pr_err("\t%d-%c%c%c%c: (%lu %s) idle=%04x/%ld/%#lx softirq=%u/%u fqs=%ld%s%s\n", cpu, "O."[!!cpu_online(cpu)],
rcuc info output in print_cpu_stall_info() contains posiible buffer overflow in the case of huge jiffies difference. The situation seems improbable, but, buffer overflow, still. Also, unsigned jiffies difference printed as (signed) %ld. This is intentional for debugging purposes, but it is not obvious from the code. Change sprintf to snprintf and add clarifying comment about intention of %ld format. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 245a62982502 ("rcu: Dump rcuc kthread status for CPUs not reporting quiescent state") Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru> --- v3: Change intention comment wording as Steven Rostedt <rostedt@goodmis.org> suggested v2: Remove signed to unsigned print format change as Paul E. McKenney <paulmck@kernel.org> suggested, add format intention clarification comment kernel/rcu/tree_stall.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)