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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZaGkn9N7pzhLriu5@mail.gmail.com>
Date: Fri, 12 Jan 2024 22:44:15 +0200
From: Maxim Mikityanskiy <maxtram95@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Eduard Zingerman <eddyz87@...il.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Shung-Hsi Yu <shung-hsi.yu@...e.com>,
	John Fastabend <john.fastabend@...il.com>,
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>,
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>,
	Jesper Dangaard Brouer <hawk@...nel.org>, bpf <bpf@...r.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	Maxim Mikityanskiy <maxim@...valent.com>
Subject: Re: [PATCH bpf-next v2 10/15] bpf: Track spilled unbounded scalars

On Fri, 12 Jan 2024 at 11:10:57 -0800, Alexei Starovoitov wrote:
> On Mon, Jan 8, 2024 at 12:53 PM Maxim Mikityanskiy <maxtram95@...il.com> wrote:
> >
> > From: Maxim Mikityanskiy <maxim@...valent.com>
> >
> > Support the pattern where an unbounded scalar is spilled to the stack,
> > then boundary checks are performed on the src register, after which the
> > stack frame slot is refilled into a register.
> >
> > Before this commit, the verifier didn't treat the src register and the
> > stack slot as related if the src register was an unbounded scalar. The
> > register state wasn't copied, the id wasn't preserved, and the stack
> > slot was marked as STACK_MISC. Subsequent boundary checks on the src
> > register wouldn't result in updating the boundaries of the spilled
> > variable on the stack.
> >
> > After this commit, the verifier will preserve the bond between src and
> > dst even if src is unbounded, which permits to do boundary checks on src
> > and refill dst later, still remembering its boundaries. Such a pattern
> > is sometimes generated by clang when compiling complex long functions.
> >
> > One test is adjusted to reflect the fact that an untracked register is
> > marked as precise at an earlier stage, and one more test is adjusted to
> > reflect that now unbounded scalars are tracked.
> >
> > Signed-off-by: Maxim Mikityanskiy <maxim@...valent.com>
> > Acked-by: Eduard Zingerman <eddyz87@...il.com>
> > ---
> >  kernel/bpf/verifier.c                                   | 7 +------
> >  tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 6 +++---
> >  tools/testing/selftests/bpf/verifier/precise.c          | 6 +++---
> >  3 files changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 055fa8096a08..e7fff5f5aa1d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4389,11 +4389,6 @@ static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
> >                reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
> >  }
> >
> > -static bool register_is_bounded(struct bpf_reg_state *reg)
> > -{
> > -       return reg->type == SCALAR_VALUE && !__is_scalar_unbounded(reg);
> > -}
> > -
> >  static bool __is_pointer_value(bool allow_ptr_leaks,
> >                                const struct bpf_reg_state *reg)
> >  {
> > @@ -4504,7 +4499,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> >                 return err;
> >
> >         mark_stack_slot_scratched(env, spi);
> > -       if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
> > +       if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
> >                 bool reg_value_fits;
> >
> >                 reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > index b05aab925ee5..57eb70e100a3 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > @@ -452,9 +452,9 @@ l0_%=:      r1 >>= 16;                                      \
> >  SEC("raw_tp")
> >  __log_level(2)
> >  __success
> > -__msg("fp-8=0m??mmmm")
> > -__msg("fp-16=00mm??mm")
> > -__msg("fp-24=00mm???m")
> > +__msg("fp-8=0m??scalar()")
> > +__msg("fp-16=00mm??scalar()")
> > +__msg("fp-24=00mm???scalar()")
> >  __naked void spill_subregs_preserve_stack_zero(void)
> >  {
> >         asm volatile (
> > diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
> > index 8a2ff81d8350..0a9293a57211 100644
> > --- a/tools/testing/selftests/bpf/verifier/precise.c
> > +++ b/tools/testing/selftests/bpf/verifier/precise.c
> > @@ -183,10 +183,10 @@
> >         .prog_type = BPF_PROG_TYPE_XDP,
> >         .flags = BPF_F_TEST_STATE_FREQ,
> >         .errstr = "mark_precise: frame0: last_idx 7 first_idx 7\
> > -       mark_precise: frame0: parent state regs=r4 stack=:\
> > +       mark_precise: frame0: parent state regs=r4 stack=-8:\
> >         mark_precise: frame0: last_idx 6 first_idx 4\
> > -       mark_precise: frame0: regs=r4 stack= before 6: (b7) r0 = -1\
> > -       mark_precise: frame0: regs=r4 stack= before 5: (79) r4 = *(u64 *)(r10 -8)\
> > +       mark_precise: frame0: regs=r4 stack=-8 before 6: (b7) r0 = -1\
> > +       mark_precise: frame0: regs=r4 stack=-8 before 5: (79) r4 = *(u64 *)(r10 -8)\
> >         mark_precise: frame0: regs= stack=-8 before 4: (7b) *(u64 *)(r3 -8) = r0\
> >         mark_precise: frame0: parent state regs=r0 stack=:\
> >         mark_precise: frame0: last_idx 3 first_idx 3\
> 
> Yesterday I've applied patches 1 through 11 to bpf-next.
> Then Yonghong found that removal of register_is_bounded()
> in this patch 10 makes __is_scalar_unbounded() unused which
> breaks build.
> So I dropped patches 10 and 11.
> 
> Today we found out that test_verifier is broken with patches 1 through 9.
> Turned out that this hunk for verifier/precise.c in patch 10 should have been
> in patch 8.
> I manually took it and force pushed bpf-next again.
> Please test bisectability of the series more carefully in the future.

So sorry I caused this trouble! I indeed mistakenly attributed this hunk
to a wrong patch, must have been more careful =/

> As far as register_is_bounded() issue.
> Maybe order patch 14 that uses __is_scalar_unbounded() first and
> then add this patch 10 ?
> Other ideas?

As for the unused function warning, I thought it wasn't a big deal if I
start using the function again later in the same series. Couldn't
anticipate how it turns out. The idea of having patch 14 in the end of
the series was to show the performance numbers. I'll reorder it before
patch 10, so that we avoid that warning. Sorry again for this mess.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