Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members
From: Alexei Starovoitov
Date: Sat Nov 04 2017 - 05:35:15 EST
On 11/3/17 3:58 PM, Sandipan Das wrote:
For added security, the layout of some structures can be
randomized by enabling CONFIG_GCC_PLUGIN_RANDSTRUCT. One
such structure is task_struct. To build BPF programs, we
use Clang which does not support this feature. So, if we
attempt to read a field of a structure with a randomized
layout within a BPF program, we do not get the expected
value because of incorrect offsets. To observe this, it
is not mandatory to have CONFIG_GCC_PLUGIN_RANDSTRUCT
enabled because the structure annotations/members added
for this purpose are enough to cause this. So, all kernel
builds are affected.
For example, considering samples/bpf/offwaketime_kern.c,
if we try to print the values of pid and comm inside the
task_struct passed to waker() by adding the following
lines of code at the appropriate place
char fmt[] = "waker(): p->pid = %u, p->comm = %s\n";
bpf_trace_printk(fmt, sizeof(fmt), _(p->pid), _(p->comm));
it is seen that upon rebuilding and running this sample
followed by inspecting /sys/kernel/debug/tracing/trace,
the output looks like the following
_-----=> irqs-off
/ _----=> need-resched
| / _---=> hardirq/softirq
|| / _--=> preempt-depth
||| / delay
TASK-PID CPU# |||| TIMESTAMP FUNCTION
| | | |||| | |
<idle>-0 [007] d.s. 1883.443594: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [018] d.s. 1883.453588: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [007] d.s. 1883.463584: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [009] d.s. 1883.483586: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [005] d.s. 1883.493583: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [009] d.s. 1883.503583: 0x00000001: waker(): p->pid = 0, p->comm =
<idle>-0 [018] d.s. 1883.513578: 0x00000001: waker(): p->pid = 0, p->comm =
systemd-journal-3140 [003] d... 1883.627660: 0x00000001: waker(): p->pid = 0, p->comm =
systemd-journal-3140 [003] d... 1883.627704: 0x00000001: waker(): p->pid = 0, p->comm =
systemd-journal-3140 [003] d... 1883.627723: 0x00000001: waker(): p->pid = 0, p->comm =
To avoid this, we add new BPF helpers that read the
correct values for some of the important task_struct
members such as pid, tgid, comm and flags which are
extensively used in BPF-based analysis tools such as
bcc. Since these helpers are built with GCC, they use
the correct offsets when referencing a member.
Signed-off-by: Sandipan Das <sandipan@xxxxxxxxxxxxxxxxxx>
...
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f90860d1f897..324508d27bd2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -338,6 +338,16 @@ union bpf_attr {
* @skb: pointer to skb
* Return: classid if != 0
*
+ * u64 bpf_get_task_pid_tgid(struct task_struct *task)
+ * Return: task->tgid << 32 | task->pid
+ *
+ * int bpf_get_task_comm(struct task_struct *task)
+ * Stores task->comm into buf
+ * Return: 0 on success or negative error
+ *
+ * u32 bpf_get_task_flags(struct task_struct *task)
+ * Return: task->flags
+ *
I don't think it's a solution.
Tracing scripts read other fields too.
Making it work for these 3 fields is a drop in a bucket.
If randomization is used I think we have to accept
that existing bpf scripts won't be usable.
Long term solution is to support 'BPF Type Format' or BTF
(which is old C-Type Format) for kernel data structures,
so bcc scripts wouldn't need to use kernel headers and clang.
The proper offsets will be described in BTF.
We were planning to use it initially to describe map key/value,
but it applies for this case as well.
There will be a tool that will take dwarf from vmlinux and
compress it into BTF. Kernel will also be able to verify
that BTF is a valid BTF.
I'm assuming that gcc randomization plugin produces dwarf
with correct offsets, if not, it would have to be fixed.