Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On 07/24/2018 04:48 PM, Steven Rostedt wrote: > On Tue, 24 Jul 2018 10:17:37 +0200 > Snild Dolkow wrote: > >> creator other >> vsnprintf: >>fill (not terminated) >>count the restread/use comm > > I think it would be better to state what was reading the comm. Like > > trace_sched_waking(p) > memcpy(comm, p->comm, TASK_COMM_LEN) > > But the rest looks fine. > > -- Steve Works for me. Reworded v2 coming up soon. //Snild
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On Tue, 24 Jul 2018 10:17:37 +0200 Snild Dolkow wrote: > On 07/23/2018 06:41 PM, Steven Rostedt wrote: > > On Mon, 23 Jul 2018 17:49:36 +0200 > > Snild Dolkow wrote: > >> Any issues with the commit message? Reading it back again now, it doesn't > >> seem quite as clear as when I wrote it. > > > > Yeah, I think it does need some updates: > > > >> There was a window for racing when task->comm was being written. The > > > > It would be nice to explain this race window in more detail. > > > I hope the following is more clear: > > kthread, tracing: Don't expose half-written comm when creating kthreads > > There is a window for racing when printing directly to task->comm, > allowing other threads to see a non-terminated string. The vsnprintf > function fills the buffer, counts the truncated chars, then finally > writes the \0 at the end. > > creator other > vsnprintf: > fill (not terminated) > count the restread/use comm I think it would be better to state what was reading the comm. Like trace_sched_waking(p) memcpy(comm, p->comm, TASK_COMM_LEN) But the rest looks fine. -- Steve > write \0 > > The consequences depend on how 'other' uses the string. In our case, > it was copied into the tracing system's saved cmdlines, a buffer of > adjacent TASK_COMM_LEN-byte buffers (note the 'n' where 0 should be): > > crash-arm64> x/1024s savedcmd->saved_cmdlines | grep 'evenk' > 0xffd5b3818640: "irq/497-pwr_evenkworker/u16:12" > > ...and a strcpy out of there would cause stack corruption: > > [224761.522292] Kernel panic - not syncing: stack-protector: > Kernel stack is corrupted in: ff9bf9783c78 > > crash-arm64> kbt | grep 'comm\|trace_print_context' > #6 0xff9bf9783c78 in trace_print_context+0x18c(+396) > comm (char [16]) = "irq/497-pwr_even" > > crash-arm64> rd 0xffd4d0e17d14 8 > ffd4d0e17d14: 2f717269 5f7277702d373934 irq/497-pwr_ > ffd4d0e17d24: 726f776b6e657665 3a3631752f72656b evenkworker/u16: > ffd4d0e17d34: f9780248ff003231 cede60e0ff9b 12..H.x..`.. > ffd4d0e17d44: cede60c8ffd4 0fd4 .`.. > > The workaround in e09e28671 (use strlcpy in __trace_find_cmdline) was > likely needed because of this same bug. > > Solved by vsnprintf:ing to a local buffer, then using set_task_comm(). > This way, there won't be a window where comm is not terminated. > > > //Snild
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On 07/23/2018 06:41 PM, Steven Rostedt wrote: > On Mon, 23 Jul 2018 17:49:36 +0200 > Snild Dolkow wrote: >> Any issues with the commit message? Reading it back again now, it doesn't >> seem quite as clear as when I wrote it. > > Yeah, I think it does need some updates: > >> There was a window for racing when task->comm was being written. The > > It would be nice to explain this race window in more detail. > I hope the following is more clear: kthread, tracing: Don't expose half-written comm when creating kthreads There is a window for racing when printing directly to task->comm, allowing other threads to see a non-terminated string. The vsnprintf function fills the buffer, counts the truncated chars, then finally writes the \0 at the end. creator other vsnprintf: fill (not terminated) count the restread/use comm write \0 The consequences depend on how 'other' uses the string. In our case, it was copied into the tracing system's saved cmdlines, a buffer of adjacent TASK_COMM_LEN-byte buffers (note the 'n' where 0 should be): crash-arm64> x/1024s savedcmd->saved_cmdlines | grep 'evenk' 0xffd5b3818640: "irq/497-pwr_evenkworker/u16:12" ...and a strcpy out of there would cause stack corruption: [224761.522292] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ff9bf9783c78 crash-arm64> kbt | grep 'comm\|trace_print_context' #6 0xff9bf9783c78 in trace_print_context+0x18c(+396) comm (char [16]) = "irq/497-pwr_even" crash-arm64> rd 0xffd4d0e17d14 8 ffd4d0e17d14: 2f717269 5f7277702d373934 irq/497-pwr_ ffd4d0e17d24: 726f776b6e657665 3a3631752f72656b evenkworker/u16: ffd4d0e17d34: f9780248ff003231 cede60e0ff9b 12..H.x..`.. ffd4d0e17d44: cede60c8ffd4 0fd4 .`.. The workaround in e09e28671 (use strlcpy in __trace_find_cmdline) was likely needed because of this same bug. Solved by vsnprintf:ing to a local buffer, then using set_task_comm(). This way, there won't be a window where comm is not terminated. //Snild
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On Mon, 23 Jul 2018 17:49:36 +0200 Snild Dolkow wrote: > On 07/23/2018 05:37 PM, Steven Rostedt wrote: > Will add: > > /* >* task is already visible to other tasks, so updating >* COMM must be protected. >*/ Thanks. > > Any issues with the commit message? Reading it back again now, it doesn't > seem quite as clear as when I wrote it. Yeah, I think it does need some updates: > There was a window for racing when task->comm was being written. The It would be nice to explain this race window in more detail. > vsnprintf function writes 16 bytes, then counts the rest, then null > terminates. In the meantime, other threads could see the non-terminated > comm value. In our case, it got into the trace system's saved cmdlines > and could cause stack corruption when strcpy'd out of there. Perhaps add in the change log something about the fact that the vsprintf() is performed on the COMM when the task is visible to other tasks, and that could cause problems if other tasks read the COMM (like in tracing) without updating it properly with set_task_comm(). -- Steve > > The workaround in e09e28671 (use strlcpy in __trace_find_cmdline) was > likely needed because of this bug. > > Solved by vsnprintf:ing to a local buffer, then using set_task_comm().
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On 07/23/2018 05:37 PM, Steven Rostedt wrote: > On Mon, 23 Jul 2018 16:23:09 +0200 > Snild Dolkow wrote: > >> On 07/23/2018 03:55 PM, Steven Rostedt wrote: >> >>> Can you add a comment here stating something to the affect of: >>> /* task is now visible to other tasks */ >>> >>> -- Steve >> Sure, but isn't that a bit misleading? It will have been visible since >> some unknown point in time between waking up kthreadd and the return of >> wait_for_completion(); we're not the ones making it visible. >> > > I guess that should be reworded, as that is not what I meant, and I > thought not what I stated. It's stating that the task is now visible, > not that we are now making it invisible. But I guess I was being too > short with what I meant. Here's the full statement: > > /* >* task is now visible by other tasks, so updating COMM >* must be protected. >*/ > > -- Steve > Ah. It's the "now" that trips me up. :) Will add: /* * task is already visible to other tasks, so updating * COMM must be protected. */ Any issues with the commit message? Reading it back again now, it doesn't seem quite as clear as when I wrote it. //Snild
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On Mon, 23 Jul 2018 16:23:09 +0200 Snild Dolkow wrote: > On 07/23/2018 03:55 PM, Steven Rostedt wrote: > > > Can you add a comment here stating something to the affect of: > > /* task is now visible to other tasks */ > > > > -- Steve > Sure, but isn't that a bit misleading? It will have been visible since > some unknown point in time between waking up kthreadd and the return of > wait_for_completion(); we're not the ones making it visible. > I guess that should be reworded, as that is not what I meant, and I thought not what I stated. It's stating that the task is now visible, not that we are now making it invisible. But I guess I was being too short with what I meant. Here's the full statement: /* * task is now visible by other tasks, so updating COMM * must be protected. */ -- Steve
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On 07/23/2018 03:55 PM, Steven Rostedt wrote: > Can you add a comment here stating something to the affect of: > /* task is now visible to other tasks */ > > -- Steve Sure, but isn't that a bit misleading? It will have been visible since some unknown point in time between waking up kthreadd and the return of wait_for_completion(); we're not the ones making it visible. //Snild
Re: [PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
On Mon, 23 Jul 2018 15:42:10 +0200
Snild Dolkow wrote:
> There was a window for racing when task->comm was being written. The
> vsnprintf function writes 16 bytes, then counts the rest, then null
> terminates. In the meantime, other threads could see the non-terminated
> comm value. In our case, it got into the trace system's saved cmdlines
> and could cause stack corruption when strcpy'd out of there.
>
> The workaround in e09e28671 (use strlcpy in __trace_find_cmdline) was
> likely needed because of this bug.
>
> Solved by vsnprintf:ing to a local buffer, then using set_task_comm().
>
> Signed-off-by: Snild Dolkow
> ---
> kernel/kthread.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 481951bf091d..28874afbf747 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -319,8 +319,10 @@ struct task_struct *__kthread_create_on_node(int
> (*threadfn)(void *data),
> task = create->result;
> if (!IS_ERR(task)) {
> static const struct sched_param param = { .sched_priority = 0 };
> + char name[TASK_COMM_LEN];
>
> - vsnprintf(task->comm, sizeof(task->comm), namefmt, args);
Can you add a comment here stating something to the affect of:
/* task is now visible to other tasks */
-- Steve
> + vsnprintf(name, sizeof(name), namefmt, args);
> + set_task_comm(task, name);
> /*
>* root may have changed our (kthreadd's) priority or CPU mask.
>* The kernel thread should not inherit these properties.
[PATCH RESEND] kthread, tracing: Don't expose half-written comm when creating kthreads
There was a window for racing when task->comm was being written. The
vsnprintf function writes 16 bytes, then counts the rest, then null
terminates. In the meantime, other threads could see the non-terminated
comm value. In our case, it got into the trace system's saved cmdlines
and could cause stack corruption when strcpy'd out of there.
The workaround in e09e28671 (use strlcpy in __trace_find_cmdline) was
likely needed because of this bug.
Solved by vsnprintf:ing to a local buffer, then using set_task_comm().
Signed-off-by: Snild Dolkow
---
kernel/kthread.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 481951bf091d..28874afbf747 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -319,8 +319,10 @@ struct task_struct *__kthread_create_on_node(int
(*threadfn)(void *data),
task = create->result;
if (!IS_ERR(task)) {
static const struct sched_param param = { .sched_priority = 0 };
+ char name[TASK_COMM_LEN];
- vsnprintf(task->comm, sizeof(task->comm), namefmt, args);
+ vsnprintf(name, sizeof(name), namefmt, args);
+ set_task_comm(task, name);
/*
* root may have changed our (kthreadd's) priority or CPU mask.
* The kernel thread should not inherit these properties.
--
2.15.1
