linux-kernel - Re: [PATCH 1/5] bpf: Clear callee saved regs after updating REG0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 8 Aug 2022 11:14:48 -0700
From:   Joanne Koong <joannelkoong@...il.com>
To:     David Vernet <void@...ifault.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, john.fastabend@...il.com, martin.lau@...ux.dev,
        song@...nel.org, yhs@...com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, tj@...nel.org,
        linux-kernel@...r.kernel.org, Kernel-team@...com
Subject: Re: [PATCH 1/5] bpf: Clear callee saved regs after updating REG0

On Mon, Aug 8, 2022 at 8:53 AM David Vernet <void@...ifault.com> wrote:
>
> In the verifier, we currently reset all of the registers containing caller
> saved args before updating the callee's return register (REG0). In a
> follow-on patch, we will need to be able to be able to inspect the caller
> saved registers when updating REG0 to determine if a dynptr that's passed
> to a helper function was allocated by a helper, or allocated by a program.
>
> This patch therefore updates check_helper_call() to clear the caller saved
> regs after updating REG0.
>
Overall lgtm

There's a patch [0] that finds + stores the ref obj id before the
caller saved regs get reset, which would make this patch not needed.
That hasn't been merged in yet though and I think there's pros for
either approach.

In the one where we find + store the ref obj id before any caller
saved regs get reset, the pro is that getting the dynptr metadata (eg
ref obj id and in the near future, the dynptr type as well) earlier
will be useful (eg when we add skb/xdp dynptrs [1], we'll need to know
the type of the dynptr in order to determine whether to set the return
reg as PTR_TO_PACKET). In this patch, the pro is that the logic is a
lot more obvious to readers that the ref obj id for the dynptr gets
found and set in order to store it in the return reg's ref obj id.

I personally lean more towards the approach in [0] because I think
that ends up being cleaner for future extensibility, but I don't feel
strongly about it and would be happy going with this approach as well

[0] https://lore.kernel.org/bpf/20220722175807.4038317-1-joannelkoong@gmail.com/#t

[1] https://lore.kernel.org/bpf/20220726184706.954822-1-joannelkoong@gmail.com/T/#t

> Signed-off-by: David Vernet <void@...ifault.com>
> ---
>  kernel/bpf/verifier.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 096fdac70165..938ba1536249 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7348,11 +7348,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>         if (err)
>                 return err;
>
> -       /* reset caller saved regs */
> -       for (i = 0; i < CALLER_SAVED_REGS; i++) {
> -               mark_reg_not_init(env, regs, caller_saved[i]);
> -               check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
> -       }
> +       /* reset return reg */
> +       mark_reg_not_init(env, regs, BPF_REG_0);
> +       check_reg_arg(env, BPF_REG_0, DST_OP_NO_MARK);
>
>         /* helper call returns 64-bit value. */
>         regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG;
> @@ -7488,6 +7486,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                 regs[BPF_REG_0].ref_obj_id = dynptr_id;
>         }
>
> +       /* reset remaining caller saved regs */
> +       BUILD_BUG_ON(caller_saved[0] != BPF_REG_0);

nit: caller_saved is a read-only const, so I don't think this line is needed

> +       for (i = 1; i < CALLER_SAVED_REGS; i++) {

nit: maybe "for i = BPF_REG_1" ?

> +               mark_reg_not_init(env, regs, caller_saved[i]);
> +               check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
> +       }
> +
>         do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
>
>         err = check_map_func_compatibility(env, meta.map_ptr, func_id);
> --
> 2.30.2
>