Re: [PATCH 1/3] bpf: Add support to check if BTF object is nested in another object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux