On Tue, Apr 23, 2024 at 3:16 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> On 4/18/24 8:03 PM, Alexei Starovoitov wrote:
> > 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.
>
> So IIUC, for a trusted pointer, any member or nested member
> access (without pointer tracing) is trusted.
> For example
> struct p2 {
> struct t1 *s1;
> struct t2 s2;
> };
> struct p {
> struct p1 f1;
> struct p2 f2;
> struct p3 f3;
> }
> struct p *trust_p;
>
> So here &trust_p->f1, &trust_p->f2, &trust_p->f3, &trust_p->f2.s2
> are all trusted, right?
Correct, but the verifier currently only allows to pass &trust_p->f1
as an argument.
