Re: [PATCH v2] tracing: Add test for user space strings when filtering on string pointers

[Steven Rostedt]

On Thu, 13 Jan 2022 22:11:35 +0000
David Laight <David.Laight@xxxxxxxxxx> wrote:


> > 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?  
> 
> Also, when the __user pointer is saved it MUST be checked for being
> a valid user pointer (eg with access_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.


> 
> 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?


> 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=18446744071906760329

And 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