| Message ID | 20240328181914.869332-1-kiryushin@ancud.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rcu: Fix buffer overlow in print_cpu_stall_info() | expand |
On Thu, Mar 28, 2024 at 09:19:14PM +0300, Nikita Kiryushin 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 (which can be a bad format, if the values > are huge). > > Change sprintf to snprintf and change %ld to %lu in format. Good catch!!! However, the signed output is intentional. The idea is that if the timekeeping code is confused enough to run the jiffies counter backwards, we see a small negative number rather than a huge positive number. For example, -132 is immediately obvious, while the 64-bit unsigned equivalent of 18446744073709551484 might not be. would you like to resend keeping the buffer-overflow fix but leaving out the signed-to-unsigned conversion? Thanx, Paul > 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> > --- > kernel/rcu/tree_stall.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h > index 5d666428546b..d4542c6e7c60 100644 > --- a/kernel/rcu/tree_stall.h > +++ b/kernel/rcu/tree_stall.h > @@ -504,7 +504,7 @@ 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); > + snprintf(buf, sizeof(buf), " rcuc=%lu 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)], > -- > 2.34.1 >
Thank you for the feedback! > would you like to resend keeping the buffer-overflow fix but leaving > out the signed-to-unsigned conversion? > I will make a second version of the patch, without conversion as it is intentional. > However, the signed output is intentional. The idea is that if the > timekeeping code is confused enough to run the jiffies counter backwards, > we see a small negative number rather than a huge positive number. > For example, -132 is immediately obvious, while the 64-bit unsigned > equivalent of 18446744073709551484 might not be. I had suspicions that was the case, however, I did not find the pointers in the code or in the commit message, that it was intentional, so I assumed a mistake. Maybe, it would be a good idea for me to add a comment with intent clarification, to reduce possibility of the same confusion in the future, while I am at it? If so, should I do it in the same patch, or make a separate one?
On Fri, 29 Mar 2024 20:56:16 +0300 Nikita Kiryushin <kiryushin@ancud.ru> wrote: > Maybe, it would be a good idea for me to add a comment with intent > clarification, to reduce possibility of the same confusion in the future, Yes please do. > while I am at it? If so, should I do it in the same patch, or make a separate one? I would keep it the same patch, but it really is Paul's decision. -- Steve
On Fri, Mar 29, 2024 at 02:32:05PM -0400, Steven Rostedt wrote: > On Fri, 29 Mar 2024 20:56:16 +0300 > Nikita Kiryushin <kiryushin@ancud.ru> wrote: > > > Maybe, it would be a good idea for me to add a comment with intent > > clarification, to reduce possibility of the same confusion in the future, > > Yes please do. > > > while I am at it? If so, should I do it in the same patch, or make a separate one? > > I would keep it the same patch, but it really is Paul's decision. I am with Steve on both questions. Thanx, Paul
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5d666428546b..d4542c6e7c60 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -504,7 +504,7 @@ 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); + snprintf(buf, sizeof(buf), " rcuc=%lu 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 (which can be a bad format, if the values are huge). Change sprintf to snprintf and change %ld to %lu in 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> --- kernel/rcu/tree_stall.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)