On Thu, Oct 20, 2022 at 11:27:49AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Sure, I can make that change. We'll have to increase the size of the
> > prefix string on the stack, but that's hardly problematic as these
> > strings are not terribly large.
> >
> > > > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> > > > if (type_flag(reg->type) & PTR_UNTRUSTED)
> > > > flag |= PTR_UNTRUSTED;
> > > >
> > > > + /* All pointers obtained by walking a struct are nested. */
> > > > + flag |= PTR_NESTED;
> > > > +
> > >
> > > Instead of PTR_NESTED, how about PTR_WALK?
> >
> > I don't have a strong preference between either, though I would prefer
> > PTR_WALKED if we go with the latter. Does that work for you?
> >
>
> Yes, I just think PTR_NESTED is a bit misleading, it's not nested within the old
> object, we loaded a pointer from it, it should just indicate that the pointer
> came from a walk of a trusted PTR_TO_BTF_ID.
Ok, we'll go with PTR_WALKED.
> > > > [...]
> > > > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> > > > static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> > > > static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> > > > static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > > > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > > > +static const struct bpf_reg_types btf_ptr_types = {
> > > > + .types = {
> > > > + PTR_TO_BTF_ID,
> > > > + PTR_TO_BTF_ID | PTR_NESTED
> > > > + },
> > > > +};
> > >
> > > CI fails, two of those failures are from not updating
> > > check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one
> >
> > Gah, I didn't think it was necessary for this case as it's not required
> > for btf_check_func_arg_match(), which will eventually just fail in the
> > following check:
> >
> > if (!btf_type_is_struct(ref_t)) {
> > bpf_log(log, "kernel function %s args#%d pointer type %s %s is not support
> > func_name, i, btf_type_str(ref_t),
> > ref_tname);
> > return -EINVAL;
> > }
>
> Why would it fail there? It will still be a struct type.
> I think you misunderstand this a bit.
Apologies, as mentioned below I pasted the wrong if-check on accident.
If I had actually meant to paste that one, then saying I "misunderstand
this a bit" would have been a very generous understatment :-)
> When you have task from tracing ctx arg:
> r1 = ctx;
> r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> // r1 = task->next
> r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
>
> We loaded a pointer from task_struct into r1.
> Now r1 still points to a task_struct, so that check above won't fail for r1.
I meant to paste the if-condition _above_ that one. This is the if-check
we'll fail due to the presence of a type modifier (PTR_WALKED):
} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
(reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
const struct btf_type *reg_ref_t;
const struct btf *reg_btf;
const char *reg_ref_tname;
u32 reg_ref_id;
So we'll never even get to the if check I originally pasted because
reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
below we'll eventually fail later on here:
/* Permit pointer to mem, but only when argument
* type is pointer to scalar, or struct composed
* (recursively) of scalars.
* When arg_mem_size is true, the pointer can be
* void *.
* Also permit initialized local dynamic pointers.
*/
if (!btf_type_is_scalar(ref_t) &&
!__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
!arg_dynptr &&
(arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
bpf_log(log,
"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
return -EINVAL;
}
Appreciate the explanation, sorry to have made you type it.
> > Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> > difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> > allow it to work properly for normal helper calls, so I'll make that change.
> > Thanks for pointing it out. In general, the whole dance between register base
> > types + modifiers sometimes feels like a mine field...
> >
>
> Yes, I don't like how it's growing and being mixed either. Eventually I think we
> should document what combinations are allowed and reject everything else when
> initializing reg->type to prevent bugs, but IDK whether something like this
> would be accepted.
That seems like a pretty sane idea. A project for another day...
