netdev - Re: [PATCH bpf-next v2 01/14] bpf: Support getting referenced kptr from struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce15f61de21a4415d00d2e52c2eedc63564093c8.camel@gmail.com>
Date: Thu, 23 Jan 2025 01:57:08 -0800
From: Eduard Zingerman <eddyz87@...il.com>
To: Amery Hung <ameryhung@...il.com>, netdev@...r.kernel.org
Cc: bpf@...r.kernel.org, daniel@...earbox.net, andrii@...nel.org, 
	alexei.starovoitov@...il.com, martin.lau@...nel.org, sinquersw@...il.com, 
	toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, stfomichev@...il.com, 
	ekarani.silvestre@....ufcg.edu.br, yangpeihao@...u.edu.cn, 
	xiyou.wangcong@...il.com, yepeilin.cs@...il.com, amery.hung@...edance.com
Subject: Re: [PATCH bpf-next v2 01/14] bpf: Support getting referenced kptr
 from struct_ops argument

On Fri, 2024-12-20 at 11:55 -0800, Amery Hung wrote:
> From: Amery Hung <amery.hung@...edance.com>
> 
> Allows struct_ops programs to acqurie referenced kptrs from arguments
> by directly reading the argument.
> 
> The verifier will acquire a reference for struct_ops a argument tagged
> with "__ref" in the stub function in the beginning of the main program.
> The user will be able to access the referenced kptr directly by reading
> the context as long as it has not been released by the program.
> 
> This new mechanism to acquire referenced kptr (compared to the existing
> "kfunc with KF_ACQUIRE") is introduced for ergonomic and semantic reasons.
> In the first use case, Qdisc_ops, an skb is passed to .enqueue in the
> first argument. This mechanism provides a natural way for users to get a
> referenced kptr in the .enqueue struct_ops programs and makes sure that a
> qdisc will always enqueue or drop the skb.
> 
> Signed-off-by: Amery Hung <amery.hung@...edance.com>
> ---

Hi Amery,

Sorry, for joining so late in the review process.
Decided to take a look at verifier related changes.
Overall the patch looks good to me,
but I dislike the part allocating parameter ids.

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 28246c59e12e..c2f4f84e539d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6682,6 +6682,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  			info->reg_type = ctx_arg_info->reg_type;
>  			info->btf = ctx_arg_info->btf ? : btf_vmlinux;
>  			info->btf_id = ctx_arg_info->btf_id;
> +			info->ref_obj_id = ctx_arg_info->refcounted ? ++nr_ref_args : 0;
>  			return true;
>  		}
>  	}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f27274e933e5..26305571e377 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[...]

> @@ -22161,6 +22182,16 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  		mark_reg_known_zero(env, regs, BPF_REG_1);
>  	}
>  
> +	/* Acquire references for struct_ops program arguments tagged with "__ref".
> +	 * These should be the earliest references acquired. btf_ctx_access() will
> +	 * assume the ref_obj_id of the n-th __ref-tagged argument to be n.
> +	 */
> +	if (!subprog && env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> +		for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
> +			if (env->prog->aux->ctx_arg_info[i].refcounted)
> +				acquire_reference(env, 0);
> +	}
> +
>  	ret = do_check(env);
>  out:
>  	/* check for NULL is necessary, since cur_state can be freed inside

I think it would be cleaner if:
- each program would own it's instance of 'env->prog->aux->ctx_arg_info';
- ref_obj_id field would be added to 'struct bpf_ctx_arg_aux';
- parameter ids would be allocated in do_check_common(), but without
  reliance on being first to allocate.

Or add some rigour to this thing and e.g. make env->id_gen signed
and declare an enum of special ids like:

  enum special_ids {
  	STRUCT_OPS_CTX_PARAM_0 = -1,
  	STRUCT_OPS_CTX_PARAM_1 = -2,
  	STRUCT_OPS_CTX_PARAM_2 = -3,
  	...
  }

and update the loop above as:

	if (!subprog && env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
		for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
			if (env->prog->aux->ctx_arg_info[i].refcounted)
            	/* imagined function that acquires an id with specific value */
				acquire_special_reference(env, 0, STRUCT_OPS_CTX_PARAM_0 - i /* desired id */);
	}

wdyt?