On Mon, Nov 7, 2022 at 3:10 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> Introduce local kptrs, i.e. PTR_TO_BTF_ID that point to a type in
> program BTF. This is indicated by the presence of MEM_ALLOC type flag in
> reg->type to avoid having to check btf_is_kernel when trying to match
> argument types in helpers.
>
> Refactor btf_struct_access callback to just take bpf_reg_state instead
> of btf and btf_type paramters. Note that the call site in
> check_map_access now simulates access to a PTR_TO_BTF_ID by creating a
> dummy reg on stack. Since only the type, btf, and btf_id of the register
> matter for the checks, it can be done so without complicating the usual
> cases elsewhere in the verifier where reg->btf and reg->btf_id is used
> verbatim.
>
> Whenever walking such types, any pointers being walked will always yield
> a SCALAR instead of pointer. In the future we might permit kptr inside
> local kptr (either kernel or local), and it would be permitted only in
> that case.
>
> 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). Note that once
> such a local kptr is marked PTR_UNTRUSTED, it is no longer allowed to
> write to it.
>
> No PROBE_MEM handling is therefore done for loads into this type unless
> PTR_UNTRUSTED is part of the register type, 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 | 28 ++++++++++++++++--------
> include/linux/filter.h | 8 +++----
> kernel/bpf/btf.c | 16 ++++++++++----
> kernel/bpf/verifier.c | 37 ++++++++++++++++++++++++++------
> net/bpf/bpf_dummy_struct_ops.c | 14 ++++++------
> net/core/filter.c | 34 ++++++++++++-----------------
> net/ipv4/bpf_tcp_ca.c | 13 ++++++-----
> net/netfilter/nf_conntrack_bpf.c | 17 ++++++---------
> 8 files changed, 99 insertions(+), 68 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index afc1c51b59ff..75dbd2ecf80a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -524,6 +524,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_obj_new.
> + */
> + MEM_ALLOC = BIT(11 + BPF_BASE_TYPE_BITS),
> +
you fixed one naming confusion with RINGBUF and basically are creating
a new one, where "ALLOC" means "local kptr"... If we are stuck with
"local kptr" (which I find very confusing as well, but that's beside
the point), why not stick to the whole "local" terminology here?
MEM_LOCAL?
> __BPF_TYPE_FLAG_MAX,
> __BPF_TYPE_LAST_FLAG = __BPF_TYPE_FLAG_MAX - 1,
> };
> @@ -771,6 +776,7 @@ struct bpf_prog_ops {
> union bpf_attr __user *uattr);
> };
>
[...]
> -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> - const struct btf_type *t, int off, int size,
> - enum bpf_access_type atype __maybe_unused,
> +int btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> + int off, int size, enum bpf_access_type atype __maybe_unused,
> u32 *next_btf_id, enum bpf_type_flag *flag)
> {
> + const struct btf *btf = reg->btf;
> enum bpf_type_flag tmp_flag = 0;
> + const struct btf_type *t;
> + u32 id = reg->btf_id;
> int err;
> - u32 id;
>
> + t = btf_type_by_id(btf, id);
> do {
> err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag);
>
> switch (err) {
> case WALK_PTR:
> + /* For local types, the destination register cannot
> + * become a pointer again.
> + */
> + if (type_is_local_kptr(reg->type))
> + return SCALAR_VALUE;
passing the entire bpf_reg_state just to differentiate between local
vs kernel pointer seems like a huge overkill. bpf_reg_state is quite a
complicated and extensive amount of state, and it seems cleaner to
just pass it as a flag whether to allow pointer chasing or not. At
least then we know we only care about that specific aspect, not about
dozens of other possible fields of bpf_reg_state.
> /* If we found the pointer or scalar on t+off,
> * we're done.
> */
[...]
