On Fri, Nov 18, 2022 at 10:45:37AM -0600, David Vernet wrote:
> On Fri, Nov 18, 2022 at 08:45:44AM -0600, David Vernet wrote:
>
> [...]
>
> > > > bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > > const struct bpf_prog *prog,
> > > > struct bpf_insn_access_aux *info)
> > > > @@ -5722,6 +5727,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > > }
> > > >
> > > > info->reg_type = PTR_TO_BTF_ID;
> > > > + if (prog_type_args_trusted(prog->type))
> > > > + info->reg_type |= PTR_TRUSTED;
> > > > +
> > > > if (tgt_prog) {
> > > > enum bpf_prog_type tgt_type;
> > > >
> > > > @@ -6558,15 +6566,26 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > > /* These register types have special constraints wrt ref_obj_id
> > > > * and offset checks. The rest of trusted args don't.
> > > > */
> > > > - obj_ptr = reg->type == PTR_TO_CTX || reg->type == PTR_TO_BTF_ID ||
> > > > + obj_ptr = reg->type == PTR_TO_CTX ||
> > > > + base_type(reg->type) == PTR_TO_BTF_ID ||
> > > > reg2btf_ids[base_type(reg->type)];
> > > >
> > > > /* Check if argument must be a referenced pointer, args + i has
> > > > * been verified to be a pointer (after skipping modifiers).
> > > > * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> > > > + *
> > > > + * All object pointers must be refcounted, other than:
> > > > + * - PTR_TO_CTX
> > > > + * - PTR_TRUSTED pointers
> > > > */
> > > > - if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> > > > - bpf_log(log, "R%d must be referenced\n", regno);
> > > > + if (is_kfunc &&
> > > > + trusted_args &&
> > > > + obj_ptr &&
> > > > + base_type(reg->type) != PTR_TO_CTX &&
> > > > + (!(type_flag(reg->type) & PTR_TRUSTED) ||
> > > > + (type_flag(reg->type) & ~PTR_TRUSTED)) &&
> > > > + !reg->ref_obj_id) {
> > >
> > > This is pretty hard to read.
> > > Is this checking:
> > > !(reg->type == PTR_TO_BTF_ID || reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED))
> > > ?
> > >
> > > Why not to use the above?
> >
> > Agreed this is more readable, I'll do this for v8 (from a helper as you
> > suggested).
>
> Sorry, my initial response was incorrect. After thinking about this
> more, I don't think this conditional would be correct here:
>
> !(reg->type == PTR_TO_BTF_ID || reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED))
>
> That conditional is saying, "If it's PTR_TO_BTF_ID, and either no
> modifiers are set, or PTR_TRUSTED is set". Or in other words, "If
> PTR_TO_BTF_ID is set, we don't need a refcount check unless a modifier
> other than PTR_TRUSTED is set on the register." This is incorrect, as it
> would short-circuit out of the check before !reg->ref_obj_id for
> reg->type == PTR_TO_BTF_ID, so we would skip the reference requirement
> for normal, unmodified PTR_TO_BTF_ID objects. It would also cause us to
> incorrectly _not_ skip the ref_obj_id > 0 check for when a
> reg2btf_ids[base_type(reg->type)] register has the PTR_TRUSTED modifier.
>
> What we really need is a check that encodes, "Don't require a refcount
> if PTR_TRUSTED is present and no other type modifiers are present",
> i.e.:
>
> !(type_flag(reg->type) & PTR_TRUSTED) || (type_flag(reg->type) & ~PTR_TRUSTED)
>
> My intention was to be conservative here and say "only trust PTR_TRUSTED
> if no other type modifiers are set". I think this is necessary because
> other type modifiers such as PTR_UNTRUSTED could theoretically be set on
> the register as well. Clearly this code is pretty difficult to reason
> about though, so I'm open to suggestions for how to simplify it.
>
> I'll point out specifically that it's difficult to reason about when
> modifiers are or are not safe to allow. For example, we definitely don't
> want to skip the refcount check for OBJ_RELEASE | PTR_TRUSTED, because
OBJ_RELEASE cannot be part of reg flag.
It's only in arg_type.
Anyway Kumar's refactoring was applied the code in question looks different now:
It would fall into this part:
case KF_ARG_PTR_TO_BTF_ID:
/* Only base_type is checked, further checks are done here */
if (reg->type != PTR_TO_BTF_ID &&
(!reg2btf_ids[base_type(reg->type)] || type_flag(reg->type))) {
verbose(env, "arg#%d expected pointer to btf or socket\n", i);
return -EINVAL;
}
ret = process_kf_arg_ptr_to_btf_id(env, reg, ref_t, ref_tname, ref_id, meta, i);
> if it's a release arg it should always have a refcount on it.
> PTR_UNTRUSTED | PTR_TRUSTED would also make no sense. MEM_FIXED_SIZE
> though seems fine? In general, I thought it was prudent for us to take
> the most conservative possible approach here, which is that PTR_TRUSTED
> only applies when no other modifiers are present, and it applies for all
> obj_ptr types (other than PTR_TO_CTX which does its own thing).
Probably worth refining when PTR_TRUSTED is cleared.
For example adding PTR_UNTRUSTED should definitely clear it.
MEM_ALLOC flag is probably equivalent to PTR_TRUSTED.
Maybe the bit:
regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC;
should set PTR_TRUSTED as well?
>
> Note as well that this check is different from the one you pointed out
> below, which is verifying that PTR_TRUSTED is the only modifier for both
> reg2btf_ids[base_type(reg->type)] and base_type(reg->type) ==
> PTR_TO_BTF_ID. Additionally, the check is different than the check in
> check_reg_type(), which I'll highlight below where the code is actually
> modified.
I'm mainly objecting to logic:
!(type_flag(reg->type) & PTR_TRUSTED) || (type_flag(reg->type) & ~PTR_TRUSTED)
which looks like 'catch-all'.
Like it will error on MEM_ALLOC which probably not correct.
In other words it's 'too conservative'. Meaning it's rejecting valid code.
> > > >
> > > > found:
> > > > - if (reg->type == PTR_TO_BTF_ID) {
> > > > + if (base_type(reg->type) == PTR_TO_BTF_ID && !(type_flag(reg->type) & ~PTR_TRUSTED)) {
>
> As mentioned above, this check is different than the one we're doing in
> btf_ctx_access() when determining if the reg requires a ref_obj_id > 0.
> This check is actually doing what you originally suggested above:
>
> if (reg->type == PTR_TO_BTF_ID || reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED))
>
> I think what you wrote is more readable and am happy to apply it to this
> check in v8, but unfortunately I don't think we really have an
> opportunity to avoid code duplication here with a helper (though a
> helper may still improve readability).
ok. forget the helper. open coding all conditions is probably cleaner,
since they will be different in every case.
