On Fri, Mar 04, 2022 at 05:35:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> Lift the list of register types allowed for having fixed and variable
> offsets when passed as helper function arguments into a common helper,
> so that they can be reused for kfunc checks in later commits. Keeping a
> common helper aids maintainability and allows us to follow the same
> consistent rules across helpers and kfuncs. Also, convert check_func_arg
> to use this function.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> include/linux/bpf_verifier.h | 3 ++
> kernel/bpf/verifier.c | 69 +++++++++++++++++++++---------------
> 2 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 7a7be8c057f2..38b24ee8d8c2 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>
> int check_ptr_off_reg(struct bpf_verifier_env *env,
> const struct bpf_reg_state *reg, int regno);
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> + const struct bpf_reg_state *reg, int regno,
> + enum bpf_arg_type arg_type);
> int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> u32 regno);
> int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a57db4b2803c..c85f4b2458f4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> return 0;
> }
>
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> + const struct bpf_reg_state *reg, int regno,
> + enum bpf_arg_type arg_type)
> +{
> + enum bpf_reg_type type = reg->type;
> + int err;
> +
> + switch ((u32)type) {
> + case SCALAR_VALUE:
> + /* Pointer types where reg offset is explicitly allowed: */
> + case PTR_TO_PACKET:
> + case PTR_TO_PACKET_META:
> + case PTR_TO_MAP_KEY:
> + case PTR_TO_MAP_VALUE:
> + case PTR_TO_MEM:
> + case PTR_TO_MEM | MEM_RDONLY:
> + case PTR_TO_MEM | MEM_ALLOC:
> + case PTR_TO_BUF:
> + case PTR_TO_BUF | MEM_RDONLY:
> + case PTR_TO_STACK:
> + /* Some of the argument types nevertheless require a
> + * zero register offset.
> + */
> + if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> + goto force_off_check;
> + break;
> + /* All the rest must be rejected: */
> + default:
> +force_off_check:
> + err = __check_ptr_off_reg(env, reg, regno,
> + type == PTR_TO_BTF_ID);
> + if (err < 0)
> + return err;
Nit. Since it is refactoring to a new function now,
it can directly return __check_ptr_off_reg().
> + break;
> + }
> + return 0;
> +}
May as well flip the arg_type check and leave calling
the __check_ptr_off_reg at the end.
Uncompiled code:
int check_func_arg_reg_off(struct bpf_verifier_env *env,
const struct bpf_reg_state *reg, int regno,
enum bpf_arg_type arg_type)
{
enum bpf_reg_type type = reg->type;
bool fixed_off_ok = false;
switch ((u32)type) {
case SCALAR_VALUE:
/* Pointer types where reg offset is explicitly allowed: */
case PTR_TO_PACKET:
case PTR_TO_PACKET_META:
case PTR_TO_MAP_KEY:
case PTR_TO_MAP_VALUE:
case PTR_TO_MEM:
case PTR_TO_MEM | MEM_RDONLY:
case PTR_TO_MEM | MEM_ALLOC:
case PTR_TO_BUF:
case PTR_TO_BUF | MEM_RDONLY:
case PTR_TO_STACK:
/* Some of the argument types nevertheless require a
* zero register offset.
*/
if (arg_type != ARG_PTR_TO_ALLOC_MEM)
return 0;
break;
case PTR_TO_BTF_ID:
fixed_off_ok = true;
break;
}
return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
}
Then 'case PTR_TO_BTF_ID' can then be reused in patch 4 instead
of adding a special 'if (type == PTR_TO_BTF_ID)' just
before the 'switch ((u32)type)'
Both are nits. can be ignored.
> +
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> const struct bpf_func_proto *fn)
> @@ -5408,34 +5446,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> if (err)
> return err;
>
> - switch ((u32)type) {
> - case SCALAR_VALUE:
> - /* Pointer types where reg offset is explicitly allowed: */
> - case PTR_TO_PACKET:
> - case PTR_TO_PACKET_META:
> - case PTR_TO_MAP_KEY:
> - case PTR_TO_MAP_VALUE:
> - case PTR_TO_MEM:
> - case PTR_TO_MEM | MEM_RDONLY:
> - case PTR_TO_MEM | MEM_ALLOC:
> - case PTR_TO_BUF:
> - case PTR_TO_BUF | MEM_RDONLY:
> - case PTR_TO_STACK:
> - /* Some of the argument types nevertheless require a
> - * zero register offset.
> - */
> - if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> - goto force_off_check;
> - break;
> - /* All the rest must be rejected: */
> - default:
> -force_off_check:
> - err = __check_ptr_off_reg(env, reg, regno,
> - type == PTR_TO_BTF_ID);
> - if (err < 0)
> - return err;
> - break;
> - }
> + err = check_func_arg_reg_off(env, reg, regno, arg_type);
> + if (err)
> + return err;
>
> skip_type_check:
> if (reg->ref_obj_id) {
> --
> 2.35.1
>
