On Tue, Sep 19, 2023 at 11:59 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Sep 19, 2023 at 2:06 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Mon, Sep 18, 2023 at 2:01 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > >
> > > In mark_chain_precision() logic, when we reach the entry to a global
> > > func, it is expected that R1-R5 might be still requested to be marked
> > > precise. This would correspond to some integer input arguments being
> > > tracked as precise. This is all expected and handled as a special case.
> > >
> > > What's not expected is that we'll leave backtrack_state structure with
> > > some register bits set. This is because for subsequent precision
> > > propagations backtrack_state is reused without clearing masks, as all
> > > code paths are carefully written in a way to leave empty backtrack_state
> > > with zeroed out masks, for speed.
> > >
> > > The fix is trivial, we always clear register bit in the register mask, and
> > > then, optionally, set reg->precise if register is SCALAR_VALUE type.
> > >
> > > Reported-by: Chris Mason <clm@xxxxxxxx>
> > > Fixes: be2ef8161572 ("bpf: allow precision tracking for programs with subprogs")
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > > kernel/bpf/verifier.c | 8 +++-----
> > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index bb78212fa5b2..c0c7d137066a 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -4047,11 +4047,9 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int regno)
> > > bitmap_from_u64(mask, bt_reg_mask(bt));
> > > for_each_set_bit(i, mask, 32) {
> > > reg = &st->frame[0]->regs[i];
> > > - if (reg->type != SCALAR_VALUE) {
> > > - bt_clear_reg(bt, i);
> > > - continue;
> > > - }
> > > - reg->precise = true;
> > > + bt_clear_reg(bt, i);
> > > + if (reg->type == SCALAR_VALUE)
> > > + reg->precise = true;
> >
> > Looks good, but is there a selftest that can demonstrate the issue?
>
> I'll see if I can write something small and reliable.
I give up. It seems like lots of conditions have to come together to
trigger this. In production it was an application that happened to
finish global func validation with that r1 set as precise in
backtrack_state, and then proceeded to have some equivalent state
matched immediately, which triggered propagate_precision() ->
mark_chain_precision_batch(), but doing propagation of r9. Then with
this bug we were looking to propagate r1 and r9, but the code path
under verification didn't have any instruction touching r1 until we
bubbled back up to helper call instruction, where verifier complained
about r1 being required to be precise right after helper call (which
is illegal, as r1-r5 are clobbered).
Few simple tests I tried failed to set up all the necessary conditions
to trigger this in the exact sequence necessary. The fix is simple and
well understood, I'd vote for landing it, given crafting a test is
highly non-trivial.
