On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
>
> Move the check for a NULL or zero register to check_helper_mem_access. This
> makes check_stack_boundary easier to understand.
>
> Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> ---
Looks good as is, but I'm curious about the question below.
Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> kernel/bpf/verifier.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 509754c3aa7d..649bcfb4535e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
> struct bpf_func_state *state = func(env, reg);
> int err, min_off, max_off, i, j, slot, spi;
>
> - if (reg->type != PTR_TO_STACK) {
> - /* Allow zero-byte read from NULL, regardless of pointer type */
> - if (zero_size_allowed && access_size == 0 &&
> - register_is_null(reg))
> - return 0;
> -
> - verbose(env, "R%d type=%s expected=%s\n", regno,
> - reg_type_str[reg->type],
> - reg_type_str[PTR_TO_STACK]);
> - return -EACCES;
> - }
> -
> if (tnum_is_const(reg->var_off)) {
> min_off = max_off = reg->var_off.value + reg->off;
> err = __check_stack_boundary(env, regno, min_off, access_size,
> @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> access_size, zero_size_allowed,
> "rdwr",
> &env->prog->aux->max_rdwr_access);
> - default: /* scalar_value|ptr_to_stack or invalid ptr */
> + case PTR_TO_STACK:
> return check_stack_boundary(env, regno, access_size,
> zero_size_allowed, meta);
> + default: /* scalar_value or invalid ptr */
> + /* Allow zero-byte read from NULL, regardless of pointer type */
> + if (zero_size_allowed && access_size == 0 &&
> + register_is_null(reg))
> + return 0;
Given comment explicitly states "regardless of pointer type",
shouldn't this be checked before we do pointer type-specific checks?
> +
> + verbose(env, "R%d type=%s expected=%s\n", regno,
> + reg_type_str[reg->type],
> + reg_type_str[PTR_TO_STACK]);
> + return -EACCES;
> }
> }
>
> --
> 2.25.1
>
