netdev - Re: [PATCH bpf-next] bpf: Fix propagation of bounds from 64-bit min/max into 32-bit and var

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Oct 2021 12:22:22 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next] bpf: Fix propagation of bounds from 64-bit
 min/max into 32-bit and var_off.

On Fri, Oct 29, 2021 at 11:29 AM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 10/29/21 9:31 AM, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@...nel.org>
> >
> > 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@...nel.org>
>
> See an unrelated nits below.
>
> Acked-by: Yonghong Song <yhs@...com>
>
> > ---
> >   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.