On Wed, Apr 01, 2020 at 01:09:05PM +0200, Jiri Olsa wrote:
> Adding btf_struct_address function that takes 2 BTF objects
> and offset as arguments and checks wether object A is nested
> in object B on given offset.
>
> This function is be used when checking the helper function
> PTR_TO_BTF_ID arguments. If the argument has an offset value,
> the btf_struct_address will check if the final address is
> the expected BTF ID.
>
> This way we can access nested BTF objects under PTR_TO_BTF_ID
> pointer type and pass them to helpers, while they still point
> to valid kernel BTF objects.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 3 ++
> kernel/bpf/btf.c | 69 +++++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 18 +++++++++--
> 3 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index fd2b2322412d..d3bad7ee60c6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1196,6 +1196,9 @@ int btf_struct_access(struct bpf_verifier_log *log,
> const struct btf_type *t, int off, int size,
> enum bpf_access_type atype,
> u32 *next_btf_id);
> +int btf_struct_address(struct bpf_verifier_log *log,
> + const struct btf_type *t,
> + u32 off, u32 exp_id);
> int btf_resolve_helper_id(struct bpf_verifier_log *log,
> const struct bpf_func_proto *fn, int);
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d65c6912bdaf..9aafffa57d8b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4002,6 +4002,75 @@ int btf_struct_access(struct bpf_verifier_log *log,
> return -EINVAL;
> }
>
> +int btf_struct_address(struct bpf_verifier_log *log,
> + const struct btf_type *t,
> + u32 off, u32 exp_id)
> +{
> + u32 i, moff, mtrue_end, msize = 0;
> + const struct btf_member *member;
> + const struct btf_type *mtype;
> + const char *tname, *mname;
> +
> +again:
> + tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> + if (!btf_type_is_struct(t)) {
> + bpf_log(log, "Type '%s' is not a struct\n", tname);
> + return -EINVAL;
> + }
> +
> + if (off > t->size) {
> + bpf_log(log, "address beyond struct %s at off %u size %u\n",
> + tname, off, t->size);
> + return -EACCES;
> + }
> +
> + for_each_member(i, t, member) {
> + /* offset of the field in bytes */
> + moff = btf_member_bit_offset(t, member) / 8;
> + if (off < moff)
> + /* won't find anything, field is already too far */
> + break;
> +
> + /* we found the member */
> + if (off == moff && member->type == exp_id)
> + return 0;
> +
> + /* type of the field */
> + mtype = btf_type_by_id(btf_vmlinux, member->type);
> + mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
> +
> + mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
> + NULL, NULL);
> + if (IS_ERR(mtype)) {
> + bpf_log(log, "field %s doesn't have size\n", mname);
> + return -EFAULT;
> + }
> +
> + mtrue_end = moff + msize;
> + if (off >= mtrue_end)
> + /* no overlap with member, keep iterating */
> + continue;
> +
> + /* the 'off' we're looking for is either equal to start
> + * of this field or inside of this struct
> + */
> + if (btf_type_is_struct(mtype)) {
> + /* our field must be inside that union or struct */
> + t = mtype;
> +
> + /* adjust offset we're looking for */
> + off -= moff;
> + goto again;
> + }
Looks like copy-paste that should be generalized into common helper.
> +
> + bpf_log(log, "struct %s doesn't have struct field at offset %d\n", tname, off);
> + return -EACCES;
> + }
> +
> + bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
> + return -EACCES;
> +}
> +
> static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
> int arg)
> {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 04c6630cc18f..6eb88bef4379 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3103,6 +3103,18 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> return 0;
> }
>
> +static void check_ptr_in_btf(struct bpf_verifier_env *env,
> + struct bpf_reg_state *reg,
> + u32 exp_id)
> +{
> + const struct btf_type *t = btf_type_by_id(btf_vmlinux, reg->btf_id);
> +
> + if (!btf_struct_address(&env->log, t, reg->off, exp_id)) {
> + reg->btf_id = exp_id;
> + reg->off = 0;
This doesn't look right.
If you simply overwrite btf_id and off in the reg it will contain wrong info
in any subsequent instruction.
Typically it would be ok, since this reg is a function argument and will be
scratched after the call, but consider:
bpf_foo(&file->f_path, &file->f_owner);
The same base register will be used to construct R1 and R2
and above re-assign will screw up R1.
