On Tue, Apr 23, 2024 at 10:50 PM David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 18, 2024 at 08:03:40PM -0700, 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);
>
> Yes, I believe this should already work. It would be the same as casting
> the task as a struct thread_info. btf_struct_ids_match() should
> btf_struct_walk() the task, and find a struct thread_info object at that
> offset and successfully compare the BTF IDs of that with the arg type.
> If not for the check_func_arg_reg_off() code described below, it should
> also work with nonzero offsets as well. When we begin the walk it can be
> at any offset. After we do the first struct walk, we continue descending
> at offset 0 from that first inner struct type.
>
> > 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.
>
> Agreed
>
> > 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.
>
> That check is currently an invariant because of this code:
>
> 11720 case KF_ARG_PTR_TO_MAP:
> 11721 case KF_ARG_PTR_TO_ALLOC_BTF_ID:
> 11722 case KF_ARG_PTR_TO_BTF_ID:
> 11723 if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
> 11724 break;
> 11725
> 11726 if (!is_trusted_reg(reg)) {
> 11727 if (!is_kfunc_rcu(meta)) {
> 11728 verbose(env, "R%d must be referenced or trusted\n", regno);
> 11729 return -EINVAL;
> 11730 }
> 11731 if (!is_rcu_reg(reg)) {
> 11732 verbose(env, "R%d must be a rcu pointer\n", regno);
> 11733 return -EINVAL;
> 11734 }
> 11735 }
> 11736
> 11737 fallthrough;
> 11738 case KF_ARG_PTR_TO_CTX:
> 11739 /* Trusted arguments have the same offset checks as release arguments */
> 11740 arg_type |= OBJ_RELEASE; <<<<< because of this
> 11741 break;
>
> The OBJ_RELEASE causes check_func_arg_reg_off() to fail to verify if
> there's a nonzero offset. In reality, I _think_ we only need to check
> for a nonzero offset for KF_RELEASE, and possibly KF_ACQUIRE.
Why special case KF_RELEASE/ACQUIRE ?
I think they're no different from kfuncs with KF_TRUSTED_ARGS.
Should be safe to allow non-zero offset trusted arg in all cases.
> >
> > 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.
>
> Yes, this should continue to just work, but I think we may also have to
> be cognizant to not allow this type of pattern:
>
> struct some_other_type {
> int field_at_off_zero;
> struct nf_conn___init ct;
> };
>
> In this case, we don't want to allow &other_type->ct to be passed to a
> kfunc expecting a struct nf_conn. So we'd also have to compare the type
> at the register offset to make sure it's not a nocast alias, not just
> the type in the register itself. I'm not sure if this is a problem in
> practice. I expect it isn't. struct nf_conn___init exists solely to
> allow the struct nf_conn kfuncs to enforce calling semantics so that an
> uninitialized struct nf_conn object can't be passed to specific kfuncs
> that are expecting an initialized object. I don't see why we'd ever
> embed a wrapper type like that inside of another type. But still
> something to be cognizant of.
Agree that it's not a problem now and I wouldn't proactively
complicate the verifier.
__init types are in the kernel code and it gets code reviewed.
So 'struct some_other_type' won't happen out of nowhere.
