Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jann Horn <jannh@google.com>
To: Kees Cook <keescook@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@kernel.org>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	Qitao Xu <qitao.xu@bytedance.com>,
	"David S. Miller" <davem@davemloft.net>,
	Network Development <netdev@vger.kernel.org>,
	Cong Wang <cong.wang@bytedance.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-hardening@vger.kernel.org
Subject: tracepoints and %p [was: Re: [Patch net-next resend v2] net: use %px to print skb address in trace_netif_receive_skb]
Date: Wed, 28 Jul 2021 17:13:12 +0200	[thread overview]
Message-ID: <CAG48ez0b-t_kJXVeFixYMoqRa-g1VRPUhFVknttiBYnf-cjTyg@mail.gmail.com> (raw)
In-Reply-To: <202107230000.B52B102@keescook>

+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.

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.




> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#p-format-specifier
>
> >
> > Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > ---
> >  include/trace/events/net.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> > index 2399073c3afc..78c448c6ab4c 100644
> > --- a/include/trace/events/net.h
> > +++ b/include/trace/events/net.h
> > @@ -136,7 +136,7 @@ DECLARE_EVENT_CLASS(net_dev_template,
> >               __assign_str(name, skb->dev->name);
> >       ),
> >
> > -     TP_printk("dev=%s skbaddr=%p len=%u",
> > +     TP_printk("dev=%s skbaddr=%px len=%u",
> >               __get_str(name), __entry->skbaddr, __entry->len)
> >  )
> >
> > --
> > 2.27.0
> >
>
> --
> Kees Cook

  parent reply	other threads:[~2021-07-28 15:13 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-15  5:59 [Patch net-next resend v2] net: use %px to print skb address in trace_netif_receive_skb Cong Wang
2021-07-15 17:40 ` patchwork-bot+netdevbpf
2021-07-23  7:09 ` Kees Cook
2021-07-28  3:17   ` Cong Wang
2021-07-28 15:13   ` Jann Horn [this message]
2021-07-28 15:56     ` tracepoints and %p [was: Re: [Patch net-next resend v2] net: use %px to print skb address in trace_netif_receive_skb] Steven Rostedt
2021-07-28 18:48       ` Kees Cook
2021-07-28 18:56         ` Jann Horn
2021-07-28 20:41         ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAG48ez0b-t_kJXVeFixYMoqRa-g1VRPUhFVknttiBYnf-cjTyg@mail.gmail.com \
    --to=jannh@google.com \
    --cc=cong.wang@bytedance.com \
    --cc=davem@davemloft.net \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=qitao.xu@bytedance.com \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    --cc=xiyou.wangcong@gmail.com \
    --subject='tracepoints and %p [was: Re: [Patch net-next resend v2] net: use %px to print skb address in trace_netif_receive_skb]' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).