tracing: Add test for user space strings when filtering on string pointers
Subsystems (2)
Steven Rostedt
maintainerPATCH v2 1/1 tracing: Add test for user space strings when filtering on string pointers
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
kvm-03-guest16 login: [72198.026181] BUG: unable to handle page fault for address: 00007fffaae8ef60 #PF: supervisor read access in kernel mode
#PF: error_code(0x0001) - permissions violation
PGD 80000001008b7067 P4D 80000001008b7067 PUD 2393f1067 PMD 2393ec067 PTE 8000000108f47867
Oops: 0001 [#1] PREEMPT SMP PTI
CPU: 1 PID: 1 Comm: systemd Kdump: loaded Not tainted 5.14.0-32.el9.x86_64 #1
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:strlen+0x0/0x20
Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11
48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8
48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
RSP: 0018:ffffb5b900013e48 EFLAGS: 00010246
RAX: 0000000000000018 RBX: ffff8fc1c49ede00 RCX: 0000000000000000
RDX: 0000000000000020 RSI: ffff8fc1c02d601c RDI: 00007fffaae8ef60
RBP: 00007fffaae8ef60 R08: 0005034f4ddb8ea4 R09: 0000000000000000
R10: ffff8fc1c02d601c R11: 0000000000000000 R12: ffff8fc1c8a6e380
R13: 0000000000000000 R14: ffff8fc1c02d6010 R15: ffff8fc1c00453c0
FS: 00007fa86123db40(0000) GS:ffff8fc2ffd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fffaae8ef60 CR3: 0000000102880001 CR4: 00000000007706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
filter_pred_pchar+0x18/0x40
filter_match_preds+0x31/0x70
ftrace_syscall_enter+0x27a/0x2c0
syscall_trace_enter.constprop.0+0x1aa/0x1d0
do_syscall_64+0x16/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7fa861d88664To be even more robust, test both kernel and user space strings. If the string fails to read, then simply have the filter fail.
---
Changes since v1: https://lkml.kernel.org/r/[email protected]
- Up the size of temp buffer from 512 to 1024
Still less than PATH_MAX, but should be good enough in real systems. If this is an issue, then we can add a way for users to updated it.
- Add documentation about the limit of comparing to string pointers.
- Add typecast from str to ustr to denote adding "__user"
- Allocate buffer only when comparing with string pointers.
- Only do the test on string pointers and not all string compares.
Documentation/trace/events.rst | 10 +++++ kernel/trace/trace_events_filter.c | 59 ++++++++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 3 deletions(-)
Documentation/trace/events.rst +10 -0
@@ -230,6 +230,16 @@ Currently the caret ('^') for an error always appears at the beginning of the filter string; the error message should still be useful though even without more accurate position info. +5.2.1 Filter limitations +------------------------ + +If a filter is placed on a string pointer ``(char *)`` that does not point +to a string on the ring buffer, but instead points to kernel or user space +memory, then, for safety reasons, at most 1024 bytes of the content is +copied onto a temporary buffer to do the compare. If the copy of the memory +faults (the pointer points to memory that should not be accessed), then the +string compare will be treated as not matching. + 5.3 Clearing filters --------------------
kernel/trace/trace_events_filter.c +56 -3
92 lines not on this page · raw message
Signature · 1 line
2.33.0
From: Steven Rostedt
Quoted 6 lines
Sent: 10 January 2022 16:56
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
Quoted 7 lines
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
...
kernel/trace/trace_events_filter.c DEFINE_EQUALITY_PRED(32); · line 687
Quoted 9 lines
kernel/trace/trace_events_filter.c +9
-0+if (likely ((unsigned long )str >=TASK_SIZE )) { +/* For safety, do not trust the string pointer */ +if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE )) +return NULL ; + }else { +/* user space address? */ + ustr = (char __user *)str; +if (!strncpy_from_user_nofault (kstr, ustr,USTRING_BUF_SIZE )) +return NULL ;
Is that check against TASK_SIZE even correct for all architectures? copy_to/from_user() uses access_ok() - which is architecture dependant.
I think you need to remember where the pointer came from.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Steven Rostedt
maintainerQuoted 15 lines · On Mon, 10 Jan 2022 17:11:52 +0000 David Laight <[email protected]> wrote:
From: Steven Rostedt
Quoted 6 lines
Sent: 10 January 2022 16:56
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
Quoted 7 lines
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
Quoted 5 lines
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
Quoted 14 lines
...
kernel/trace/trace_events_filter.c DEFINE_EQUALITY_PRED(32); · line 687
Quoted 9 lines
kernel/trace/trace_events_filter.c +9
-0+if (likely ((unsigned long )str >=TASK_SIZE )) { +/* For safety, do not trust the string pointer */ +if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE )) +return NULL ; + }else { +/* user space address? */ + ustr = (char __user *)str; +if (!strncpy_from_user_nofault (kstr, ustr,USTRING_BUF_SIZE )) +return NULL ;Is that check against
TASK_SIZEeven correct for all architectures? copy_to/from_user() usesaccess_ok()- which is architecture dependant.
The problem with access_ok() (which I tried first) is that it can't be used from interrupt context, and this check can happen in interrupt context. Either way, if we pick the wrong one for the arch, the only thing bad that can happen is that it returns "fault" and the filter fails, just like if the pointer was to bad memory.
Quoted 3 lines
I think you need to remember where the pointer came from.
Why?
-- Steve
Steven Rostedt
maintainerQuoted 16 lines · On Mon, 10 Jan 2022 12:24:36 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 10 lines
Quoted 9 lines
Pingfan reported that the following causes a fault:
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
Quoted 5 lines
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
Quoted 4 lines
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
BTW, this patch doesn't give any new access to anything. It only protects if the pointer is pointing someplace that will give a fault. The user could today point that pointer to anything in kernel memory and trace it (but only read it if you are root). And worse, if the pointer is to something that can fault, it will trigger a BUG(). This could happen if root creates the filter, and a non-privileged user sets that pointer to something bad. Which means anyone can cause the machine to fault if root does this kind of tracing. This is why I added the stable tag.
This patch is to prevent that BUG() from happening.
-- Steve
From: Steven Rostedt
Quoted 1 line
Sent: 10 January 2022 17:25
...
Quoted 14 lines
Quoted 13 lines
...
kernel/trace/trace_events_filter.c DEFINE_EQUALITY_PRED(32); · line 687
Quoted 9 lines
kernel/trace/trace_events_filter.c +9
-0+if (likely ((unsigned long )str >=TASK_SIZE )) { +/* For safety, do not trust the string pointer */ +if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE )) +return NULL ; + }else { +/* user space address? */ + ustr = (char __user *)str; +if (!strncpy_from_user_nofault (kstr, ustr,USTRING_BUF_SIZE )) +return NULL ;Is that check against
TASK_SIZEeven correct for all architectures? copy_to/from_user() usesaccess_ok()- which is architecture dependant.
Quoted 5 lines
The problem with
access_ok()(which I tried first) is that it can't be used from interrupt context, and this check can happen in interrupt context. Either way, if we pick the wrong one for the arch, the only thing bad that can happen is that it returns "fault" and the filter fails, just like if the pointer was to bad memory.
Isn't there also at least one architecture where you can't differentiate between user and kernel pointers by looking at the address? (Something like sparc ASI is used for user accesses so both user and kernel get the full 4G address range. But it isn't sparc (or pdp/11)) ISTR it causing issues with the code for kernel_setsockopt() and required a separate flag.
Put that together with something that needs user_access_begin() to bracket user accesses and you probably fail big-time.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 23 lines · David Laight
From: Steven Rostedt
Quoted 1 line
Sent: 10 January 2022 17:25
...
Quoted 14 lines
Quoted 13 lines
...
kernel/trace/trace_events_filter.c DEFINE_EQUALITY_PRED(32); · line 687
Quoted 9 lines
kernel/trace/trace_events_filter.c +9
-0+if (likely ((unsigned long )str >=TASK_SIZE )) { +/* For safety, do not trust the string pointer */ +if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE )) +return NULL ; + }else { +/* user space address? */ + ustr = (char __user *)str; +if (!strncpy_from_user_nofault (kstr, ustr,USTRING_BUF_SIZE )) +return NULL ;Is that check against
TASK_SIZEeven correct for all architectures? copy_to/from_user() usesaccess_ok()- which is architecture dependant.Quoted 5 lines
The problem with
access_ok()(which I tried first) is that it can't be used from interrupt context, and this check can happen in interrupt context. Either way, if we pick the wrong one for the arch, the only thing bad that can happen is that it returns "fault" and the filter fails, just like if the pointer was to bad memory.
Quoted 6 lines
Isn't there also at least one architecture where you can't differentiate between user and kernel pointers by looking at the address? (Something like sparc ASI is used for user accesses so both user and kernel get the full 4G address range. But it isn't sparc (or pdp/11)) ISTR it causing issues with the code for
kernel_setsockopt()and required a separate flag.
On s390 TASK_SIZE is defined as -PAGE_SIZE, so with the patch above the kernel would always try to fetch it from user space. I think it would be the same for parisc.
Quoted 8 lines
Put that together with something that needs
user_access_begin()to bracket user accesses and you probably fail big-time.David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Steven Rostedt
maintainerQuoted 10 lines · On Tue, 11 Jan 2022 21:55:53 +0100 Sven Schnelle <[email protected]> wrote:
Quoted 6 lines
Isn't there also at least one architecture where you can't differentiate between user and kernel pointers by looking at the address? (Something like sparc ASI is used for user accesses so both user and kernel get the full 4G address range. But it isn't sparc (or pdp/11)) ISTR it causing issues with the code for
kernel_setsockopt()and required a separate flag.On s390
TASK_SIZEis defined as -PAGE_SIZE, so with the patch above the kernel would always try to fetch it from user space. I think it would be the same for parisc.
As a work around for these cases, would something like this work?
-- Steve
kernel/trace/trace_events_filter.c +9 -1
@@ -676,7 +676,15 @@ static __always_inline char *test_string(char *str) ubuf =this_cpu_ptr (ustring_per_cpu); kstr = ubuf->buffer; -if (likely ((unsigned long )str >=TASK_SIZE )) { +/* +* Test the address of ustring_per_cpu against TASK_SIZE, as +* comparing TASK_SIZE to determine kernel/user space address +* is not enough on some architectures. If the address is less +* than TASK_SIZE we know this is the case, in which we should +* always use the from_kernel variant. +*/ +if ((unsigned long )&ustring_per_cpu < (unsigned long )TASK_SIZE || +likely ((unsigned long )str >=TASK_SIZE )) {/* For safety, do not trust the string pointer */ if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE ))return NULL ;
Hi Steve,
Quoted 10 lines · Steven Rostedt
Quoted 7 lines · On Tue, 11 Jan 2022 21:55:53 +0100 Sven Schnelle <[email protected]> wrote:
Quoted 6 lines
Isn't there also at least one architecture where you can't differentiate between user and kernel pointers by looking at the address? (Something like sparc ASI is used for user accesses so both user and kernel get the full 4G address range. But it isn't sparc (or pdp/11)) ISTR it causing issues with the code for
kernel_setsockopt()and required a separate flag.
Quoted 5 lines
Quoted 3 lines
On s390
TASK_SIZEis defined as -PAGE_SIZE, so with the patch above the kernel would always try to fetch it from user space. I think it would be the same for parisc.As a work around for these cases, would something like this work?
Hmm, i don't see how. On s390, TASK_SIZE is -PAGE_SIZE, which means 0xfffffffffffff000 so i think the if() condition below is always true.
Too bad that the __user attribute is stripped during a normal compile. But couldn't we add the information whether a pointer belongs to user or kernel space in the trace event definition? For syscall tracing it's easy, because pointer types in SYSCALL_DEFINE() and friends are always userspace pointers?
Quoted 23 lines
-- Steve
kernel/trace/trace_events_filter.c +9
-1@@ -676,7 +676,15 @@ static __always_inline char *test_string(char *str) ubuf =this_cpu_ptr (ustring_per_cpu); kstr = ubuf->buffer; -if (likely ((unsigned long )str >=TASK_SIZE )) { +/* +* Test the address of ustring_per_cpu against TASK_SIZE, as +* comparing TASK_SIZE to determine kernel/user space address +* is not enough on some architectures. If the address is less +* than TASK_SIZE we know this is the case, in which we should +* always use the from_kernel variant. +*/ +if ((unsigned long )&ustring_per_cpu < (unsigned long )TASK_SIZE || +likely ((unsigned long )str >=TASK_SIZE )) {/* For safety, do not trust the string pointer */ if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE ))return NULL ;
Steven Rostedt
maintainerQuoted 3 lines · On Thu, 13 Jan 2022 22:28:01 +0100 Sven Schnelle <[email protected]> wrote:
Hmm, i don't see how. On s390,
TASK_SIZEis -PAGE_SIZE, which means0xfffffffffffff000so i think theif()condition below is always true.
Yes, I did that to just use the kernel version and not the user space one.
This is just a workaround for now.
Quoted 6 lines
Too bad that the
__userattribute is stripped during a normal compile. But couldn't we add the information whether a pointer belongs to user or kernel space in the trace event definition? For syscall tracing it's easy, because pointer types inSYSCALL_DEFINE()and friends are always userspace pointers?
We could add something later. As it is currently the merge window, and this is a real bug, I'm going to just leave it as is, and we can work to fix the other archs later. I need to get a pull request ready by tomorrow.
-- Steve
Steven Rostedt
maintainerQuoted 3 lines · On Thu, 13 Jan 2022 16:51:15 -0500 Steven Rostedt <[email protected]> wrote:
We could add something later. As it is currently the merge window, and this is a real bug, I'm going to just leave it as is, and we can work to fix the other archs later. I need to get a pull request ready by tomorrow.
Actually I got this working, and looks like a reasonable answer to the problem. It basically requires that the user specify that the pointer points into user space for the kernel to read it.
Thus instead of:
echo 'filename ~ "trace"' > events/syscalls/sys_enter_openat/filter
They must now do:
echo 'filename.ustring ~ "trace"' > events/syscalls/sys_enter_openat/filter
I updated the documentation to reflect this.
-- Steve
From: Steven Rostedt <[email protected]>
Subject: [PATCH] tracing: Add ustring operation to filtering string pointers
Since referencing user space pointers is special, if the user wants to filter on a field that is a pointer to user space, then they need to specify it.
Add a ".ustring" attribute to the field name for filters to state that the field is pointing to user space such that the kernel can take the appropriate action to read that pointer.
--- Documentation/trace/events.rst | 9 ++++ kernel/trace/trace_events_filter.c | 81 +++++++++++++++++++++--------- 2 files changed, 66 insertions(+), 24 deletions(-)
Documentation/trace/events.rst +9 -0
@@ -198,6 +198,15 @@ The glob (~) accepts a wild card character (\*,?) and character classes prev_comm ~ "*sh*" prev_comm ~ "ba*sh" +If the field is a pointer that points into user space (for example +"filename" from sys_enter_openat), then you have to append ".ustring" to the +field name:: + + filename.ustring ~ "password" + +As the kernel will have to know how to retrieve the memory that the pointer +is at from user space. + 5.2 Setting filters -------------------
kernel/trace/trace_events_filter.c +57 -24
129 lines not on this page · raw message
Signature · 1 line
2.33.0
Sven Schnelle
Tested-byQuoted 13 lines · Steven Rostedt
Quoted 3 lines · On Thu, 13 Jan 2022 16:51:15 -0500 Steven Rostedt <[email protected]> wrote:
We could add something later. As it is currently the merge window, and this is a real bug, I'm going to just leave it as is, and we can work to fix the other archs later. I need to get a pull request ready by tomorrow.
Actually I got this working, and looks like a reasonable answer to the problem. It basically requires that the user specify that the pointer points into user space for the kernel to read it.
Thus instead of:
Quoted 7 lines
echo 'filename ~ "trace"' > events/syscalls/sys_enter_openat/filter
They must now do:
echo 'filename.ustring ~ "trace"' > events/syscalls/sys_enter_openat/filter
I updated the documentation to reflect this.
Looks good and works on s390, feel free to add my tested-by:
Thanks!
From: Steven Rostedt
Quoted 5 lines
Sent: 14 January 2022 02:15
On Thu, 13 Jan 2022 16:51:15 -0500
Steven Rostedt <[email protected]> wrote:
Quoted 7 lines
Quoted 3 lines
We could add something later. As it is currently the merge window, and this is a real bug, I'm going to just leave it as is, and we can work to fix the other archs later. I need to get a pull request ready by tomorrow.
Actually I got this working, and looks like a reasonable answer to the problem. It basically requires that the user specify that the pointer points into user space for the kernel to read it.
Certainly look better.
I'm not sure about the difference between the _inatomic() and _nocheck() user access functions though.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Sven Schnelle
Quoted 16 lines
Sent: 13 January 2022 21:28
Hi Steve,
Quoted 10 lines · Steven Rostedt
Quoted 7 lines · On Tue, 11 Jan 2022 21:55:53 +0100 Sven Schnelle <[email protected]> wrote:
Quoted 6 lines
Isn't there also at least one architecture where you can't differentiate between user and kernel pointers by looking at the address? (Something like sparc ASI is used for user accesses so both user and kernel get the full 4G address range. But it isn't sparc (or pdp/11)) ISTR it causing issues with the code for
kernel_setsockopt()and required a separate flag.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
On s390
TASK_SIZEis defined as -PAGE_SIZE, so with the patch above the kernel would always try to fetch it from user space. I think it would be the same for parisc.As a work around for these cases, would something like this work?
Hmm, i don't see how. On s390,
TASK_SIZEis -PAGE_SIZE, which means0xfffffffffffff000so i think theif()condition below is always true.
Isn't TASK_SIZE also dependant on current->xxxx on at least one architecture? Possibly even x86-64.
Quoted 5 lines
Too bad that the
__userattribute is stripped during a normal compile. But couldn't we add the information whether a pointer belongs to user or kernel space in the trace event definition? For syscall tracing it's easy, because pointer types inSYSCALL_DEFINE()and friends are always userspace pointers?
Also, when the __user pointer is saved it MUST be checked for being a valid user pointer (eg with access_ok(ptr, 1).
You really do need to remember whether the pointer is user or kernel when you save it.
I also suspect that you need to check for contexts where 'current' isn't really valid (eg any kind on interrupt) and ensure the user reads aren't even attempted.
The excuse of being 'root' in order to request/read trace isn't really a very good one.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Steven Rostedt
maintainerQuoted 8 lines · On Thu, 13 Jan 2022 22:11:35 +0000 David Laight <[email protected]> wrote:
Quoted 5 lines
Too bad that the
__userattribute is stripped during a normal compile. But couldn't we add the information whether a pointer belongs to user or kernel space in the trace event definition? For syscall tracing it's easy, because pointer types inSYSCALL_DEFINE()and friends are always userspace pointers?Also, when the
__userpointer is saved it MUST be checked for being a valid user pointer (eg withaccess_ok(ptr, 1).
It's rather hard to even know if a pointer is __user or not. It could be some random address in any event field.
Quoted 7 lines
You really do need to remember whether the pointer is user or kernel when you save it.
I also suspect that you need to check for contexts where 'current' isn't really valid (eg any kind on interrupt) and ensure the user reads aren't even attempted.
It's not going to crash, even if it is required. The strncpy_from_user/kernel_nofault() should detect any of that, right? Or are those functions not safe to call?
Quoted 2 lines
The excuse of being 'root' in order to request/read trace isn't really a very good one.
Not sure what you are getting at here? If you are worried about tracing reading anything, then disable it. There's a lockdown on tracing. for those that are worried.
Heck I could just do:
# echo 'p:random_umem __common_interrupt add=+0(@0x7f073a188000):u64' > /sys/kernel/tracing/kprobe_events
# trace-cmd start -e random_umem
# trace-cmd show
# tracer: nop
#
# entries-in-buffer/entries-written: 15/15 #P:8
#
# _-----=> irqs-off/BH-disabled
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / _-=> migrate-disable
# |||| / delay
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
<idle>-0 [000] d.h1. 844.051612: random_umem: (__common_interrupt+0x0/0x100) add=7291432837672293
<idle>-0 [000] d.h1. 844.051694: random_umem: (__common_interrupt+0x0/0x100) add=74591483011526501
<idle>-0 [000] d.H1. 844.051743: random_umem: (__common_interrupt+0x0/0x100) add=21474836483
<idle>-0 [000] d.h1. 844.383333: random_umem: (__common_interrupt+0x0/0x100) add=115964116992
<idle>-0 [000] d.h1. 844.383802: random_umem: (__common_interrupt+0x0/0x100) add=1125968626319387
<idle>-0 [000] d.h1. 844.383864: random_umem: (__common_interrupt+0x0/0x100) add=115964116992
<idle>-0 [000] d.h1. 844.533321: random_umem: (__common_interrupt+0x0/0x100) add=4032068056083813
<idle>-0 [000] d.h1. 844.533801: random_umem: (__common_interrupt+0x0/0x100) add=6929886302
kworker/0:1-15 [000] d.h.. 844.900412: random_umem: (__common_interrupt+0x0/0x100) add=17367183
<idle>-0 [000] d.h1. 844.933428: random_umem: (__common_interrupt+0x0/0x100) add=115964116992
<idle>-0 [000] d.h1. 844.933818: random_umem: (__common_interrupt+0x0/0x100) add=17367183
<idle>-0 [000] d.h1. 844.933955: random_umem: (__common_interrupt+0x0/0x100) add=74591483008647173
<idle>-0 [000] d.h1. 845.364181: random_umem: (__common_interrupt+0x0/0x100) add=1125968626319387
bash-1759 [000] d.h.. 845.364541: random_umem: (__common_interrupt+0x0/0x100) add=13912554418208784
bash-1759 [000] d.h.. 845.364592: random_umem: (__common_interrupt+0x0/0x100) add=18446744071906760329And at every interrupt I get the memory at: 0x7f073a188000 for the current running task that was running when the interrupt happened.
As I said. It's an API.
-- Steve
From: Steven Rostedt
Quoted 26 lines
Sent: 10 January 2022 17:25
Quoted 15 lines · On Mon, 10 Jan 2022 17:11:52 +0000 David Laight <[email protected]> wrote:
From: Steven Rostedt
Quoted 6 lines
Sent: 10 January 2022 16:56
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
Quoted 7 lines
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
Quoted 5 lines
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
Quoted 4 lines
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
Is this filtering trace using a filename passed to a system call by a user program? In which case a user program can set up a system call that normally fails (because the copy_from_user() errors) but if root tries to run a system call event trace on that process can read arbitrary addresses and
thus crash the system?
While unlikely root might be persuaded to try to run the trace.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Steven Rostedt
maintainerQuoted 5 lines · On Mon, 10 Jan 2022 22:03:20 +0000 David Laight <[email protected]> wrote:
Quoted 4 lines
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
Quoted 7 lines
Is this filtering trace using a filename passed to a system call by a user program? In which case a user program can set up a system call that normally fails (because the
copy_from_user()errors) but if root tries to run a system call event trace on that process can read arbitrary addresses and
thus crash the system?While unlikely root might be persuaded to try to run the trace.
Yes. That's exactly what the code does today, and why it's a bug.
This patch instead uses copy_from_user_nofault/copy_from_kernel_nofault and copies it into a temp buffer and then compares against that.
If a user passes in a crazy pointer, the copy_from_user/kernel_nofault() will not read it, and the filter simply fails to match. Nothing bad will happen.
-- Steve
Hi Steven,
Sorry that I am out of office, and not reply in time.
Quoted 10 lines · Steven Rostedt
Quoted 7 lines · On Mon, 10 Jan 2022 17:11:52 +0000 David Laight <[email protected]> wrote:
From: Steven Rostedt
Quoted 6 lines
Sent: 10 January 2022 16:56
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
For accurate commit log, the swapped-out user page is not the root cause of this bug is "supervisor read access in kernel mode". And it is trueth that swapped-out user page can trigger a bug here, but it should be a different a stack.
Quoted 7 lines
Quoted 6 lines
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
Quoted 5 lines
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
I am not sure about the opposite way. Since kernel is not allowed to access userspace most of the time, then is it an leakage, which looks like:
use tracepoint as trampoline to uaccess.
read out user info from ustring_per_cpu
But any kernel code can call copy_from_user() function family freely, so it is not a problem caused by this patch, right? Or ustring_per_cpu should be zeroed out.
For V2, feel free to add "Tested-by"
Thanks,
Pingfan
Quoted 28 lines
Quoted 14 lines
...
kernel/trace/trace_events_filter.c DEFINE_EQUALITY_PRED(32); · line 687
Quoted 9 lines
kernel/trace/trace_events_filter.c +9
-0+if (likely ((unsigned long )str >=TASK_SIZE )) { +/* For safety, do not trust the string pointer */ +if (!strncpy_from_kernel_nofault (kstr, str,USTRING_BUF_SIZE )) +return NULL ; + }else { +/* user space address? */ + ustr = (char __user *)str; +if (!strncpy_from_user_nofault (kstr, ustr,USTRING_BUF_SIZE )) +return NULL ;Is that check against
TASK_SIZEeven correct for all architectures? copy_to/from_user() usesaccess_ok()- which is architecture dependant.The problem with
access_ok()(which I tried first) is that it can't be used from interrupt context, and this check can happen in interrupt context. Either way, if we pick the wrong one for the arch, the only thing bad that can happen is that it returns "fault" and the filter fails, just like if the pointer was to bad memory.Quoted 3 lines
I think you need to remember where the pointer came from.
Why?
-- Steve
Steven Rostedt
maintainerQuoted 3 lines · On Wed, 12 Jan 2022 12:13:03 +0800 Pingfan Liu <[email protected]> wrote:
Hi Steven,
Sorry that I am out of office, and not reply in time.
And I've been very sick for the last few days :-p
Quoted 20 lines · Steven Rostedt
Quoted 10 lines
Quoted 7 lines · On Mon, 10 Jan 2022 17:11:52 +0000 David Laight <[email protected]> wrote:
From: Steven Rostedt
Quoted 6 lines
Sent: 10 January 2022 16:56
From: Steven Rostedt <[email protected]>
Pingfan reported that the following causes a fault:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
Quoted 4 lines
For accurate commit log, the swapped-out user page is not the root cause of this bug is "supervisor read access in kernel mode". And it is trueth that swapped-out user page can trigger a bug here, but it should be a different a stack.
Yeah, it's true that the bug that triggered is the supervisor access, but that's more of a security concern (and an option that triggers this, to find bugs of this kind :-) To me, the real bug is the random read of a pointer. I can update the change log to be a bit more specific.
Quoted 20 lines
Quoted 8 lines
Quoted 7 lines
If a userspace pointer can end up the kernel structure then presumably a 'dodgy' user program can supply an arbitrary kernel address instead? This may give the user the ability to read arbitrary kernel addresses (including ones that are mapped to PCIe IO addresses). Doesn't sound good at all.
Quoted 5 lines
Only root has access to the information read here. All tracing requires root or those explicitly given access to the tracing data, which pretty much allows all access to kernel internals (including all memory). So nothing to worry about here ;-)
I am not sure about the opposite way. Since kernel is not allowed to access userspace most of the time, then is it an leakage, which looks like:
use tracepoint as trampoline to uaccess.
read out user info fromustring_per_cpu
Quoted 3 lines
But any kernel code can call
copy_from_user()function family freely, so it is not a problem caused by this patch, right? Orustring_per_cpushould be zeroed out.
And you can use tracing to create a kprobe that can read user space at almost any place in the kernel. Hence, we have API that allows root to do this, which is why I'm not concerned about it.
Quoted 4 lines
For V2, feel free to add "Tested-by"
Thanks,
-- Steve
Quoted 9 lines · On Wed, 12 Jan 2022 12:13:03 +0800 Pingfan Liu <[email protected]> wrote:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. If the string is not loaded into memory yet, it will trigger a fault in kernel space:
For accurate commit log, the swapped-out user page is not the root cause of this bug is "supervisor read access in kernel mode". And it is trueth that swapped-out user page can trigger a bug here, but it should be a different a stack.
I updated the change log.
Quoted 2 lines
For V2, feel free to add "Tested-by"
Thanks.
I also added a comment in the code. Here's v3:
-- Steve
From: Steven Rostedt <[email protected]>
Subject: [PATCH] tracing: Add test for user space strings when filtering on
string pointers
Pingfan reported that the following causes a fault:
echo "filename ~ \"cpu\"" > events/syscalls/sys_enter_openat/filter echo 1 > events/syscalls/sys_enter_at/enable
The reason is that trace event filter treats the user space pointer defined by "filename" as a normal pointer to compare against the "cpu" string. The following bug happened:
kvm-03-guest16 login: [72198.026181] BUG: unable to handle page fault for address: 00007fffaae8ef60 #PF: supervisor read access in kernel mode
#PF: error_code(0x0001) - permissions violation
PGD 80000001008b7067 P4D 80000001008b7067 PUD 2393f1067 PMD 2393ec067 PTE 8000000108f47867
Oops: 0001 [#1] PREEMPT SMP PTI
CPU: 1 PID: 1 Comm: systemd Kdump: loaded Not tainted 5.14.0-32.el9.x86_64 #1
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:strlen+0x0/0x20
Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11
48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8
48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
RSP: 0018:ffffb5b900013e48 EFLAGS: 00010246
RAX: 0000000000000018 RBX: ffff8fc1c49ede00 RCX: 0000000000000000
RDX: 0000000000000020 RSI: ffff8fc1c02d601c RDI: 00007fffaae8ef60
RBP: 00007fffaae8ef60 R08: 0005034f4ddb8ea4 R09: 0000000000000000
R10: ffff8fc1c02d601c R11: 0000000000000000 R12: ffff8fc1c8a6e380
R13: 0000000000000000 R14: ffff8fc1c02d6010 R15: ffff8fc1c00453c0
FS: 00007fa86123db40(0000) GS:ffff8fc2ffd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fffaae8ef60 CR3: 0000000102880001 CR4: 00000000007706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
filter_pred_pchar+0x18/0x40
filter_match_preds+0x31/0x70
ftrace_syscall_enter+0x27a/0x2c0
syscall_trace_enter.constprop.0+0x1aa/0x1d0
do_syscall_64+0x16/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7fa861d88664The above happened because the kernel tried to access user space directly and triggered a "supervisor read access in kernel mode" fault. Worse yet, the memory could not even be loaded yet, and a SEGFAULT could happen as well. This could be true for kernel space accessing as well.
To be even more robust, test both kernel and user space strings. If the string fails to read, then simply have the filter fail.
Note, TASK_SIZE is used to determine if the pointer is user or kernel space and the appropriate strncpy_from_kernel/user_nofault() function is used to copy the memory. For some architectures, the compare to TASK_SIZE may always pick user space or kernel space. If it gets it wrong, the only thing is that the filter will fail to match. In the future, this needs to be fixed to have the event denote which should be used. But failing a filter is much better than panicing the machine, and that can be solved later.
--- Documentation/trace/events.rst | 10 +++++ kernel/trace/trace_events_filter.c | 66 ++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 3 deletions(-)
Documentation/trace/events.rst +10 -0
@@ -230,6 +230,16 @@ Currently the caret ('^') for an error always appears at the beginning of the filter string; the error message should still be useful though even without more accurate position info. +5.2.1 Filter limitations +------------------------ + +If a filter is placed on a string pointer ``(char *)`` that does not point +to a string on the ring buffer, but instead points to kernel or user space +memory, then, for safety reasons, at most 1024 bytes of the content is +copied onto a temporary buffer to do the compare. If the copy of the memory +faults (the pointer points to memory that should not be accessed), then the +string compare will be treated as not matching. + 5.3 Clearing filters --------------------
kernel/trace/trace_events_filter.c +63 -3
99 lines not on this page · raw message
Signature · 1 line
2.33.0