Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 19 Nov 2022 16:20:26 -0600
From:   David Vernet <void@manifault.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     ast@kernel.org, andrii@kernel.org, daniel@iogearbox.net,
        martin.lau@linux.dev, yhs@fb.com, song@kernel.org, sdf@google.com,
        john.fastabend@gmail.com, haoluo@google.com, jolsa@kernel.org,
        kpsingh@kernel.org, memxor@gmail.com, tj@kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@fb.com
Subject: Re: [PATCH bpf-next v8 1/3] bpf: Allow trusted pointers to be passed
 to KF_TRUSTED_ARGS kfuncs
Message-ID: <Y3lWqouIxfOpMGmE@maniforge.lan>
References: <20221119210748.3325667-1-void@manifault.com>
 <20221119210748.3325667-2-void@manifault.com>
 <20221119220246.k4i3zp5wmsm6g2al@macbook-pro-5.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221119220246.k4i3zp5wmsm6g2al@macbook-pro-5.dhcp.thefacebook.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk

On Sat, Nov 19, 2022 at 02:02:46PM -0800, Alexei Starovoitov wrote:
> On Sat, Nov 19, 2022 at 03:07:46PM -0600, David Vernet wrote:
> > @@ -6887,6 +6895,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> >  			}
> >  
> >  			reg->type = PTR_TO_MEM | PTR_MAYBE_NULL;
> > +
> 
> No need to add empty line here.

Ack

