On Fri, Oct 29, 2021 at 11:29 AM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 10/29/21 9:31 AM, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > Before this fix:
> > 166: (b5) if r2 <= 0x1 goto pc+22
> > from 166 to 189: R2=invP(id=1,umax_value=1,var_off=(0x0; 0xffffffff))
> >
> > After this fix:
> > 166: (b5) if r2 <= 0x1 goto pc+22
> > from 166 to 189: R2=invP(id=1,umax_value=1,var_off=(0x0; 0x1))
> >
> > While processing BPF_JLE the reg_set_min_max() would set true_reg->umax_value = 1
> > and call __reg_combine_64_into_32(true_reg).
> >
> > Without the fix it would not pass the condition:
> > if (__reg64_bound_u32(reg->umin_value) && __reg64_bound_u32(reg->umax_value))
> >
> > since umin_value == 0 at this point.
> > Before commit 10bf4e83167c the umin was incorrectly ingored.
> > The commit 10bf4e83167c fixed the correctness issue, but pessimized
> > propagation of 64-bit min max into 32-bit min max and corresponding var_off.
> >
> > Fixes: 10bf4e83167c ("bpf: Fix propagation of 32 bit unsigned bounds from 64 bit bounds")
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> See an unrelated nits below.
>
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> > ---
> > kernel/bpf/verifier.c | 2 +-
> > tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3c8aa7df1773..29671ed49ee8 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -1425,7 +1425,7 @@ static bool __reg64_bound_s32(s64 a)
>
> We have
> static bool __reg64_bound_s32(s64 a)
> {
> return a > S32_MIN && a < S32_MAX;
> }
>
> Should we change to
> return a >= S32_MIN && a <= S32_MAX
> ?
Probably, but I haven't investigated that yet.
