Re: [RFC] bpf: allowing PTR_TO_BTF_ID | PTR_TRUSTED w/ non-zero fixed offset to selected KF_TRUSTED_ARGS BPF kfuncs

[Matt Bobrowski <mattbobrowski@google.com>]

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.

/M