Re: [PATCH bpf-next 3/7] bpf: enforce precision for r0 on callback return

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2023-11-09 at 09:50 -0800, Andrii Nakryiko wrote:
> On Thu, Nov 9, 2023 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Thu, 2023-11-09 at 09:32 -0800, Andrii Nakryiko wrote:
> > > On Thu, Nov 9, 2023 at 7:20 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
> > > > 
> > > > 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.
> > 
> > The r0 returned from bpf_loop's callback says bpf_loop to stop iteration,
> > bpf_loop returns the number of completed iterations. However, the return
> > value of bpf_loop modeled by verifier is unbounded scalar.
> > Same for map's for each.
> 
> return value of bpf_loop() is a different thing from return value of
> bpf_loop's callback. Right now bpf_loop implementation in kernel does
> 
> ret = callback(...);
> /* return value: 0 - continue, 1 - stop and return */
> if (ret)
>    return i + 1;
> 
> So yes, it doesn't rely explicitly on return value to be 1 just due to
> the above implementation. But verifier is meant to enforce that and
> the protocol is that bpf_loop and other callback calling helpers
> should rely on this value.
> 
> I think we have the same problem in check_return_code() for entry BPF
> programs. So let me taking this one out of this patch set and post a
> new one concentrating on this particular issue. I've been meaning to
> use umin/umax for return value checking anyways, so might be a good
> idea to do this anyways.

The precision mark is necessary if verifier makes some decisions
basing on the value. E.g. whether certain code path would be take or
whether specific value would be used as a pointer offset.
Neither is true for existing callbacks, value returned by callback
does not affect any verifier decisions.