On Wed, Jun 14, 2023 at 08:06:18AM +0000, Matt Bobrowski wrote:
> Hey David,
>
> I have a quick question in regards to the type tag PTR_TRUSTED, which
> I believe is causing one of my BPF programs to no longer successfully
> pass the BPF verifier. Given the following BPF program as an example:
Hi Matt,
Thanks for the message. I'll add the bpf list to cc so the community can
see and discuss.
> ---
> SEC("lsm.s/bprm_committed_creds")
> int BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm)
> {
> int ret;
> char buf[64] = {0};
>
> ret = bpf_d_path(&bprm->file->f_path, buf, sizeof(buf));
> if (ret < 0) {
> bpf_printk("bpf_d_path: ret=%d", ret);
> return 0;
> }
>
> return 0;
> }
> ---
>
> In this case, the PTR_TO_BTF_ID pointer (&bprm->file->f_path) I
> imagine is considered to be trusted and can be passed to the BPF
> helper bpf_d_path without the BPF verifier complaining.
>
> On the other hand, given the following relatively similar BPF program
> as an example:
>
> ---
> SEC("lsm.s/bprm_committed_creds")
> int BPF_PROG(bprm_committed_creds)
> {
> int ret;
> char buf[64] = {0};
> struct task_struct *current = bpf_get_current_task_btf();
>
> ret = bpf_d_path(¤t->fs->pwd, buf, sizeof(buf));
> if (ret < 0) {
> bpf_printk("bpf_d_path: ret=%d", ret);
> return 0;
> }
>
> return 0;
> }
> ---
>
> In this case, the PTR_TO_BTF_ID pointer (¤t->fs->pwd) is
> considered to be untrusted and cannot be passed to the BPF helper
> bpf_d_path as the BPF verifier fails to load the BPF program with the
> following message:
The reason this is happening is that the struct fs_struct *fs field of
struct task_struct is not marked as inheriting its parent task's trusted
status. The following diff would fix the issue:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fa43dc8e85b9..8b8ccde342f9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
struct css_set __rcu *cgroups;
struct task_struct __rcu *real_parent;
struct task_struct *group_leader;
+ struct fs_struct *fs;
};
BTF_TYPE_SAFE_RCU(struct cgroup) {
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
index dcdea3127086..aef2d4689826 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
@@ -324,3 +324,24 @@ int BPF_PROG(task_kfunc_release_in_map, struct task_struct *task, u64 clone_flag
return 0;
}
+
+SEC("lsm.s/bprm_committed_creds")
+__success
+int BPF_PROG(bprm_committed_creds)
+{
+ int ret;
+ char buf[64] = {0};
+ struct task_struct *current = bpf_get_current_task_btf();
+ struct fs_struct *fs;
+
+ bpf_rcu_read_lock();
+ fs = current->fs;
+ ret = bpf_d_path(&fs->pwd, buf, sizeof(buf));
+ bpf_rcu_read_unlock();
+ if (ret < 0) {
+ bpf_printk("bpf_d_path: ret=%d", ret);
+ return 0;
+ }
+
+ return 0;
+}
With this patch, the above test would pass (meaning the program is successfully
verified).
>
> ---
> ; ret = bpf_d_path(¤t->fs->pwd, buf, sizeof(buf));
> 15: (b7) r3 = 64 ; R3_w=64
> 16: (85) call bpf_d_path#147
> R1 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_
> ---
>
> What's interesting is that based on the following commit
> (3f00c52393445 bpf: Allow trusted pointers to be passed to
> KF_TRUSTED_ARGS kfuncs) message:
>
> ---
> PTR_TRUSTED pointers are passed directly from the kernel as a
> tracepoint or struct_ops callback argument. Any nested pointer that is
> obtained from walking a PTR_TRUSTED pointer is no longer
> PTR_TRUSTED. From the example above, the struct task_struct *task
> argument is PTR_TRUSTED, but the 'nested' pointer obtained from
> 'task->last_wakee' is not PTR_TRUSTED.
> ---
>
> I'm reading this as though the first example program should also fail
> BPF verification given that a nested pointer to struct path is
> obtained from walking a PTR_TRUSTED pointer, which presumably is
> struct linux_binprm in this case. What subtle details am I missing
> here? Why is that the first program loads, but the second does not?
The subtle details are in that these are different types. We can't assume that
a child pointer automatically inherits its parent's trusted status for all
types, so we have to hard code it for now until gcc supports using btf type
tags so this can be expressed with annotations like __trusted or __rcu.
Consider that some types may have NULL child pointers in certain scenarios.
Others may be valid as long as you access it in an RCU read region, others may
be valid as long as you access it in an RCU read region and it wasn't NULL.
The struct linux_binprm type's safety is specified in
kernel/bpf/verifier.c:
BTF_TYPE_SAFE_TRUSTED(struct linux_binprm) {
struct file *file;
};
struct task_struct is specified above:
/* RCU trusted: these fields are trusted in RCU CS and never NULL */
BTF_TYPE_SAFE_RCU(struct task_struct) {
const cpumask_t *cpus_ptr;
struct css_set __rcu *cgroups;
struct task_struct __rcu *real_parent;
struct task_struct *group_leader;
};
> What workaround needs to be implemented in order to have the second
> example program satisfy the PTR_TRUSTED contract with the BPF helper
> bpf_d_path?
See above, we need to add struct fs_struct * to the list of trusted fields for
struct task_struct, and enclose it in an RCU read region.
Thanks,
David
