On Thu, Nov 9, 2023 at 7:20 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-10-30 at 22:03 -0700, Andrii Nakryiko wrote:
> > > Given verifier checks actual value, r0 has to be precise, so we need to
> > > propagate precision properly.
> > >
> > > Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper")
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> I don't follow why this is necessary, could you please conjure
> an example showing that current behavior is not safe?
> This example could be used as a test case, as this change
> seems to not be covered by test cases.
We rely on callbacks to return specific value (0 or 1, for example),
and use or might use that in kernel code. So if we rely on the
specific value of a register, it has to be precise. Marking r0 as
precise will have implications on other registers from which r0 was
derived. This might have implications on state pruning and stuff. If
r0 and its ancestors are not precise, we might erroneously assume some
states are safe and prune them, even though they are not.
I'll see if I can come up with a simple and quick test. I can always
drop this change, it was a bit of a drive-by bug I noticed while
looking for other issues.
>
> > > ---
> > > kernel/bpf/verifier.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index fbb779583d52..098ba0e1a6ff 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -9739,6 +9739,12 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
> > > verbose(env, "R0 not a scalar value\n");
> > > return -EACCES;
> > > }
> > > +
> > > + /* we are going to enforce precise value, mark r0 precise */
> > > + err = mark_chain_precision(env, BPF_REG_0);
> > > + if (err)
> > > + return err;
> > > +
> > > if (!tnum_in(range, r0->var_off)) {
> > > verbose_invalid_scalar(env, r0, &range, "callback return", "R0");
> > > return -EINVAL;
>
