Message-ID: <20221118184500.yshwvcrx2a34xkmc@MacBook-Pro-5.local>
Date:   Fri, 18 Nov 2022 10:45:00 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Vernet <void@...ifault.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, andrii@...nel.org,
        daniel@...earbox.net, martin.lau@...ux.dev, memxor@...il.com,
        yhs@...com, song@...nel.org, sdf@...gle.com,
        john.fastabend@...il.com, kpsingh@...nel.org, jolsa@...nel.org,
        haoluo@...gle.com, tj@...nel.org, kernel-team@...com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v7 1/3] bpf: Allow trusted pointers to be passed
 to KF_TRUSTED_ARGS kfuncs

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.

Powered by blists - more mailing lists