On Wed, Apr 17, 2024 at 5:11 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > > > On 4/17/24 1:19 PM, Matt Bobrowski wrote: > > On Mon, Apr 15, 2024 at 09:43:42AM -0700, Yonghong Song wrote: > >> On 4/12/24 4:31 AM, Matt Bobrowski wrote: > >>> Hi, > >>> > >>> Currently, if a BPF kfunc has been annotated with KF_TRUSTED_ARGS, any > >>> supplied PTR_TO_BTF_ID | PTR_TRUSTED argument to that BPF kfunc must > >>> have it's fixed offset set to zero, or else the BPF program being > >>> loaded will be outright rejected by the BPF verifier. > >>> > >>> This non-zero fixed offset restriction in most cases makes a lot of > >>> sense, as it's considered to be a robust means of assuring that the > >>> supplied PTR_TO_BTF_ID to the KF_TRUSTED_ARGS annotated BPF kfunc > >>> upholds it's PTR_TRUSTED property. However, I believe that there are > >>> also cases out there whereby a PTR_TO_BTF_ID | PTR_TRUSTED w/ a fixed > >>> offset can still be considered as something which posses the > >>> PTR_TRUSTED property, and could be safely passed to a BPF kfunc that > >>> is annotated w/ KF_TRUSTED_ARGS. I believe that this can particularly > >>> hold true for selected embedded data structure members present within > >>> given PTR_TO_BTF_ID | PTR_TRUSTED types i.e. struct > >>> task_struct.thread_info, struct file.nf_path. > >>> > >>> Take for example the struct thread_info which is embedded within > >>> struct task_struct. In a BPF program, if we happened to acquire a > >>> PTR_TO_BTF_ID | PTR_TRUSTED for a struct task_struct via > >>> bpf_get_current_task_btf(), and then constructed a pointer of type > >>> struct thread_info which was assigned the address of the embedded > >>> struct task_struct.thread_info member, we'd have ourselves a > >>> PTR_TO_BTF_ID | PTR_TRUSTED w/ a fixed offset. Now, let's > >>> hypothetically also say that we had a BPF kfunc that took a struct > >>> thread_info pointer as an argument and the BPF kfunc was also > >>> annotated w/ KF_TRUSTED_ARGS. If we attempted to pass the constructed > >>> PTR_TO_BTF_ID | PTR_TRUSTED w/ fixed offset to this hypothetical BPF > >>> kfunc, the BPF program would be rejected by the BPF verifier. This is > >>> irrespective of the fact that supplying pointers to such embedded data > >>> structure members of a PTR_TO_BTF_ID | PTR_TRUSTED may be considered > >>> to be safe. > >>> > >>> One of the ideas that I had in mind to workaround the non-zero fixed > >>> offset restriction was to simply introduce a new BPF kfunc annotation > >>> i.e. __offset_allowed that could be applied on selected BPF kfunc > >>> arguments that are expected to be KF_TRUSTED_ARGS. Such an annotation > >>> would effectively control whether we enforce the non-zero offset > >>> restriction or not in check_kfunc_args(), check_func_arg_reg_off(), > >>> and __check_ptr_off_reg(). Although, now I'm second guessing myself > >>> and I am wondering whether introducing something like the > >>> __offset_allowed annotation for BPF kfunc arguments could lead to > >>> compromising any of the safety guarantees that are provided by the BPF > >>> verifier. Does anyone see an immediate problem with using such an > >>> approach? I raise concerns, because it feels like we're effectively > >>> punching a hole in the BPF verifier, but it may also be perfectly safe > >>> to do on carefully selected PTR_TO_BTF_ID | PTR_TRUSTED types > >>> i.e. struct thread_info, struct file, and it's just my paranoia > >>> getting the better of me. Or, maybe someone has another idea to > >>> support PTR_TO_BTF_ID | PTR_TRUSTED w/ fixed offset safely and a > >>> little more generally without the need to actually make use of any > >>> other BPF kfunc annotations? > >> In verifier.c, we have BTF_TYPE_SAFE_TRUSTED to indidate that > >> a pointer of a particular struct is safe and trusted if the point > >> of that struct is trusted, e.g., > >> > >> BTF_TYPE_SAFE_TRUSTED(struct file) { > >> struct inode *f_inode; > >> }; > >> > >> We do the above since gcc does not support btf_tag yet. > > Yes, I'm rather familiar with this construct. > > > >> I guess you could do > >> > >> BTF_TYPE_SAFE_TRUSTED(struct file) { > >> struct path f_path; > >> }; > >> > >> and enhance verifier with the above information. > >> > >> But the above 'struct path f_path' may unnecessary > >> consume extra memory since we only care about field > >> 'f_path'. Maybe create a new construct like > >> > >> /* pointee is a field of the struct */ > >> BTF_TYPE_SAFE_FIELD_TRUSTED(struct file) { > >> struct path *f_path; > >> }; > > I don't fully understand how something like > > BTF_TYPE_SAFE_FIELD_TRUSTED could work in practice. Do you mind > > elaborating on that a little? > > > > What I'm currently thinking is that with something like > > BTF_TYPE_SAFE_FIELD_TRUSTED, if the BPF verifier sees a PTR_TO_BTF_ID > > | PTR_TRUSTED w/ a fixed offset supplied to a BPF kfunc, then the BPF > > verifier can also check that fixed offset for the supplied > > PTR_TO_BTF_ID | PTR_TRUSTED actually accesses a member that has been > > explicitly annotated as being trusted via > > BTF_TYPE_SAFE_FIELD_TRUSTED. Maybe that would be better then making > > use of an __offset_allowed annotation, which would solely rely on the > > btf_struct_ids_match() check for its safety. > Right. What you described in the above is what I think as well. I believe BTF_TYPE_SAFE_* or __offset_allowed annotations are not necessary. In this case thread_info is the first field of struct task_struct and I suspect the verifier already allows: bpf_kfunc void do_stuff_with_thread(struct thread_info *ti) KF_TRUSTED_ARGS and use it as: task = bpf_get_current_task_btf(); do_stuff_with_thread(&task->thread_info); We have similar setup with: struct bpf_cpumask { cpumask_t cpumask; ... }; and kfunc that accepts trusted cpumask_t * will accept trusted struct bpf_cpumask *. The other way around should be rejected, of course. Similar approach should work with file/path. The only difference is that the offset will be non-zero. process_kf_arg_ptr_to_btf_id() needs to get smarter. David Vernet added that check: WARN_ON_ONCE(is_kfunc_trusted_args(meta) && reg->off); as part of commit b613d335a743c. iirc the reg->off==0 check is there, as an extra caution. We can allow off!=0 and it won't confuse btf_type_ids_nocast_alias. struct nf_conn___init { int another_field_at_off_zero; struct nf_conn ct; }; will still trigger strict_type_match as expected. Maybe other places in the verifier need to get smarter too to allow non-zero offset into kf_trusted_args.