Hi Alexei,
Sorry to bother you again!
On 6/11/21 5:42 AM, Alexei Starovoitov wrote:
On Wed, Jun 9, 2021 at 5:05 AM Ravi Bangoria
<ravi.bangoria@xxxxxxxxxxxxx> wrote:
Hi Alexei,
On 7/28/20 8:51 PM, Jean-Philippe Brucker wrote:
The following patch adds support for BPF_PROBE_MEM on arm64. The
implementation is simple but I wanted to give a bit of background first.
If you're familiar with recent BPF development you can skip to the patch
(or fact-check the following blurb).
BPF programs used for tracing can inspect any of the traced function's
arguments and follow pointers in struct members. Traditionally the BPF
program would get a struct pt_regs as argument and cast the register
values to the appropriate struct pointer. The BPF verifier would mandate
that any memory access uses the bpf_probe_read() helper, to suppress
page faults (see samples/bpf/tracex1_kern.c).
With BPF Type Format embedded into the kernel (CONFIG_DEBUG_INFO_BTF),
the verifier can now check the type of any access performed by a BPF
program. It rejects for example programs that cast to a different
structure and perform out-of-bounds accesses, or programs that attempt
to dereference something that isn't a pointer, or that hasn't gone
through a NULL check.
As this makes tracing programs safer, the verifier now allows loading
programs that access struct members without bpf_probe_read(). It is
however still possible to trigger page faults. For example in the
following example with which I've tested this patch, the verifier does
not mandate a NULL check for the second-level pointer:
/*
* From tools/testing/selftests/bpf/progs/bpf_iter_task.c
* dump_task() is called for each task.
*/
SEC("iter/task")
int dump_task(struct bpf_iter__task *ctx)
{
struct seq_file *seq = ctx->meta->seq;
struct task_struct *task = ctx->task;
/* Program would be rejected without this check */
if (task == NULL)
return 0;
/*
* However the verifier does not currently mandate
* checking task->mm, and the following faults for kernel
* threads.
*/
BPF_SEQ_PRINTF(seq, "pid=%d vm=%d", task->pid, task->mm->total_vm);
return 0;
}
Even if it checked this case, the verifier couldn't guarantee that all
accesses are safe since kernel structures could in theory contain
garbage or error pointers. So to allow fast access without
bpf_probe_read(), a JIT implementation must support BPF exception
tables. For each access to a BTF pointer, the JIT generates an entry
into an exception table appended to the BPF program. If the access
faults at runtime, the handler skips the faulting instruction. The
example above will display vm=0 for kernel threads.
I'm trying with the example above (task->mm->total_vm) on x86 machine
with bpf/master (11fc79fc9f2e3) plus commit 4c5de127598e1 ("bpf: Emit
explicit NULL pointer checks for PROBE_LDX instructions.") *reverted*,
I'm seeing the app getting killed with error in dmesg.
$ sudo bpftool iter pin bpf_iter_task.o /sys/fs/bpf/task
$ sudo cat /sys/fs/bpf/task
Killed
$ dmesg
[ 188.810020] BUG: kernel NULL pointer dereference, address: 00000000000000c8
[ 188.810030] #PF: supervisor read access in kernel mode
[ 188.810034] #PF: error_code(0x0000) - not-present page
IIUC, this should be handled by bpf exception table rather than killing
the app. Am I missing anything?
For PROBE_LDX the verifier guarantees that the address is either
a very likely valid kernel address or NULL. On x86 the user and kernel
address spaces are shared and NULL is a user address, so there cannot be
an exception table for NULL. Hence x86-64 JIT inserts NULL check when
it converts PROBE_LDX into load insn.
IIUC, there could be 3 types of addresses a BPF prog can have:
1. NULL ptr. Which is handled by adding additional check in BPF program.
So this won't cause a fault.
2. Valid kernel address. This will never cause a fault.
3. Bad address. This is very unlikely but possible. IIUC, BPF extable
is introduced to handle such scenarios. Unfortunately, with any type
of bad address (user or kernel), extable on x86 never gets involved.
Kernel always kills the application with error in dmesg.
Please let me know if I understood it incorrectly.
TLDR;
To check the case 3 further, I tried with bpf/master, this time without
reverting your patch. I added a dummy structure and a bad pointer to it
in task_struct.
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d2c881384517..4698188bcf45 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -646,6 +646,10 @@ struct kmap_ctrl {
#endif
};
+struct dummy_task_ele {
+ int dummy;
+};
+
struct task_struct {
#ifdef CONFIG_THREAD_INFO_IN_TASK
/*
@@ -771,6 +775,8 @@ struct task_struct {
struct mm_struct *mm;
struct mm_struct *active_mm;
+ struct dummy_task_ele *dte;
+
/* Per-thread vma caching: */
struct vmacache vmacache;
diff --git a/kernel/fork.c b/kernel/fork.c
index dc06afd725cb..ed01f25edd8e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2116,6 +2116,9 @@ static __latent_entropy struct task_struct *copy_process(
retval = copy_mm(clone_flags, p);
if (retval)
goto bad_fork_cleanup_signal;
+
+ p->dte = (void *)0xd12345;
+
retval = copy_namespaces(clone_flags, p);
if (retval)
goto bad_fork_cleanup_mm;
And with below change in testcase:
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task.c b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
index b7f32c160f4e..391c1b3da638 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
@@ -21,6 +21,6 @@ int dump_task(struct bpf_iter__task *ctx)
if (ctx->meta->seq_num == 0)
BPF_SEQ_PRINTF(seq, " tgid gid\n");
- BPF_SEQ_PRINTF(seq, "%8d %8d\n", task->tgid, task->pid);
+ BPF_SEQ_PRINTF(seq, "%8d %8d %d\n", task->tgid, task->pid, task->dte->dummy);
return 0;
}
I see the same issue:
$ sudo bpftool iter pin bpf_iter_task.o /sys/fs/bpf/task
$ sudo cat /sys/fs/bpf/task
Killed
$ dmesg
[ 166.864325] BUG: unable to handle page fault for address: 0000000000d12345
[ 166.864336] #PF: supervisor read access in kernel mode
[ 166.864338] #PF: error_code(0x0000) - not-present page
0xd12345 is unallocated userspace address. Similarly, I also tried with
p->dte = (void *)0xffffffffc1234567 after confirming it's not allocated
to kernel or any module address. I see the same failure with it too.
Though, the same test with bpf_probe_read(task->dte->dummy) works fine.
Ravi
