Messages in this thread Patch in this message | | | From | Sven Schnelle <> | Subject | Re: [PATCH v2] tracing: Add test for user space strings when filtering on string pointers | Date | Thu, 13 Jan 2022 22:28:01 +0100 |
| |
Hi Steve,
Steven Rostedt <rostedt@goodmis.org> writes:
> On Tue, 11 Jan 2022 21:55:53 +0100 > Sven Schnelle <svens@linux.ibm.com> wrote: > >> > 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. > > 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?
> -- Steve > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 91352a64be09..06013822764c 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -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;
| |