On Thu, Nov 9, 2023 at 7:21 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>
> Could you please add a test case?
>
There is already at least one test case that relies on this behavior
:) But yep, I'll add a dedicated test.
> [...]
>
> > ---
> > 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?
I don't think so. We rely on all bytes we are reading to be either
spills (and thus spill_cnt == size), in which case verifier logic
makes sure we have spill at slot boundary (off % BPF_REG_SIZE == 0).
Or it's all STACK_ZERO, and then zero_cnt == size, in which case we
know it's zero.
Unless I missed something else?
>
> [...]
>
>
