On 10/13/22 2:22 AM, Kumar Kartikeya Dwivedi wrote:
> Introduce the idea of local kptrs, i.e. PTR_TO_BTF_ID that point to a
> type in program BTF. This is indicated by the presence of MEM_TYPE_LOCAL
> type tag in reg->type to avoid having to check btf_is_kernel when trying
> to match argument types in helpers.
>
> For now, these local kptrs will always be referenced in verifier
> context, hence ref_obj_id == 0 for them is a bug. It is allowed to write
> to such objects, as long fields that are special are not touched
> (support for which will be added in subsequent patches).
>
> No PROBE_MEM handling is hence done since they can never be in an
> undefined state, and their lifetime will always be valid.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> include/linux/bpf.h | 14 +++++++++++---
> include/linux/filter.h | 4 +++-
> kernel/bpf/btf.c | 9 ++++++++-
> kernel/bpf/verifier.c | 15 ++++++++++-----
> net/bpf/bpf_dummy_struct_ops.c | 3 ++-
> net/core/filter.c | 13 ++++++++-----
> net/ipv4/bpf_tcp_ca.c | 3 ++-
> net/netfilter/nf_conntrack_bpf.c | 1 +
> 8 files changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 46330d871d4e..a2f4d3356cc8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -526,6 +526,11 @@ enum bpf_type_flag {
> /* Size is known at compile time. */
> MEM_FIXED_SIZE = BIT(10 + BPF_BASE_TYPE_BITS),
>
> + /* MEM is of a type from program BTF, not kernel BTF. This is used to
> + * tag PTR_TO_BTF_ID allocated using bpf_kptr_alloc.
> + */
> + MEM_TYPE_LOCAL = BIT(11 + BPF_BASE_TYPE_BITS),
> +
> __BPF_TYPE_FLAG_MAX,
> __BPF_TYPE_LAST_FLAG = __BPF_TYPE_FLAG_MAX - 1,
> };
> @@ -774,6 +779,7 @@ struct bpf_prog_ops {
> union bpf_attr __user *uattr);
> };
>
> +struct bpf_reg_state;
> struct bpf_verifier_ops {
> /* return eBPF function prototype for verification */
> const struct bpf_func_proto *
> @@ -795,6 +801,7 @@ struct bpf_verifier_ops {
> struct bpf_insn *dst,
> struct bpf_prog *prog, u32 *target_size);
> int (*btf_struct_access)(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
Not that struct_ops API is meant to be stable, but would be good to note that
this changes that API in the summary.
On that note, maybe passing whole bpf_reg_state *reg can be avoided for now
by making this a 'bool disallow_ptr_walk' or similar, since that's the only
thing this patch is using it for.
> const struct btf *btf,
> const struct btf_type *t, int off, int size,
> enum bpf_access_type atype,
> @@ -2076,10 +2083,11 @@ static inline bool bpf_tracing_btf_ctx_access(int off, int size,
> return btf_ctx_access(off, size, type, prog, info);
> }
>
> -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> +int btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg, const struct btf *btf,
> const struct btf_type *t, int off, int size,
> - enum bpf_access_type atype,
> - u32 *next_btf_id, enum bpf_type_flag *flag);
> + enum bpf_access_type atype, u32 *next_btf_id,
> + enum bpf_type_flag *flag);
> bool btf_struct_ids_match(struct bpf_verifier_log *log,
> const struct btf *btf, u32 id, int off,
> const struct btf *need_btf, u32 need_type_id,
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index efc42a6e3aed..9b94e24f90b9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -568,7 +568,9 @@ struct sk_filter {
> DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>
> extern struct mutex nf_conn_btf_access_lock;
> -extern int (*nfct_btf_struct_access)(struct bpf_verifier_log *log, const struct btf *btf,
> +extern int (*nfct_btf_struct_access)(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> + const struct btf *btf,
> const struct btf_type *t, int off, int size,
> enum bpf_access_type atype, u32 *next_btf_id,
> enum bpf_type_flag *flag);
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 066984d73a8b..65f444405d9c 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6019,11 +6019,13 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> return -EINVAL;
> }
>
> -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> +int btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg, const struct btf *btf,
> const struct btf_type *t, int off, int size,
> enum bpf_access_type atype __maybe_unused,
> u32 *next_btf_id, enum bpf_type_flag *flag)
> {
> + bool local_type = reg && (type_flag(reg->type) & MEM_TYPE_LOCAL);
> enum bpf_type_flag tmp_flag = 0;
> int err;
> u32 id;
> @@ -6033,6 +6035,11 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
>
> switch (err) {
> case WALK_PTR:
> + /* For local types, the destination register cannot
> + * become a pointer again.
> + */
> + if (local_type)
> + return SCALAR_VALUE;
> /* If we found the pointer or scalar on t+off,
> * we're done.
> */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c47cecda302..6ee8c06c2080 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4522,16 +4522,20 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> return -EACCES;
> }
>
> - if (env->ops->btf_struct_access) {
> - ret = env->ops->btf_struct_access(&env->log, reg->btf, t,
> + if (env->ops->btf_struct_access && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
> + WARN_ON_ONCE(!btf_is_kernel(reg->btf));
> + ret = env->ops->btf_struct_access(&env->log, reg, reg->btf, t,
> off, size, atype, &btf_id, &flag);
> } else {
> - if (atype != BPF_READ) {
> + if (atype != BPF_READ && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
> verbose(env, "only read is supported\n");
> return -EACCES;
> }
>
> - ret = btf_struct_access(&env->log, reg->btf, t, off, size,
> + if (reg->type & MEM_TYPE_LOCAL)
> + WARN_ON_ONCE(!reg->ref_obj_id);
Can we instead verbose(env, ...) and return error? Then when someone tries to
add local kptrs that don't set ref_obj_id in the future, it'll be more obvious
that this wasn't explicitly supported and they need to check verifier logic
carefully. Also rest of check_ptr_to_btf_access checks do verbose + err.
Similar for btf_is_kernel WARN above.
> +
> + ret = btf_struct_access(&env->log, reg, reg->btf, t, off, size,
more re: passing entire reg state to btf_struct access:
In the next patch in the series ("bpf: Recognize bpf_{spin_lock,list_head,list_node} in local kptrs")
you do btf_find_struct_meta(btf, reg->btf_id). I see why you couldn't use 't'
that's passed in here / elsewhere since you need the btf_id for meta lookup.
Perhaps 'btf_type *t' param can be changed to btf_id, eliminating the need
to pass 'reg'.
Alternatively, since we're already passing reg->btf and result of
btf_type_by_id(reg->btf, reg->btf_id), seems like btf_struct_access
maybe is tied closely enough to reg state that passing reg state
directly and getting rid of extraneous args is cleaner.
> atype, &btf_id, &flag);
> }
>
> @@ -4596,7 +4600,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
> return -EACCES;
> }
>
> - ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag);
> + ret = btf_struct_access(&env->log, NULL, btf_vmlinux, t, off, size, atype, &btf_id, &flag);
> if (ret < 0)
> return ret;
>
> @@ -5816,6 +5820,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> * fixed offset.
> */
> case PTR_TO_BTF_ID:
> + case PTR_TO_BTF_ID | MEM_TYPE_LOCAL:
> /* 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.
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index e78dadfc5829..d7aa636d90ce 100644
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -156,6 +156,7 @@ static bool bpf_dummy_ops_is_valid_access(int off, int size,
> }
>
> static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> const struct btf *btf,
> const struct btf_type *t, int off,
> int size, enum bpf_access_type atype,
> @@ -177,7 +178,7 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
> return -EACCES;
> }
>
> - err = btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> + err = btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id,
> flag);
> if (err < 0)
> return err;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index bb0136e7a8e4..cc7af7be91d9 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8647,13 +8647,15 @@ static bool tc_cls_act_is_valid_access(int off, int size,
> DEFINE_MUTEX(nf_conn_btf_access_lock);
> EXPORT_SYMBOL_GPL(nf_conn_btf_access_lock);
>
> -int (*nfct_btf_struct_access)(struct bpf_verifier_log *log, const struct btf *btf,
> +int (*nfct_btf_struct_access)(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg, const struct btf *btf,
> const struct btf_type *t, int off, int size,
> enum bpf_access_type atype, u32 *next_btf_id,
> enum bpf_type_flag *flag);
> EXPORT_SYMBOL_GPL(nfct_btf_struct_access);
>
> static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> const struct btf *btf,
> const struct btf_type *t, int off,
> int size, enum bpf_access_type atype,
> @@ -8663,12 +8665,12 @@ static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
> int ret = -EACCES;
>
> if (atype == BPF_READ)
> - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id,
> flag);
>
> mutex_lock(&nf_conn_btf_access_lock);
> if (nfct_btf_struct_access)
> - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
> + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag);
> mutex_unlock(&nf_conn_btf_access_lock);
>
> return ret;
> @@ -8734,6 +8736,7 @@ void bpf_warn_invalid_xdp_action(struct net_device *dev, struct bpf_prog *prog,
> EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
>
> static int xdp_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> const struct btf *btf,
> const struct btf_type *t, int off,
> int size, enum bpf_access_type atype,
> @@ -8743,12 +8746,12 @@ static int xdp_btf_struct_access(struct bpf_verifier_log *log,
> int ret = -EACCES;
>
> if (atype == BPF_READ)
> - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id,
> flag);
>
> mutex_lock(&nf_conn_btf_access_lock);
> if (nfct_btf_struct_access)
> - ret = nfct_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
> + ret = nfct_btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id, flag);
> mutex_unlock(&nf_conn_btf_access_lock);
>
> return ret;
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 6da16ae6a962..1fe3935c4260 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -69,6 +69,7 @@ static bool bpf_tcp_ca_is_valid_access(int off, int size,
> }
>
> static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> const struct btf *btf,
> const struct btf_type *t, int off,
> int size, enum bpf_access_type atype,
> @@ -78,7 +79,7 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
> size_t end;
>
> if (atype == BPF_READ)
> - return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> + return btf_struct_access(log, reg, btf, t, off, size, atype, next_btf_id,
> flag);
>
> if (t != tcp_sock_type) {
> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index 8639e7efd0e2..f6036a84484b 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -191,6 +191,7 @@ BTF_ID(struct, nf_conn___init)
>
> /* Check writes into `struct nf_conn` */
> static int _nf_conntrack_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> const struct btf *btf,
> const struct btf_type *t, int off,
> int size, enum bpf_access_type atype,
