Re: [PATCH bpf-next 6/7] bpf: preserve constant zero when doing partial register restore

Eduard Zingerman
2023-11-09T15:21:02Z

On Mon, 2023-10-30 at 22:03 -0700, Andrii Nakryiko wrote:
> Similar to special handling of STACK_ZERO, when reading 1/2/4 bytes from
> stack from slot that has register spilled into it and that register has
> a constant value zero, preserve that zero and mark spilled register as
> precise for that. This makes spilled const zero register and STACK_ZERO
> cases equivalent in their behavior.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Could you please add a test case?

[...]

> ---
> kernel/bpf/verifier.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0eecc6b3109c..8cfe060e4938 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4958,22 +4958,39 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
> copy_register_state(&state->regs[dst_regno], reg);
> state->regs[dst_regno].subreg_def = subreg_def;
> } else {
[...]
> +
> + if (spill_cnt == size &&
> + tnum_is_const(reg->var_off) && reg->var_off.value == 0) {
> + __mark_reg_const_zero(&state->regs[dst_regno]);
> + /* this IS register fill, so keep insn_flags */
> + } else if (zero_cnt == size) {
> + /* similarly to mark_reg_stack_read(), preserve zeroes */
> + __mark_reg_const_zero(&state->regs[dst_regno]);
> + insn_flags = 0; /* not restoring original register state */
> + } else {
> + mark_reg_unknown(env, state->regs, dst_regno);
> + insn_flags = 0; /* not restoring original register state */
> + }

Condition for this branch is (off % BPF_REG_SIZE != 0) || size != spill_size,
is it necessary to check for some unusual offsets, e.g. off % BPF_REG_SIZE == 7
or something like that? …