From: Cong Wang <hidden> Date: 2021-07-15 06:00:19
From: Qitao Xu <redacted>
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.
Note, trace ring buffer is only accessible to privileged users,
it is safe to use a real kernel address here.
Reviewed-by: Cong Wang <redacted>
Signed-off-by: Qitao Xu <redacted>
---
include/trace/events/net.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Wed, 14 Jul 2021 22:59:23 -0700 you wrote:
From: Qitao Xu <redacted>
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.
Note, trace ring buffer is only accessible to privileged users,
it is safe to use a real kernel address here.
[...]
On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:
From: Qitao Xu <redacted>
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
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#p-format-specifier
From: Cong Wang <hidden> Date: 2021-07-28 03:17:59
On Fri, Jul 23, 2021 at 12:09 AM Kees Cook [off-list ref] wrote:
On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:
quoted
From: Qitao Xu <redacted>
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).
+tracing maintainers
On Fri, Jul 23, 2021 at 9:09 AM Kees Cook [off-list ref] wrote:
On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:
quoted
From: Qitao Xu <redacted>
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).
quoted
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.
+tracing maintainers
On Fri, Jul 23, 2021 at 9:09 AM Kees Cook [off-list ref] wrote:
quoted
On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:
quoted
From: Qitao Xu <redacted>
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).
quoted
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 ;-)
"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
-- Steve
+tracing maintainers
On Fri, Jul 23, 2021 at 9:09 AM Kees Cook [off-list ref] wrote:
quoted
On Wed, Jul 14, 2021 at 10:59:23PM -0700, Cong Wang wrote:
quoted
From: Qitao Xu <redacted>
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).
quoted
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."
quoted
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
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.
The trace events shown by tracefs/trace are formatted via the kernel
sprintf() and friends, which will hash "%p".
But the raw data read to trace-cmd (and other tools), uses
libtraceevent, that parses the printf-fmt of a trace event just like
the kernel does. But since we are already in user space, it is pretty
pointless to implement "%p" with hashing. But it does understand what
"%px" is.
How does ftrace interact with lockdown's confidentiality mode?
as Jann replied. If you enable "LOCKDOWN_TRACEFS" you lose all access
to the tracing interface.
-- Steve