Re: tracepoints and %p [was: Re: [Patch net-next resend v2] net: use %px to print skb address in trace_netif_receive_skb]

[Kees Cook <keescook@chromium.org>]

On Wed, Jul 28, 2021 at 11:56:33AM -0400, Steven Rostedt wrote:
> On Wed, 28 Jul 2021 17:13:12 +0200
> Jann Horn <jannh@google.com> wrote:
> 
> > +tracing maintainers
> > 
> > On Fri, Jul 23, 2021 at 9:09 AM Kees Cook <keescook@chromium.org> wrote:
> > > On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:  
> > > > From: Qitao Xu <qitao.xu@bytedance.com>
> > > >
> > > > The print format of skb adress in tracepoint class net_dev_template
> > > > is changed to %px from %p, because we want to use skb address
> > > > as a quick way to identify a packet.  
> > >
> > > No; %p was already hashed to uniquely identify unique addresses. This
> > > is needlessly exposing kernel addresses with no change in utility. See
> > > [1] for full details on when %px is justified (almost never).
> > >  
> > > > Note, trace ring buffer is only accessible to privileged users,
> > > > it is safe to use a real kernel address here.  
> > >
> > > That's not accurate either; there is a difference between uid 0 and
> > > kernel mode privilege levels.
> > >
> > > Please revert these:
> > >
> > >         851f36e40962408309ad2665bf0056c19a97881c
> > >         65875073eddd24d7b3968c1501ef29277398dc7b
> > >
> > > And adjust this to replace %px with %p:
> > >
> > >         70713dddf3d25a02d1952f8c5d2688c986d2f2fb
> > >
> > > Thanks!
> > >
> > > -Kees  
> > 
> > Hi Kees,
> > 
> > as far as I understand, the printf format strings for tracepoints
> > don't matter for exposing what data is exposed to userspace - the raw
> > data, not the formatted data, is stored in the ring buffer that
> > userspace can access via e.g. trace_pipe_raw (see
> > https://www.kernel.org/doc/Documentation/trace/ftrace.txt), and the
> > data can then be formatted **by userspace tooling** (e.g.
> > libtraceevent). As far as I understand, the stuff that root can read
> > via debugfs is the data stored by TP_fast_assign() (although root
> > _can_ also let the kernel do the printing and read it in text form).
> > Maybe Steven Rostedt can help with whether that's true and provide
> > more detail on this.
> 
> That is exactly what is happening. I wrote the following to the replied
> text up at the top, then noticed you basically stated the same thing
> here ;-)

Where is the %px being formatted then? If it's the kernel itself (which
is the only thing that does %px), then it doesn't need to be %px, since
the raw data is separate. i.e. leave it %p for whatever logs will get
spilled out to who knows where.

> "You can get the raw data from the trace buffers directly via the
> trace_pipe_raw. The data is copied directly without any processing. The
> TP_fast_assign() adds the data into the buffer, and the printf() is
> only reading what's in that buffer. The hashing happens later. If you
> read the buffers directly, you get all the data you want."
> 
> > 
> > In my view, the ftrace subsystem, just like the BPF subsystem, is
> > root-only debug tracing infrastructure that can and should log
> > detailed information about kernel internals, no matter whether that
> > information might be helpful to attackers, because if an attacker is
> > sufficiently privileged to access this level of debug information,
> > that's beyond the point where it makes sense to worry about exposing
> > kernel pointers. But even if you disagree, I don't think that ftrace
> > format strings are relevant here.
> 
> Anyway, those patches are not needed. (Kees is going to hate me).
> 
> Since a345a6718bd56 added in 5.12, you can just do:
> 
>  # trace-cmd start -e net_dev_start_xmit
>  # trace-cmd show
> [..]
>             sshd-1853    [007] ...1  1995.000611: net_dev_start_xmit: dev=em1 queue_mapping=0 skbaddr=00000000f8c47ebd vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=3 len=150 data_len=84 network_offset=14 transport_offset_valid=1 transport_offset=34 tx_flags=0 gso_size=0 gso_segs=1 gso_type=0x1
> 
> Notice the value of skbaddr=00000000f8c47ebd ?
> 
> Now I do:
> 
> 	# trace-cmd start -O nohash-ptr -e net_dev_start_xmit
> 	# trace-cmd show
> [..]
>             sshd-1853    [007] ...1  2089.462722: net_dev_start_xmit: dev= queue_mapping=0 skbaddr=ffff8cfbc3ffd0e0 vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800 ip_summed=3 len=150 data_len=84 network_offset=14 transport_offset_valid=1 transport_offset=34 tx_flags=0 gso_size=0 gso_segs=1 gso_type=0x1
> 
> And now we have:
> 
> skbaddr=ffff8cfbc3ffd0e0

How does ftrace interact with lockdown's confidentiality mode?

-- 
Kees Cook