> >  			reg->id = ++env->id_gen;
> >  
> >  			continue;
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 195d24316750..3a90a1c7613f 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -557,7 +557,7 @@ static bool is_cmpxchg_insn(const struct bpf_insn *insn)
> >  static const char *reg_type_str(struct bpf_verifier_env *env,
> >  				enum bpf_reg_type type)
> >  {
> > -	char postfix[16] = {0}, prefix[32] = {0};
> > +	char postfix[16] = {0}, prefix[64] = {0};
> >  	static const char * const str[] = {
> >  		[NOT_INIT]		= "?",
> >  		[SCALAR_VALUE]		= "scalar",
> > @@ -589,16 +589,14 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> >  			strncpy(postfix, "_or_null", 16);
> >  	}
> >  
> > -	if (type & MEM_RDONLY)
> > -		strncpy(prefix, "rdonly_", 32);
> > -	if (type & MEM_RINGBUF)
> > -		strncpy(prefix, "ringbuf_", 32);
> > -	if (type & MEM_USER)
> > -		strncpy(prefix, "user_", 32);
> > -	if (type & MEM_PERCPU)
> > -		strncpy(prefix, "percpu_", 32);
> > -	if (type & PTR_UNTRUSTED)
> > -		strncpy(prefix, "untrusted_", 32);
> > +	snprintf(prefix, sizeof(prefix), "%s%s%s%s%s%s",
> > +		 type & MEM_RDONLY ? "rdonly_" : "",
> > +		 type & MEM_RINGBUF ? "ringbuf_" : "",
> > +		 type & MEM_USER ? "user_" : "",
> > +		 type & MEM_PERCPU ? "percpu_" : "",
> > +		 type & PTR_UNTRUSTED ? "untrusted_" : "",
> > +		 type & PTR_TRUSTED ? "trusted_" : ""
> > +	);
> 
> Nice. Could have been a separate patch, but ok.

Will do next time, sorry for the bloat in this one.

> 
> >  
> >  found:
> > -	if (reg->type == PTR_TO_BTF_ID) {
> > +	if (reg->type == PTR_TO_BTF_ID || (reg->type & PTR_TRUSTED)) {
> 
> No need for (). The operator precedence is pretty clear.

Ack

> >  		/* For bpf_sk_release, it needs to match against first member
> >  		 * 'struct sock_common', hence make an exception for it. This
> >  		 * allows bpf_sk_release to work for multiple socket types.
> > @@ -6058,6 +6070,8 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> >  	 */
> >  	case PTR_TO_BTF_ID:
> >  	case PTR_TO_BTF_ID | MEM_ALLOC:
> > +	case PTR_TO_BTF_ID | PTR_TRUSTED:
> > +	case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED:
> >  		/* When referenced PTR_TO_BTF_ID is passed to release function,
> >  		 * it's fixed offset must be 0.	In the other cases, fixed offset
> >  		 * can be non-zero.
> > @@ -7942,6 +7956,25 @@ static bool is_kfunc_arg_kptr_get(struct bpf_kfunc_call_arg_meta *meta, int arg)
> >  	return arg == 0 && (meta->kfunc_flags & KF_KPTR_GET);
> >  }
> >  
> > +static bool is_trusted_reg(const struct bpf_reg_state *reg)
> > +{
> > +	/* A referenced register is always trusted. */
> > +	if (reg->ref_obj_id)
> > +		return true;
> > +
> > +	/* If a register is not referenced, it is trusted if it has either the
> > +	 * MEM_ALLOC or PTR_TRUSTED type modifiers, and no others. Some of the
> > +	 * other type modifiers may be safe, but we elect to take an opt-in
> > +	 * approach here as some (e.g. PTR_UNTRUSTED and PTR_MAYBE_NULL) are
> > +	 * not.
> > +	 *
> > +	 * Eventually, we should make PTR_TRUSTED the single source of truth
> > +	 * for whether a register is trusted.
> > +	 */
> > +	return (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS) &&
> 
> No need for ().

Ack

> > +	       !bpf_type_has_unsafe_modifiers(reg->type);
> > +}
> > +
> ...
> > -		if (is_kfunc_release(meta) && reg->ref_obj_id)
> > +		if (is_kfunc_release(meta) && reg->ref_obj_id) {
> >  			arg_type |= OBJ_RELEASE;
> > +			if (bpf_type_has_unsafe_modifiers(reg->type)) {
> > +				verbose(env, "R%d release reg has unsafe modifiers\n", i);
> > +				return -EINVAL;
> > +			}
> 
> This part is a bit controversial, sicne it messes up the verifier messages.
> Meaning that doing the check that early is losing important context.
> 
> > +		}
> >  		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
> >  		if (ret < 0)
> >  			return ret;
> > @@ -8705,7 +8745,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> >  			break;
> >  		case KF_ARG_PTR_TO_BTF_ID:
> >  			/* Only base_type is checked, further checks are done here */
> > -			if (reg->type != PTR_TO_BTF_ID &&
> > +			if (base_type(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);
> 
> With base_type() addition maybe the bpf_type_has_unsafe_modifiers() check
> should be done here ?
> Then test_verifier wouldn't need to change.
> It's not the change itself that is a concern, but the loss of context in the messages.
> I guess one can argue that erroring on PTR_TO_BTF_ID | PTR_MAYBE_NULL
> with "reg has unsafe modifiers" is just as correct as saying
> "expected pointer to btf or socket" a bit later.

This was my thinking. I thought it was a clearer message than "expected
pointer to btf or socket". It _is_ a ptr to btf, but it has modifiers.
Teasing that apart for the release reg seemed like an improvement.

> Both could be improved.
> If we keep it early while doing is_kfunc_release(meta) && reg->ref_obj_id
> we could say:
> "%s is not allowed in release function"
> reg_type_str(env,reg->type)
> Which for verifier/calls.c test case will be:
> "ptr_prog_test_ref_kfunc_or_null is not allowed in release function"
> 
> If we do it later here it could be:
> "arg#$d is %s. Expected %s or socket",
> reg_type_str(env,reg->type)
> reg_type_str(env,base_type(reg->type) | type_flag(reg->type) & ~BPF_REG_TRUSTED_MODIFIERS)
> "arg#0 is ptr_prog_test_ref_kfunc_or_null. Expected ptr_prog_test_ref_kfunc or socket"
> 
> which is even better and it will make it easier for user to fix the code.

I like this, it's much better. I'll send out v9 with this change.