On Fri, 2025-07-04 at 20:50 +0200, Kumar Kartikeya Dwivedi wrote:
> On Fri, 4 Jul 2025 at 20:33, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Fri, 2025-07-04 at 11:28 -0700, Eduard Zingerman wrote:
> > > On Fri, 2025-07-04 at 20:03 +0200, Kumar Kartikeya Dwivedi wrote:
> > >
> > > [...]
> > >
> > > > > @@ -7818,6 +7821,22 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> > > > > sub->args[i].btf_id = kern_type_id;
> > > > > continue;
> > > > > }
> > > > > + if (tags & ARG_TAG_UNTRUSTED) {
> > > > > + int kern_type_id;
> > > > > +
> > > > > + if (tags & ~ARG_TAG_UNTRUSTED) {
> > > > > + bpf_log(log, "arg#%d untrusted cannot be combined with any other tags\n", i);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + kern_type_id = btf_get_ptr_to_btf_id(log, i, btf, t);
> > > >
> > > > So while this makes sense for trusted, I think for untrusted, we
> > > > should allow types in program BTF as well.
> > > > This is one of the things I think lacks in bpf_rdonly_cast as well, to
> > > > be able to cast to types in program BTF.
> > > > Say you want to reinterpret some kernel memory into your own type and
> > > > access it using a struct in the program which is a different type.
> > > > I think it makes sense to make this work.
> > >
> > > Hi Kumar,
> > >
> > > Thank you for the review.
> > > Allowing local program BTF makes sense to me.
> > > I assume we should first search in kernel BTF and fallback to program
> > > BTF if nothing found. This way verifier might catch a program
> > > accessing kernel data structure but having wrong assumptions about
> > > field offsets (not using CO-RE). On the other hand, this might get
> > > confusing if there is an accidental conflict between kernel data
> > > structure name and program local data structure name.
> >
> > Maybe just add __arg_untrusted_local and avoid ambiguity?
>
> That might be less ambiguous, sure. But I don't see why the fallback
> would be confusing.
> It might be nice if we can support it without asking users to learn
> about the difference between the two tags, but if it's too ugly we can
> go with explicit local tag.
> A user can have a struct without preserve_access_index now and having
> the same name as the kernel struct, and the program will load things
> at potentially wrong offsets.
> If the same type exists, the program would fail compilation in C due
> to duplicate types.
> Are there any other cases where it might be a footgun that you anticipate?
Well, basically two cases assuming that program does not use vmlinux.h:
struct kernel_type { // assume kernel type has 'i' at another offset
int *i;
};
__weak int global(struct kernel_type *p __arg_untrusted) {
return p->i[7]; // assume no CO-RE relocation for &p->i
}
In this case, if kernel BTF is searched first verifier can catch that
access to 'i' is bogus. However, that is not necessary if one assumes
that verifier should only check for errors that can bring down the
kernel.
Another case:
// assume there is kernel type with the same name, but program
// author does not know about that and does not use vmlinux.h,
// thus avoiding compilation error.
struct accidental_kernel_type {
int *i;
};
__weak int global(struct accidental_kernel_type *p __arg_untrusted) {
return p->i[7];
}
In this case user would not expect any errors at load time.
Hm.
Maybe just always use program BTF?
> > > Supporting bpf_core_cast for both prog BTF and kernel BTF types is not
> > > trivial because we cannot disambiguate local vs kernel types.
> > > IIRC module BTF types probably don't work either but that's a different story.
>
> > I can add bpf_rdonly_cast_local() as a followup, do you remember
> > context in which you needed this?
>
> Adding Matt.
>
> Not long ago we were discussing iterating over the bpf linked list
> since support doesn't exist in the kernel and it was safe in the
> specific context to iterate over the list.
> Ofcourse, we could add iteration support in the kernel, but another
> approach would be the ability to subtract offset from node to arrive
> at an untrusted pointer to type in prog BTF (that was allocated using
> bpf_obj_new).
> But bpf_core_cast didn't work there, so we ended up discarding that approach.
> Not to get hung up on this specific example, but I think it would be
> useful in general.
I see, makes sense.
That should be possible now using bpf_rdonly_cast(..., 0) but one
would need to explicitly cast each pointer in this way.
