arch/x86: fix out-of-bounds in get_wchan()
Subsystems (4)
PATCH 1/1 arch/x86: fix out-of-bounds in get_wchan()
get_wchan() checks that fp is within stack bounds, but then dereferences fp+8. This can crash kernel or leak sensitive information. Also the function operates on a potentially running stack, but does not use READ_ONCE. As the result it can check that one value is within stack bounds, but then deref another value.
Fix the bounds check and use READ_ONCE for all volatile data.
The bug was discovered with KASAN.
---
FTR, here is the KASAN report:
[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.578633] Accessed by thread T10915: [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444 [ 124.582763] #4 ffffffff81342108 in do_task_stat array.c:0 [ 124.583634] #5 ffffffff81342dcc in proc_tgid_stat ??:0 [ 124.584548] #6 ffffffff8133c984 in proc_single_show base.c:0 [ 124.585461] #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222 [ 124.586313] #8 ffffffff8129e503 in vfs_read ??:0 [ 124.587137] #9 ffffffff8129f800 in SyS_read ??:0 [ 124.587827] #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164 [ 124.588738] [ 124.593434] Shadow bytes around the buggy address: [ 124.594270] ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.595339] ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.596453] ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.597466] ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.598501] ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00 [ 124.600873] ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.601892] ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.603037] ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.604047] ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd [ 124.605054] ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa [ 124.605993] Shadow byte legend (one shadow byte represents 8 application bytes): [ 124.606958] Addressable: 00 [ 124.607483] Partially addressable: 01 02 03 04 05 06 07 [ 124.608219] Heap redzone: fa [ 124.608724] Heap kmalloc redzone: fb [ 124.609249] Freed heap region: fd [ 124.609753] Shadow gap:fe [ 124.610292] ========================================================================= --- arch/x86/kernel/process_64.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
arch/x86/kernel/process_64.c +7 -5
@@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )return 0 ; - fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp);do {if (fp < (unsigned long )stack || - fp >= (unsigned long )stack+THREAD_SIZE ) + fp+8 >= (unsigned long )stack+THREAD_SIZE )return 0 ; - ip = *(u64 *)(fp+8 ); + ip =READ_ONCE (*(u64 *)(fp+8 ));if (!in_sched_functions (ip))return ip; - fp = *(u64 *)fp; + fp =READ_ONCE (*(u64 *)fp); }while (count++ <16 );return 0 ; }
Signature · 1 line
2.6.0.rc2.230.g3dd15c0
Borislav Petkov
maintainerQuoted 58 lines · Dmitry Vyukov
get_wchan()checks that fp is within stack bounds, but then dereferences fp+8. This can crash kernel or leak sensitive information. Also the function operates on a potentially running stack, but does not useREAD_ONCE. As the result it can check that one value is within stack bounds, but then deref another value.Fix the bounds check and use
READ_ONCEfor all volatile data.The bug was discovered with KASAN.
Signed-off-by Dmitry Vyukov <[email protected]>---
FTR, here is the KASAN report:[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.578633] Accessed by thread T10915: [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444 [ 124.582763] #4 ffffffff81342108 in do_task_stat array.c:0 [ 124.583634] #5 ffffffff81342dcc in proc_tgid_stat ??:0 [ 124.584548] #6 ffffffff8133c984 in proc_single_show base.c:0 [ 124.585461] #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222 [ 124.586313] #8 ffffffff8129e503 in vfs_read ??:0 [ 124.587137] #9 ffffffff8129f800 in SyS_read ??:0 [ 124.587827] #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164 [ 124.588738] [ 124.593434] Shadow bytes around the buggy address: [ 124.594270] ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.595339] ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.596453] ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.597466] ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.598501] ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00 [ 124.600873] ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.601892] ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.603037] ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.604047] ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd [ 124.605054] ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa [ 124.605993] Shadow byte legend (one shadow byte represents 8 application bytes): [ 124.606958] Addressable: 00 [ 124.607483] Partially addressable: 01 02 03 04 05 06 07 [ 124.608219] Heap redzone: fa [ 124.608724] Heap kmalloc redzone: fb [ 124.609249] Freed heap region: fd [ 124.609753] Shadow gap:fe [ 124.610292] ========================================================================= --- arch/x86/kernel/process_64.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process_64.c +0
-0
arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 513
Quoted 10 lines
arch/x86/kernel/process_64.c +4
-2@@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )return 0 ; - fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp);
Why isn't this:
fp = READ_ONCE(*(u64 *)p->thread.sp);
like the original code did?
Actually, the original code looks fishy to me too - it did access live stack three times. And shouldn't we be accessing it only once?
I.e.,
fp_st = READ_ONCE(p->thread.sp); if (fp_st < stack || fp_st >= stack + THREAD_SIZE) return 0; fp = *(u64 *)fp_st;
Hmm?
Maybe I'm not completely clear on how the whole locking happens here because we do
if (!p || p == current || p->state == TASK_RUNNING)
return 0;earlier but apparently we can become TASK_RUNNING after the check...
Also, shouldn't this one have a CVE number assigned or so due to the leakage potential?
Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Quoted 72 lines · Borislav Petkov
Quoted 58 lines · Dmitry Vyukov
get_wchan()checks that fp is within stack bounds, but then dereferences fp+8. This can crash kernel or leak sensitive information. Also the function operates on a potentially running stack, but does not useREAD_ONCE. As the result it can check that one value is within stack bounds, but then deref another value.Fix the bounds check and use
READ_ONCEfor all volatile data.The bug was discovered with KASAN.
Signed-off-by Dmitry Vyukov <[email protected]>---
FTR, here is the KASAN report:[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.578633] Accessed by thread T10915: [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444 [ 124.582763] #4 ffffffff81342108 in do_task_stat array.c:0 [ 124.583634] #5 ffffffff81342dcc in proc_tgid_stat ??:0 [ 124.584548] #6 ffffffff8133c984 in proc_single_show base.c:0 [ 124.585461] #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222 [ 124.586313] #8 ffffffff8129e503 in vfs_read ??:0 [ 124.587137] #9 ffffffff8129f800 in SyS_read ??:0 [ 124.587827] #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164 [ 124.588738] [ 124.593434] Shadow bytes around the buggy address: [ 124.594270] ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.595339] ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.596453] ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.597466] ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.598501] ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00 [ 124.600873] ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.601892] ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.603037] ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.604047] ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd [ 124.605054] ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa [ 124.605993] Shadow byte legend (one shadow byte represents 8 application bytes): [ 124.606958] Addressable: 00 [ 124.607483] Partially addressable: 01 02 03 04 05 06 07 [ 124.608219] Heap redzone: fa [ 124.608724] Heap kmalloc redzone: fb [ 124.609249] Freed heap region: fd [ 124.609753] Shadow gap:fe [ 124.610292] ========================================================================= --- arch/x86/kernel/process_64.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process_64.c +0
-0arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 513
Quoted 10 lines
arch/x86/kernel/process_64.c +4
-2@@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )return 0 ; - fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp);Why isn't this:
Quoted 3 lines
fp = READ_ONCE(*(u64 *)p->thread.sp);like the original code did?
Original code did:
if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
return 0;
fp = *(u64 *)(p->thread.sp);p->thread.sp can change concurrently.
So we could check that p->thread.sp is within stack bounds, but then dereference another value (which is already outside of bounds).
Quoted 11 lines
Actually, the original code looks fishy to me too - it did access live stack three times. And shouldn't we be accessing it only once?
I.e.,
fp_st = READ_ONCE(p->thread.sp); if (fp_st < stack || fp_st >= stack + THREAD_SIZE) return 0; fp = *(u64 *)fp_st;Hmm?
That's what my patch does.
Quoted 10 lines
Maybe I'm not completely clear on how the whole locking happens here because we do
if (!p || p == current || p->state == TASK_RUNNING) return 0;earlier but apparently we can become
TASK_RUNNINGafter the check...Also, shouldn't this one have a CVE number assigned or so due to the leakage potential?
Borislav Petkov
maintainerQuoted 9 lines · Dmitry Vyukov
Original code did:
if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE) return 0; fp = *(u64 *)(p->thread.sp);
p->thread.spcan change concurrently.
So we could check thatp->thread.spis within stack bounds, but then dereference another value (which is already outside of bounds).
Right, we do deref it. I realized that after hitting "Send" :\
Which begs another, probably also stupid, question:
What guarantees the task won't disappear after we've checked p?
I.e., after this:
if (!p || p == current || p->state == TASK_RUNNING)
return 0;Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Quoted 13 lines · Borislav Petkov
Quoted 9 lines · Dmitry Vyukov
Original code did:
if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE) return 0; fp = *(u64 *)(p->thread.sp);
p->thread.spcan change concurrently.
So we could check thatp->thread.spis within stack bounds, but then dereference another value (which is already outside of bounds).Right, we do deref it. I realized that after hitting "Send" :\
Quoted 8 lines
Which begs another, probably also stupid, question:
What guarantees the task won't disappear after we've checked p?
I.e., after this:
if (!p || p == current || p->state == TASK_RUNNING) return 0;
I have not checked, but I would expect that it is caller's responsibility. There is generally no way to magically resurrect a pointer to a freed object passed in.
Borislav Petkov
maintainerQuoted 2 lines · Dmitry Vyukov
I have not checked, but I would expect that it is caller's responsibility.
Looks like it: proc, for example, does get_pid_task()->get_task_struct().
Quoted 2 lines
There is generally no way to magically resurrect a pointer to a freed object passed in.
Right.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Quoted 91 lines · Borislav Petkov
Quoted 58 lines · Dmitry Vyukov
get_wchan()checks that fp is within stack bounds, but then dereferences fp+8. This can crash kernel or leak sensitive information. Also the function operates on a potentially running stack, but does not useREAD_ONCE. As the result it can check that one value is within stack bounds, but then deref another value.Fix the bounds check and use
READ_ONCEfor all volatile data.The bug was discovered with KASAN.
Signed-off-by Dmitry Vyukov <[email protected]>---
FTR, here is the KASAN report:[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.578633] Accessed by thread T10915: [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444 [ 124.582763] #4 ffffffff81342108 in do_task_stat array.c:0 [ 124.583634] #5 ffffffff81342dcc in proc_tgid_stat ??:0 [ 124.584548] #6 ffffffff8133c984 in proc_single_show base.c:0 [ 124.585461] #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222 [ 124.586313] #8 ffffffff8129e503 in vfs_read ??:0 [ 124.587137] #9 ffffffff8129f800 in SyS_read ??:0 [ 124.587827] #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164 [ 124.588738] [ 124.593434] Shadow bytes around the buggy address: [ 124.594270] ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.595339] ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.596453] ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.597466] ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.598501] ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00 [ 124.600873] ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 124.601892] ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.603037] ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa [ 124.604047] ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd [ 124.605054] ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa [ 124.605993] Shadow byte legend (one shadow byte represents 8 application bytes): [ 124.606958] Addressable: 00 [ 124.607483] Partially addressable: 01 02 03 04 05 06 07 [ 124.608219] Heap redzone: fa [ 124.608724] Heap kmalloc redzone: fb [ 124.609249] Freed heap region: fd [ 124.609753] Shadow gap:fe [ 124.610292] ========================================================================= --- arch/x86/kernel/process_64.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process_64.c +0
-0arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 513
Quoted 10 lines
arch/x86/kernel/process_64.c +4
-2@@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )return 0 ; - fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp);Why isn't this:
fp = READ_ONCE(*(u64 *)p->thread.sp);like the original code did?
Actually, the original code looks fishy to me too - it did access live stack three times. And shouldn't we be accessing it only once?
I.e.,
fp_st = READ_ONCE(p->thread.sp); if (fp_st < stack || fp_st >= stack + THREAD_SIZE) return 0; fp = *(u64 *)fp_st;Hmm?
Maybe I'm not completely clear on how the whole locking happens here because we do
Quoted 7 lines
if (!p || p == current || p->state == TASK_RUNNING) return 0;earlier but apparently we can become
TASK_RUNNINGafter the check...Also, shouldn't this one have a CVE number assigned or so due to the leakage potential?
Dunno. Should it? Most likely the data will be leaked iff it is in_sched_functions. Never requested a CVE number before. If you insist, I guess I can contact MITRE.
Borislav Petkov
maintainerQuoted 2 lines · Dmitry Vyukov
Dunno. Should it? Most likely the data will be leaked iff it is
in_sched_functions.
.. if not in_sched_functions...
Quoted 2 lines
Never requested a CVE number before. If you insist, I guess I can contact MITRE.
Just wondering - generally the leaking to userspace things do get a CVE number but let's see what the others think.
luto will know - he loves to open CVEs! :-P
Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <[email protected]>:
arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 511
Quoted 5 lines
stack = (unsigned long)task_stack_page(p);arch/x86/kernel/process_64.c +3
-1-if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )
Since we deference fp, it should be "|| fp + sizeof(u64) >= stack + THREAD_SIZE"
arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 516
Quoted 7 lines
return 0;arch/x86/kernel/process_64.c +2
-2- fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp); do { if (fp < (unsigned long)stack || - fp >= (unsigned long)stack+THREAD_SIZE) + fp+8 >= (unsigned long)stack+THREAD_SIZE)
The same as above;
'fp+8 +sizeof(u64) >= ...'Quoted 4 lines
return 0;arch/x86/kernel/process_64.c +1
-1- ip = *(u64 *)(fp+8 ); + ip =READ_ONCE (*(u64 *)(fp+8 ));
Quoted 8 lines · Andrey Ryabinin
2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <[email protected]>:
arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 511
Quoted 5 lines
stack = (unsigned long)task_stack_page(p);arch/x86/kernel/process_64.c +3
-1-if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +/* The task can be already running at this point, so tread carefully. */ + fp =READ_ONCE (p->thread.sp); +if (fp < stack || fp >= stack+THREAD_SIZE )Since we deference fp, it should be "|| fp + sizeof(u64) >= stack +
THREAD_SIZE"
Good point.
I guess it should be "|| fp + sizeof(u64) > stack + THREAD_SIZE", because == is OK if we add 8.
Quoted 15 lines
arch/x86/kernel/process_64.c unsigned long get_wchan(struct task_struct *p) · line 516
Quoted 7 lines
return 0;arch/x86/kernel/process_64.c +2
-2- fp = *(u64 *)(p->thread.sp); + fp =READ_ONCE (*(u64 *)fp); do { if (fp < (unsigned long)stack || - fp >= (unsigned long)stack+THREAD_SIZE) + fp+8 >= (unsigned long)stack+THREAD_SIZE)The same as above; 'fp+8 +sizeof(u64) >= ...'Quoted 4 lines
return 0;arch/x86/kernel/process_64.c +1
-1- ip = *(u64 *)(fp+8 ); + ip =READ_ONCE (*(u64 *)(fp+8 ));
Quoted 8 lines · Dmitry Vyukov
Quoted 7 lines · Andrey Ryabinin
2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <[email protected]>:
Quoted 5 lines
stack = (unsigned long)task_stack_page(p);(quoted diff) +3
-1- if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE) + /* The task can be already running at this point, so tread carefully. */ + fp = READ_ONCE(p->thread.sp); + if (fp < stack || fp >= stack+THREAD_SIZE)
Quoted 6 lines
Quoted 1 line
Since we deference fp, it should be "|| fp + sizeof(u64) >= stack +
THREAD_SIZE"Good point.
I guess it should be "|| fp + sizeof(u64) > stack +THREAD_SIZE", because == is OK if we add 8.
This whole mess with +8 and -16 and whatever is just crap. And all of it completely undocumented. Proper version below.
Thanks,
tglx
8<-------------------------------
Subject: x86/process: Add proper bound checks in 64bit get_wchan()
From: Thomas Gleixner <[email protected]>
Date: Mon, 28 Sep 2015 17:16:52 +0200
Dmitry Vyukov reported the following using trinity and the memory error detector AddressSanitizer
(https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel).
[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.576801] ffff88002e280000 is located 131938492886538 bytes to the left of 28857600-byte region [ffffffff81282e0a, ffffffff82e0830a) [ 124.578633] Accessed by thread T10915: [ 124.579295] inlined in describe_heap_address ./arch/x86/mm/asan/report.c:164 [ 124.579295] #0 ffffffff810dd277 in asan_report_error ./arch/x86/mm/asan/report.c:278 [ 124.580137] #1 ffffffff810dc6a0 in asan_check_region ./arch/x86/mm/asan/asan.c:37 [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan
./arch/x86/kernel/process_64.c:444
The address checks in the 64bit implementation of get_wchan() are wrong in several ways:
- The lower bound of the stack is not the start of the stack page. It's the start of the stack page plus sizeof (struct thread_info)
- The upper bound must be top of stack minus 2 * sizeof(unsigned long). This is required because the stack pointer points at the frame pointer. The layout on the stack is: ... IP FP ... IP FP.
Fix the bound checks and get rid of the mix of numeric constants, u64 and unsigned long. Making all unsigned long allows us to use the same function for 32bit as well.
Based-on-patch-from: Wolfram Gloger <[email protected]>
--- arch/x86/kernel/process_64.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)
arch/x86/kernel/process_64.c +32 -9
@@ -501,24 +501,47 @@ EXPORT_SYMBOL_GPL(set_personality_ia32);unsigned long get_wchan (struct task_struct *p) { -unsigned long stack; -u64 fp, ip; +unsigned long start, bottom, top, sp, fp, ip;int count =0 ;if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; - stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) + + start = (unsigned long )task_stack_page (p); +if (!start)return 0 ; - fp = *(u64 *)(p->thread.sp); + +/* +* Layout of the stack page: +* +* ----------- top = start = THREAD_SIZE - sizeof(unsigned long) +* stack +* ----------- bottom = start + sizeof(thread_info) +* thread_info +* ----------- start +* +* The tasks stack pointer points at the location where the +* framepointer is stored. The data on the stack is: +* ... IP FP ... IP FP +* +* We need to read FP and IP, so we need to adjust the upper +* bound by another unsigned long. +*/ + top = start +THREAD_SIZE -2 *sizeof (unsigned long ); + bottom = start +sizeof (struct thread_info ); + + sp = p->thread.sp; +if (sp < bottom || sp > top) +return 0 ; + + fp = *(unsigned long *)sp;do { -if (fp < (unsigned long )stack || - fp >= (unsigned long )stack+THREAD_SIZE ) +if (fp < bottom || fp > top)return 0 ; - ip = *(u64 *)(fp+8 ); + ip = *(unsigned long *)(fp +sizeof (unsigned long ));if (!in_sched_functions (ip))return ip; - fp = *(u64 *)fp; + fp = *(unsigned long *)fp; }while (count++ <16 );return 0 ; }
Quoted 94 lines · Thomas Gleixner
Quoted 8 lines · Dmitry Vyukov
Quoted 7 lines · Andrey Ryabinin
2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <[email protected]>:
Quoted 5 lines
stack = (unsigned long)task_stack_page(p);(quoted diff) +3
-1- if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE) + /* The task can be already running at this point, so tread carefully. */ + fp = READ_ONCE(p->thread.sp); + if (fp < stack || fp >= stack+THREAD_SIZE)Quoted 6 lines
Quoted 1 line
Since we deference fp, it should be "|| fp + sizeof(u64) >= stack +
THREAD_SIZE"Good point.
I guess it should be "|| fp + sizeof(u64) > stack +THREAD_SIZE", because == is OK if we add 8.This whole mess with +8 and -16 and whatever is just crap. And all of it completely undocumented. Proper version below.
Thanks,
tglx
8<-------------------------------
Subject: x86/process: Add proper bound checks in 64bit
get_wchan()
From: Thomas Gleixner <[email protected]>
Date: Mon, 28 Sep 2015 17:16:52 +0200Dmitry Vyukov reported the following using trinity and the memory error detector AddressSanitizer
(https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel).[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.576801] ffff88002e280000 is located 131938492886538 bytes to the left of 28857600-byte region [ffffffff81282e0a, ffffffff82e0830a) [ 124.578633] Accessed by thread T10915: [ 124.579295] inlined in describe_heap_address ./arch/x86/mm/asan/report.c:164 [ 124.579295] #0 ffffffff810dd277 in asan_report_error ./arch/x86/mm/asan/report.c:278 [ 124.580137] #1 ffffffff810dc6a0 in asan_check_region ./arch/x86/mm/asan/asan.c:37 [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan
./arch/x86/kernel/process_64.c:444The address checks in the 64bit implementation of
get_wchan()are wrong in several ways:- The lower bound of the stack is not the start of the stack page. It's the start of the stack page plus sizeof (struct
thread_info)- The upper bound must be top of stack minus 2 * sizeof(unsigned long). This is required because the stack pointer points at the frame pointer. The layout on the stack is: ... IP FP ... IP FP.
Fix the bound checks and get rid of the mix of numeric constants, u64 and unsigned long. Making all unsigned long allows us to use the same function for 32bit as well.
Reported-by Dmitry Vyukov <[email protected]>Reported-by Sasha Levin <[email protected]>Based-on-patch-from: Wolfram Gloger <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>Cc Andrey Ryabinin <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Andrey Konovalov <[email protected]>--- arch/x86/kernel/process_64.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)Index:
tip/arch/x86/kernel/process_64.c
===================================================================arch/x86/kernel/process_64.c +2
-4@@ -501,24 +501,47 @@ EXPORT_SYMBOL_GPL(set_personality_ia32);unsigned long get_wchan (struct task_struct *p) { -unsigned long stack; -u64 fp, ip; +unsigned long start, bottom, top, sp, fp, ip;int count =0 ;if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; - stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +
arch/x86/kernel/process_64.c EXPORT_SYMBOL_GPL(set_personality_ia32); · line 518
Quoted 10 lines
arch/x86/kernel/process_64.c +8
-1+ start = (unsigned long )task_stack_page (p); +if (!start)return 0 ; - fp = *(u64 *)(p->thread.sp); + +/* +* Layout of the stack page: +* +* ----------- top = start = THREAD_SIZE - sizeof(unsigned long) +* stack
There's TOP_OF_KERNEL_STACK_PADDING in here, too. Arguably the padding is still in bounds, though. Also, I think you mean "start +", not "start =".
Quoted 21 lines
arch/x86/kernel/process_64.c +19
-1+* ----------- bottom = start + sizeof(thread_info) +* thread_info +* ----------- start +* +* The tasks stack pointer points at the location where the +* framepointer is stored. The data on the stack is: +* ... IP FP ... IP FP +* +* We need to read FP and IP, so we need to adjust the upper +* bound by another unsigned long. +*/ + top = start +THREAD_SIZE -2 *sizeof (unsigned long ); + bottom = start +sizeof (struct thread_info ); + + sp = p->thread.sp; +if (sp < bottom || sp > top) +return 0 ; + + fp = *(unsigned long *)sp; do { - if (fp < (unsigned long)stack ||
arch/x86/kernel/process_64.c EXPORT_SYMBOL_GPL(set_personality_ia32); · line 545
Quoted 10 lines
arch/x86/kernel/process_64.c +3
-3- fp >= (unsigned long)stack+THREAD_SIZE) +if (fp < bottom || fp > top)return 0 ; - ip = *(u64 *)(fp+8 ); + ip = *(unsigned long *)(fp +sizeof (unsigned long ));if (!in_sched_functions (ip))return ip; - fp = *(u64 *)fp; + fp = *(unsigned long *)fp; }while (count++ <16 );
I'm be vaguely amazed if this isn't an exploitable info leak even without the out of bounds thing. Can we really not find a way to do this without walking the stack?
The bounds checking looks okay, though.
--Andy
Quoted 146 lines · Andy Lutomirski
Quoted 94 lines · Thomas Gleixner
Quoted 8 lines · Dmitry Vyukov
Quoted 7 lines · Andrey Ryabinin
2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <[email protected]>:
Quoted 5 lines
stack = (unsigned long)task_stack_page(p);(quoted diff) +3
-1- if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE) + /* The task can be already running at this point, so tread carefully. */ + fp = READ_ONCE(p->thread.sp); + if (fp < stack || fp >= stack+THREAD_SIZE)Quoted 6 lines
Quoted 1 line
Since we deference fp, it should be "|| fp + sizeof(u64) >= stack +
THREAD_SIZE"Good point.
I guess it should be "|| fp + sizeof(u64) > stack +THREAD_SIZE", because == is OK if we add 8.This whole mess with +8 and -16 and whatever is just crap. And all of it completely undocumented. Proper version below.
Thanks,
tglx
8<-------------------------------
Subject: x86/process: Add proper bound checks in 64bit
get_wchan()
From: Thomas Gleixner <[email protected]>
Date: Mon, 28 Sep 2015 17:16:52 +0200Dmitry Vyukov reported the following using trinity and the memory error detector AddressSanitizer
(https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel).[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000 [ 124.576801] ffff88002e280000 is located 131938492886538 bytes to the left of 28857600-byte region [ffffffff81282e0a, ffffffff82e0830a) [ 124.578633] Accessed by thread T10915: [ 124.579295] inlined in describe_heap_address ./arch/x86/mm/asan/report.c:164 [ 124.579295] #0 ffffffff810dd277 in asan_report_error ./arch/x86/mm/asan/report.c:278 [ 124.580137] #1 ffffffff810dc6a0 in asan_check_region ./arch/x86/mm/asan/asan.c:37 [ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0 [ 124.581893] #3 ffffffff8107c093 in get_wchan
./arch/x86/kernel/process_64.c:444The address checks in the 64bit implementation of
get_wchan()are wrong in several ways:- The lower bound of the stack is not the start of the stack page. It's the start of the stack page plus sizeof (struct
thread_info)- The upper bound must be top of stack minus 2 * sizeof(unsigned long). This is required because the stack pointer points at the frame pointer. The layout on the stack is: ... IP FP ... IP FP.
Fix the bound checks and get rid of the mix of numeric constants, u64 and unsigned long. Making all unsigned long allows us to use the same function for 32bit as well.
Reported-by Dmitry Vyukov <[email protected]>Reported-by Sasha Levin <[email protected]>Based-on-patch-from: Wolfram Gloger <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>Cc Andrey Ryabinin <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Andrey Konovalov <[email protected]>--- arch/x86/kernel/process_64.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)Index:
tip/arch/x86/kernel/process_64.c
===================================================================arch/x86/kernel/process_64.c +2
-4@@ -501,24 +501,47 @@ EXPORT_SYMBOL_GPL(set_personality_ia32);unsigned long get_wchan (struct task_struct *p) { -unsigned long stack; -u64 fp, ip; +unsigned long start, bottom, top, sp, fp, ip;int count =0 ;if (!p || p == current || p->state ==TASK_RUNNING )return 0 ; - stack = (unsigned long )task_stack_page (p); -if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE ) +arch/x86/kernel/process_64.c EXPORT_SYMBOL_GPL(set_personality_ia32); · line 518
Quoted 10 lines
arch/x86/kernel/process_64.c +8
-1+ start = (unsigned long )task_stack_page (p); +if (!start)return 0 ; - fp = *(u64 *)(p->thread.sp); + +/* +* Layout of the stack page: +* +* ----------- top = start = THREAD_SIZE - sizeof(unsigned long) +* stack There's
TOP_OF_KERNEL_STACK_PADDINGin here, too. Arguably the padding is still in bounds, though. Also, I think you mean "start +", not "start =".Quoted 21 lines
arch/x86/kernel/process_64.c +19
-1+* ----------- bottom = start + sizeof(thread_info) +* thread_info +* ----------- start +* +* The tasks stack pointer points at the location where the +* framepointer is stored. The data on the stack is: +* ... IP FP ... IP FP +* +* We need to read FP and IP, so we need to adjust the upper +* bound by another unsigned long. +*/ + top = start +THREAD_SIZE -2 *sizeof (unsigned long ); + bottom = start +sizeof (struct thread_info ); + + sp = p->thread.sp; +if (sp < bottom || sp > top) +return 0 ; + + fp = *(unsigned long *)sp; do { - if (fp < (unsigned long)stack ||arch/x86/kernel/process_64.c EXPORT_SYMBOL_GPL(set_personality_ia32); · line 545
Quoted 10 lines
arch/x86/kernel/process_64.c +3
-3- fp >= (unsigned long)stack+THREAD_SIZE) +if (fp < bottom || fp > top)return 0 ; - ip = *(u64 *)(fp+8 ); + ip = *(unsigned long *)(fp +sizeof (unsigned long ));if (!in_sched_functions (ip))return ip; - fp = *(u64 *)fp; + fp = *(unsigned long *)fp; }while (count++ <16 );I'm be vaguely amazed if this isn't an exploitable info leak even without the out of bounds thing. Can we really not find a way to do this without walking the stack?
Quoted 1 line
The bounds checking looks okay, though.
Also, I like Borislav's READ_ONCE suggestion. Let's avoid TOCTOU due to optimization.
Re: a CVE: if anyone wants a CVE, ask oss-security. It's unclear to me exactly how one might exploit this.
--Andy
Borislav Petkov
maintainerQuoted 2 lines · Andy Lutomirski
Also, I like Borislav's
READ_ONCEsuggestion. Let's avoid TOCTOU due to optimization.
Dmitry's original patch did READ_ONCE already.
Quoted 2 lines
Re: a CVE: if anyone wants a CVE, ask oss-security. It's unclear to me exactly how one might exploit this.
I was just asking - it possibly leaking sensitive info and all.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.PATCH 1/1 fs/proc: Don't expose absolute kernel addresses via wchan
In reply to Andy LutomirskiQuoted 27 lines · Andy Lutomirski
Quoted 27 lines
(quoted diff) +21
-3+ * ----------- bottom = start + sizeof(thread_info) + * thread_info + * ----------- start + * + * The tasks stack pointer points at the location where the + * framepointer is stored. The data on the stack is: + * ... IP FP ... IP FP + * + * We need to read FP and IP, so we need to adjust the upper + * bound by another unsigned long. + */ + top = start + THREAD_SIZE - 2 * sizeof(unsigned long); + bottom = start + sizeof(struct thread_info); + + sp = p->thread.sp; + if (sp < bottom || sp > top) + return 0; + + fp = *(unsigned long *)sp; do { - if (fp < (unsigned long)stack || - fp >= (unsigned long)stack+THREAD_SIZE) + if (fp < bottom || fp > top) return 0; - ip = *(u64 *)(fp+8); + ip = *(unsigned long *)(fp + sizeof(unsigned long)); if (!in_sched_functions(ip))
Quoted 8 lines
Review
Quoted 4 lines
return ip;(quoted diff) +1
-1- fp = *(u64 *)fp; + fp = *(unsigned long *)fp; } while (count++ < 16);I'm be vaguely amazed if this isn't an exploitable info leak even without the out of bounds thing. Can we really not find a way to do this without walking the stack?
So wchan leaks absolute kernel addresses to unprivileged user-space, of kernel functions that sleep:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}So for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
So shouldn't we try to set all numeric output to 0 and only allow symbolic output via /proc/PID/wchan?
These days there's very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
( The absolute sleep address can generally still be profiled via perf, by tasks
with sufficient privileges. )
I.e. how about something like the patch below? (completely untested.)
Thanks,
Ingo
======================>
fs/proc/array.c | 2 +- fs/proc/base.c | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-)
fs/proc/array.c +1 -1
@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */ seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +1 -6
@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - }return 0 ; }
Re: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
fs/proc/array.c static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 510
Quoted 10 lines · Ingo Molnar
fs/proc/array.c +1
-1@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */
That should get rid of all wchan usage in do_task_stat()
Thanks,
tglx
PATCH v2 1/1 fs/proc: Don't expose absolute kernel addresses via wchan
Quoted 13 lines · Thomas Gleixner
fs/proc/array.c static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 510
Quoted 10 lines · Ingo Molnar
fs/proc/array.c +1
-1@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */ That should get rid of all wchan usage in
do_task_stat()
Indeed - updated patch attached.
Thanks,
Ingo
================================>
Quoted 1 line
From
985037cd05b379240dd381b29c2525758c665bb0Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Wed, 30 Sep 2015 09:15:37 +0200
Subject: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
So wchan leaks absolute kernel addresses to unprivileged user-space, of kernel functions that sleep:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}So for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
These days there's very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to 0 and keeps the symbolic output in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
--- fs/proc/array.c | 6 ++---- fs/proc/base.c | 7 +------ 2 files changed, 3 insertions(+), 10 deletions(-)
fs/proc/array.c +2 -4
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;
@@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,unlock_task_sighand (task, &flags); } -if (permitted && (!whole || num_threads <2 )) - wchan =get_wchan (task);if (!whole) { min_flt = task->min_flt; maj_flt = task->maj_flt;
@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */ seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +1 -6
@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - }return 0 ; }
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Quoted 8 lines · Ingo Molnar
These days there's very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to 0 and keeps the symbolic output in
/proc/PID/wchan.
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
In reply to Ingo MolnarQuoted 128 lines · Ingo Molnar
Quoted 13 lines · Thomas Gleixner
fs/proc/array.c static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 510
Quoted 10 lines · Ingo Molnar
fs/proc/array.c +1
-1@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */ That should get rid of all wchan usage in
do_task_stat()Indeed - updated patch attached.
Thanks,
Ingo
================================>
From985037cd05b379240dd381b29c2525758c665bb0Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Wed, 30 Sep 2015 09:15:37 +0200
Subject: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchanSo wchan leaks absolute kernel addresses to unprivileged user-space, of kernel functions that sleep:
static int proc_pid_wchan(struct
seq_file*m, structpid_namespace*ns,
struct pid *pid, structtask_struct*task){ unsigned long wchan; char symname[KSYM_NAME_LEN]; wchan = get_wchan(task); if (lookup_symbol_name(wchan, symname) < 0) { if (!ptrace_may_access(task, PTRACE_MODE_READ)) return 0; seq_printf(m, "%lu", wchan); } else { seq_printf(m, "%s", symname); } return 0; }So for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses
/proc/PID/wchan, not the absolute address in/proc/PID/stat:triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY)= 6These days there's very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to 0 and keeps the symbolic output in
/proc/PID/wchan.( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )Cc Al Viro <[email protected]>Cc Alexander Potapenko <[email protected]>Cc Andrey Konovalov <[email protected]>Cc Andrey Ryabinin <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Borislav Petkov <[email protected]>Cc Denys Vlasenko <[email protected]>Cc Dmitry Vyukov <[email protected]>Cc Kees Cook <[email protected]>Cc Kostya Serebryany <[email protected]>Cc Linus Torvalds <[email protected]>Cc Mike Galbraith <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Sasha Levin <[email protected]>Cc Thomas Gleixner <[email protected]>Cc kasan-dev <[email protected]>Signed-off-by Ingo Molnar <[email protected]>--- fs/proc/array.c | 6 ++---- fs/proc/base.c | 7 +------ 2 files changed, 3 insertions(+), 10 deletions(-)fs/proc/array.c +1
-3@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;@@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,unlock_task_sighand (task, &flags); } -if (permitted && (!whole || num_threads <2 )) - wchan =get_wchan (task);if (!whole) { min_flt = task->min_flt; maj_flt = task->maj_flt;
fs/proc/array.c static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 508
Quoted 5 lines
fs/proc/array.c +1
-1@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */
Probably should also update Documentation/filesystems/proc.txt with something like:
Documentation/filesystems/proc.txt +1 -1
@@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, was wchan, see /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
Quoted 12 lines
fs/proc/array.c +0
-0seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);fs/proc/base.c +0
-1@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) {
fs/proc/base.c static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, · line 437
Quoted 10 lines
fs/proc/base.c +1
-5-if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - }return 0 ; }
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Quoted 6 lines · Kees Cook
Probably should also update
Documentation/filesystems/proc.txtwith something like:
Documentation/filesystems/proc.txt Table 1-4: Contents of the stat files (as of 2.6.30-rc7) · line 316
Quoted 8 lines
Documentation/filesystems/proc.txt +1
-1@@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, was wchan, see /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
Good catch!
Thanks,
tglx
PATCH v3 1/1 fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
In reply to Kees CookQuoted 13 lines · Kees Cook
Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 508
Quoted 6 lines
(quoted diff) +1
-1@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL); seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL); seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL); - seq_put_decimal_ull(m, ' ', wchan); + seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */Probably should also update
Documentation/filesystems/proc.txtwith something like:
Documentation/filesystems/proc.txt Table 1-4: Contents of the stat files (as of 2.6.30-rc7) · line 316
Quoted 8 lines
Documentation/filesystems/proc.txt +1
-1@@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, was wchan, see /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
Indeed - I ended up clarifying both wchan explanations, see the changes below.
I also made the 'no symbols' output "0" (instead of an empty string), to better match the /proc/PID/stat behavior and previous output.
I'll push it out after a bit more testing and if nothing goes wrong I'll send this patch to Linus in the v4.4 merge window.
Thanks,
Ingo
============>
Quoted 1 line
From
bc43bb95763e5b215e389f75860eca0952ca4704Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field) leaks absolute kernel addresses to unprivileged user-space, of kernel functions that sleep:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan, if KALLSYMS is turned off or if the symbol lookup fails for some reason:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}This isn't ideal, because for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
These days there's very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to "0" and keeps only symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
--- Documentation/filesystems/proc.txt | 5 +++-- fs/proc/array.c | 6 ++---- fs/proc/base.c | 9 +++------ 3 files changed, 8 insertions(+), 12 deletions(-)
Documentation/filesystems/proc.txt +3 -2
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is + blocked in) symbol string. "0" if not blocked or !KALLSYMS. pagemap Page table stack Report full stack trace, enable via CONFIG_STACKTRACE smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
fs/proc/array.c +2 -4
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;
@@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,unlock_task_sighand (task, &flags); } -if (permitted && (!whole || num_threads <2 )) - wchan =get_wchan (task);if (!whole) { min_flt = task->min_flt; maj_flt = task->maj_flt;
@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); +seq_puts (m," 0" );/* Used to be numeric wchan - replaced by /proc/PID/wchan */ seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +3 -6
@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );return 0 ; }
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
2015-10-01 10:57 GMT+03:00 Ingo Molnar <[email protected]>:
Documentation/filesystems/proc.txt Table 1-1: Process specific entries in /proc · line 144
Quoted 11 lines
Documentation/filesystems/proc.txt +2
-1@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
/proc/PID/wchan is under #ifdef CONFIG_KALLSYMS.
Quoted 8 lines
fs/proc/base.c +0
-0@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task);
fs/proc/base.c static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, · line 436
Quoted 10 lines
fs/proc/base.c +3
-6-if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );
Maybe we should respect 'kptr_restrict' sysctl when we use '%ps', '%pB' etc. printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed). In that case you could just do 'seq_printf(m, "%ps", wchan)'.
OTOH, %ps, %pS are used mostly in debugging, so investigating some crash in production kernel with no !KALLSYMS and with kptr_restrict != 0 will be a nightmare.
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
Quoted 14 lines · Andrey Ryabinin
2015-10-01 10:57 GMT+03:00 Ingo Molnar <[email protected]>:
Documentation/filesystems/proc.txt Table 1-1: Process specific entries in /proc · line 144
Quoted 11 lines
Documentation/filesystems/proc.txt +2
-1@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
/proc/PID/wchanis under #ifdefCONFIG_KALLSYMS.
Yeah, indeed, so I clarified that text to now read:
+ wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.
Quoted 23 lines
Quoted 8 lines
fs/proc/base.c +0
-0@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task);fs/proc/base.c static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, · line 436
Quoted 10 lines
fs/proc/base.c +3
-6-if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );Maybe we should respect '
kptr_restrict' sysctl when we use '%ps', '%pB' etc. printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed). In that case you could just do 'seq_printf(m, "%ps", wchan)'.
Quoted 3 lines
OTOH, %ps, %pS are used mostly in debugging, so investigating some crash in production kernel with no !KALLSYMS and with
kptr_restrict!= 0 will be a nightmare.
So this code does not use %pX, it prints the symbol. Yes, the symbol in itself is 'information' about the execution of the task in itself - but /proc per se is all about providing information about tasks in the system (including to unprivileged users), so there's IMHO little point in restricting this output any further ...
I think ktrp_restrict is mostly about not exposing absolute addresses.
Thanks,
Ingo
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
Quoted 46 lines · Ingo Molnar
Quoted 14 lines · Andrey Ryabinin
2015-10-01 10:57 GMT+03:00 Ingo Molnar <[email protected]>:
Documentation/filesystems/proc.txt Table 1-1: Process specific entries in /proc · line 144
Quoted 11 lines
Documentation/filesystems/proc.txt +2
-1@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
/proc/PID/wchanis under #ifdefCONFIG_KALLSYMS.Yeah, indeed, so I clarified that text to now read:
+ wchan Present with
CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.Quoted 23 lines
Quoted 8 lines
fs/proc/base.c +0
-0@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task);fs/proc/base.c static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, · line 436
Quoted 10 lines
fs/proc/base.c +3
-6-if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );Maybe we should respect '
kptr_restrict' sysctl when we use '%ps', '%pB' etc. printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed). In that case you could just do 'seq_printf(m, "%ps", wchan)'.
Quoted 5 lines
Quoted 3 lines
OTOH, %ps, %pS are used mostly in debugging, so investigating some crash in production kernel with no !KALLSYMS and with
kptr_restrict!= 0 will be a nightmare.So this code does not use %pX, it prints the symbol.
I think you misunderstood me.
Yes, this code currently doesn't use %pX, but it could:
fs/proc/base.c +1 -12
@@ -425,18 +425,7 @@ static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,static int proc_pid_wchan (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task) { -unsigned long wchan; -char symname[KSYM_NAME_LEN ]; - - wchan =get_wchan (task); - -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { -seq_printf (m,"%s" , symname); - } +seq_printf (m,"%ps" ,get_wchan (task));return 0 ; }
There is a problem here, though. %ps will print absolute kernel address instead of symbol name if KALLSYMS=n or if resolution of address failed.
So I was wondering, may be should just fix %ps ?
i.e. print 0 instead of absolute address if KALLSYMS=n or lookup failure?
Quoted 7 lines
Yes, the symbol in itself is
'information' about the execution of the task in itself - but /proc per se is all about providing information about tasks in the system (including to unprivileged users), so there's IMHO little point in restricting this output any further ...I think
ktrp_restrictis mostly about not exposing absolute addresses.
Right, and '%ps' may expose absolute address if KALLSYMS=n or address lookup failed for some reason.
Quoted 4 lines
Thanks,
Ingo
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
Quoted 29 lines · Andrey Ryabinin
I think you misunderstood me.
Yes, this code currently doesn't use %pX, but it could:fs/proc/base.c +1
-12@@ -425,18 +425,7 @@ static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,static int proc_pid_wchan (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task) { -unsigned long wchan; -char symname[KSYM_NAME_LEN ]; - - wchan =get_wchan (task); - -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { -seq_printf (m,"%s" , symname); - } +seq_printf (m,"%ps" ,get_wchan (task));return 0 ; }
Quoted 4 lines
There is a problem here, though. %ps will print absolute kernel address instead of symbol name if KALLSYMS=n or if resolution of address failed.
So I was wondering, may be should just fix %ps ?
i.e. print 0 instead of absolute address if KALLSYMS=n or lookup failure?
There's another problem as well: your change loses the PTRACE_MODE_READ permission check.
But ... I think I like it open coded, which is good precisely because it will stay invariant even if we change details in the %ps/etc. debug output.
Thanks,
Ingo
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
2015-10-01 13:39 GMT+03:00 Ingo Molnar <[email protected]>:
Quoted 3 lines
There's another problem as well: your change loses the
PTRACE_MODE_READpermission check.
I guess, you should fix your patch than, because it removed this check likewise...
Quoted 4 lines
But ... I think I like it open coded, which is good precisely because it will stay invariant even if we change details in the %ps/etc. debug output.
Fair enough.
Quoted 3 lines
Thanks,
Ingo
PATCH v5 1/1 fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
Quoted 7 lines · Andrey Ryabinin
2015-10-01 13:39 GMT+03:00 Ingo Molnar <[email protected]>:
Quoted 3 lines
There's another problem as well: your change loses the
PTRACE_MODE_READpermission check.I guess, you should fix your patch than, because it removed this check likewise...
Doh, indeed. I didn't notice that bug in testing, because the wchan field of /proc/PID/stat was 0/1 according to the ptrace check so 'ps' masked the symbol accordingly - but the /proc/PID/wchan value was always accessible.
Updated patch below.
Thanks,
Ingo
=======================>
Quoted 1 line
From
b2f73922d119686323f14fbbe46587f863852328Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains the absolute kernel address of the kernel function a task is blocked in) leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if KALLSYMS is turned off or if the symbol lookup fails for some reason:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}This isn't ideal, because for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
There's one compatibility quirk here: procps relies on whether the absolute value is non-zero - and we can provide that functionality by outputing "0" or "1" depending on whether the task is blocked (whether there's a wchan address).
These days there appears to be very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to "0" or "1" and keeps only symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
--- Documentation/filesystems/proc.txt | 5 +++-- fs/proc/array.c | 16 ++++++++++++++-- fs/proc/base.c | 9 +++------ 3 files changed, 20 insertions(+), 10 deletions(-)
Documentation/filesystems/proc.txt +3 -2
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function + symbol the task is blocked in - or "0" if not blocked. pagemap Page table stack Report full stack trace, enable via CONFIG_STACKTRACE smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
fs/proc/array.c +14 -2
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp, wchan =0 ;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); + +/* +* We used to output the absolute kernel address, but that's an +* information leak - so instead we show a 0/1 flag here, to signal +* to user-space whether there's a wchan field in /proc/PID/wchan. +* +* This works with older implementations of procps as well. +*/ +if (wchan) +seq_puts (m," 1" ); +else +seq_puts (m," 0" ); +seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +3 -6
@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (wchan &&ptrace_may_access (task,PTRACE_MODE_READ ) && !lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );return 0 ; }
PATCH v4 1/1 fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
In reply to Ingo MolnarQuoted 27 lines · Ingo Molnar
Quoted 13 lines · Kees Cook
Review static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, · line 508
Quoted 6 lines
(quoted diff) +1
-1@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL); seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL); seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL); - seq_put_decimal_ull(m, ' ', wchan); + seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */Probably should also update
Documentation/filesystems/proc.txtwith something like:Documentation/filesystems/proc.txt Table 1-4: Contents of the stat files (as of 2.6.30-rc7) · line 316
Quoted 8 lines
Documentation/filesystems/proc.txt +1
-1@@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, was wchan, see /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exitIndeed - I ended up clarifying both wchan explanations, see the changes below.
Quoted 5 lines
I also made the 'no symbols' output "0" (instead of an empty string), to better match the
/proc/PID/statbehavior and previous output.I'll push it out after a bit more testing and if nothing goes wrong I'll send this patch to Linus in the v4.4 merge window.
Yeah, so testing uncovered the following additional ABI detail: procps relies on the wchan field in /proc/PID/stat, but only as a flag (in most cases), whether to look at /proc/PID/wchan.
To keep the ABI, the v4 patch below outputs not the absolute address, but a 0/1 flag to indicate whether the task is blocked and whether there's anything worth looking at in /proc/PID/wchan.
I tested this approach with procps and it seems to fully work. In fact due to the ptrace check we properly restrict the information to our own tasks only. root still sees the wchan field of all tasks.
Btw., the very latest procps-ng grew this nice change:
6b8dc5511fb9 ("library: refactor and rely on modern kernels for wchan")
which greatly simplified procps's handling of /proc/PID/wchan.
... but my testing was done with an older procps version.
Thanks,
Ingo
==========================>
Quoted 1 line
From
b26a16469b0b6f3f0604aafb95c50d1532b3fff2Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains the absolute kernel address of the kernel function a task is blocked in) leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if KALLSYMS is turned off or if the symbol lookup fails for some reason:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}This isn't ideal, because for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
There's one compatibility quirk here: procps relies on whether the absolute value is non-zero - and we can provide that functionality by outputing "0" or "1" depending on whether the task is blocked (whether there's a wchan address).
These days there appears to be very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to "0" or "1" and keeps only symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
--- Documentation/filesystems/proc.txt | 5 +++-- fs/proc/array.c | 16 ++++++++++++++-- fs/proc/base.c | 9 +++------ 3 files changed, 20 insertions(+), 10 deletions(-)
Documentation/filesystems/proc.txt +3 -2
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function + symbol the task is blocked in - or "0" if not blocked. pagemap Page table stack Report full stack trace, enable via CONFIG_STACKTRACE smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
fs/proc/array.c +14 -2
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp, wchan =0 ;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); + +/* +* We used to output the absolute kernel address, but that's an +* information leak - so instead we show a 0/1 flag here, to signal +* to user-space whether there's a wchan field in /proc/PID/wchan. +* +* This works with older implementations of procps as well. +*/ +if (wchan) +seq_puts (m," 1" ); +else +seq_puts (m," 0" ); +seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +3 -6
@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (!lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );return 0 ; }
tip-bot for Ingo Molnar
merge bot[tip:core/debug] fs/proc, core/debug: Don' t expose absolute kernel addresses via wchan
In reply to Ingo Molnarfs/proc, core/debug: Don't expose absolute kernel addresses via wchan
Commit-ID: b2f73922d119686323f14fbbe46587f863852328 Gitweb: http://git.kernel.org/tip/b2f73922d119686323f14fbbe46587f863852328 Author: Ingo Molnar <[email protected]> AuthorDate: Wed, 30 Sep 2015 15:59:17 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 1 Oct 2015 12:55:34 +0200
fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains the absolute kernel address of the kernel function a task is blocked in) leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if KALLSYMS is turned off or if the symbol lookup fails for some reason:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}This isn't ideal, because for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35) ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
There's one compatibility quirk here: procps relies on whether the absolute value is non-zero - and we can provide that functionality by outputing "0" or "1" depending on whether the task is blocked (whether there's a wchan address).
These days there appears to be very little legitimate reason user-space would be interested in the absolute address. The absolute address is mostly historic: from the days when we didn't have kallsyms and user-space procps had to do the decoding itself via the System.map.
So this patch sets all numeric output to "0" or "1" and keeps only symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
--- Documentation/filesystems/proc.txt | 5 +++-- fs/proc/array.c | 16 ++++++++++++++-- fs/proc/base.c | 9 +++------ 3 files changed, 20 insertions(+), 10 deletions(-)
Documentation/filesystems/proc.txt +3 -2
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc stat Process status statm Process memory status information status Process status in human readable form - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan + wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function + symbol the task is blocked in - or "0" if not blocked. pagemap Page table stack Report full stack trace, enable via CONFIG_STACKTRACE smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7) blocked bitmap of blocked signals sigign bitmap of ignored signals sigcatch bitmap of caught signals - wchan address where process went to sleep + 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead) 0 (place holder) 0 (place holder) exit_signal signal to send to parent thread on exit
fs/proc/array.c +14 -2
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,static int do_task_stat (struct seq_file *m,struct pid_namespace *ns,struct pid *pid,struct task_struct *task,int whole) { -unsigned long vsize, eip, esp, wchan = ~0UL ; +unsigned long vsize, eip, esp, wchan =0 ;int priority, nice;int tty_pgrp =-1 , tty_nr =0 ;sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,seq_put_decimal_ull (m,' ' , task->blocked.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigign.sig[0 ] &0x7fffffffUL );seq_put_decimal_ull (m,' ' , sigcatch.sig[0 ] &0x7fffffffUL ); -seq_put_decimal_ull (m,' ' , wchan); + +/* +* We used to output the absolute kernel address, but that's an +* information leak - so instead we show a 0/1 flag here, to signal +* to user-space whether there's a wchan field in /proc/PID/wchan. +* +* This works with older implementations of procps as well. +*/ +if (wchan) +seq_puts (m," 1" ); +else +seq_puts (m," 0" ); +seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ull (m,' ' ,0 );seq_put_decimal_ll (m,' ' , task->exit_signal);
fs/proc/base.c +3 -6
@@ -430,13 +430,10 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, wchan =get_wchan (task); -if (lookup_symbol_name (wchan, symname) <0 ) { -if (!ptrace_may_access (task,PTRACE_MODE_READ )) -return 0 ; -seq_printf (m,"%lu" , wchan); - }else { +if (wchan &&ptrace_may_access (task,PTRACE_MODE_READ ) && !lookup_symbol_name (wchan, symname))seq_printf (m,"%s" , symname); - } +else +seq_putc (m,'0' );return 0 ; }
Quoted 2 lines · Andy Lutomirski
I'm be vaguely amazed if this isn't an exploitable info leak even without the out of bounds thing.
The info leak happens in fs/proc, where we happily print arbitrary "IP" values, if we cant resolve a symbol.
Quoted 1 line
Can we really not find a way to do this without walking the stack?
We would have to add a 'store wait channel' mechanism to all functions which are the primary entry points to scheduling. Not impossible, but not pretty either.
If we want to prevent the stack changing under us, we'd need to take p->pi_lock and do the task != RUNNING check and the walk under it. I don't think we want to do that, unless there is a compelling reason to do so.
Thanks,
tglx