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 <[email protected]>
---
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
On Mon, 23 Jul 2018 15:42:10 +0200
Snild Dolkow <[email protected]> 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 <[email protected]>
> ---
> 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.
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
On Mon, 23 Jul 2018 16:23:09 +0200
Snild Dolkow <[email protected]> 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
On 07/23/2018 05:37 PM, Steven Rostedt wrote:
> On Mon, 23 Jul 2018 16:23:09 +0200
> Snild Dolkow <[email protected]> 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
On Mon, 23 Jul 2018 17:49:36 +0200
Snild Dolkow <[email protected]> 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().
On 07/23/2018 06:41 PM, Steven Rostedt wrote:
> On Mon, 23 Jul 2018 17:49:36 +0200
> Snild Dolkow <[email protected]> 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 rest read/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'
0xffffffd5b3818640: "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: ffffff9bf9783c78
crash-arm64> kbt | grep 'comm\|trace_print_context'
#6 0xffffff9bf9783c78 in trace_print_context+0x18c(+396)
comm (char [16]) = "irq/497-pwr_even"
crash-arm64> rd 0xffffffd4d0e17d14 8
ffffffd4d0e17d14: 2f71726900000000 5f7277702d373934 ....irq/497-pwr_
ffffffd4d0e17d24: 726f776b6e657665 3a3631752f72656b evenkworker/u16:
ffffffd4d0e17d34: f9780248ff003231 cede60e0ffffff9b 12..H.x......`..
ffffffd4d0e17d44: cede60c8ffffffd4 00000fffffffffd4 .....`..........
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
On Tue, 24 Jul 2018 10:17:37 +0200
Snild Dolkow <[email protected]> wrote:
> On 07/23/2018 06:41 PM, Steven Rostedt wrote:
> > On Mon, 23 Jul 2018 17:49:36 +0200
> > Snild Dolkow <[email protected]> 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 rest read/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'
> 0xffffffd5b3818640: "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: ffffff9bf9783c78
>
> crash-arm64> kbt | grep 'comm\|trace_print_context'
> #6 0xffffff9bf9783c78 in trace_print_context+0x18c(+396)
> comm (char [16]) = "irq/497-pwr_even"
>
> crash-arm64> rd 0xffffffd4d0e17d14 8
> ffffffd4d0e17d14: 2f71726900000000 5f7277702d373934 ....irq/497-pwr_
> ffffffd4d0e17d24: 726f776b6e657665 3a3631752f72656b evenkworker/u16:
> ffffffd4d0e17d34: f9780248ff003231 cede60e0ffffff9b 12..H.x......`..
> ffffffd4d0e17d44: cede60c8ffffffd4 00000fffffffffd4 .....`..........
>
> 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
On 07/24/2018 04:48 PM, Steven Rostedt wrote:
> On Tue, 24 Jul 2018 10:17:37 +0200
> Snild Dolkow <[email protected]> wrote:
>
>> creator other
>> vsnprintf:
>> fill (not terminated)
>> count the rest read/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