BPF List
 help / color / mirror / Atom feed
From: Eduard Zingerman <eddyz87@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Andrew Werner <awerner32@gmail.com>, bpf <bpf@vger.kernel.org>,
	Andrei Matei <andreimatei1@gmail.com>,
	Tamir Duberstein <tamird@gmail.com>,
	Joanne Koong <joannelkoong@gmail.com>,
	kernel-team@dataexmachina.dev, Song Liu <song@kernel.org>,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>
Subject: Re: [BUG] verifier escape with iteration helpers (bpf_loop, ...)
Date: Thu, 21 Sep 2023 19:23:41 +0300	[thread overview]
Message-ID: <4b121c3b96dcc0322ea111062ed2260d2d1d0ed7.camel@gmail.com> (raw)
In-Reply-To: <CAADnVQJO0aVJfV=8RDf5rdtjOCC-=57dmHF20fQYV9EiW2pJ2Q@mail.gmail.com>

On Thu, 2023-09-21 at 05:56 -0700, Alexei Starovoitov wrote:
[...]
> Now I see that asm matches if (likely(r6 != 42)).
> I suspect if you use that in C code you wouldn't need to
> write the test in asm.
> Just a thought.

Thank you this does change test behavior, however compiler still decides
to partially unroll the loop for whatever reason. Will stick to asm
snippets for now.

> Maybe instead of brute forcing all regs to live and precise
> we can add iter.depth check to stacksafe() such
> that depth=0 != depth=1, but
> depth=1 == depthN ?
> (and a tweak to iter_active_depths_differ() as well)
> 
> Then in the above r7 will be 'equivalent', but fp-8 will differ,
> then the state with r7=-32 won't be pruned
> and it will address this particular example ? or not ?

This does help for the particular example, however a simple
modification can still trick the verifier:

     ...
     r6 = bpf_get_current_pid_tgid()
     bpf_iter_num_new(&fp[-8], 0, 10)
+    bpf_iter_num_next(&fp[-8])
     while (bpf_iter_num_next(&fp[-8])) {
       ...
     }
     ...

> Another idea is to add another state.branches specifically for loop body
> and keep iterating the body until branches==0.
> Same concept as the verifier uses for the whole prog, but localized
> to a loop to make sure we don't declare 'equivalent' state
> until all paths in the loop body are explored.

I'm not sure I understand the idea. If we count branches there always
would be back-edges leading to new branches. Or do you suggest to not
prune "equivalent" loop states until all basic blocks in the loop are
visited? (So that all read marks are propagated and probably all
precision marks).

I'm still doubt range_within() check for is_iter_next_insn() case but
can't come up with counter example.

  reply	other threads:[~2023-09-21 17:27 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-07 14:04 [BUG] verifier escape with iteration helpers (bpf_loop, ...) Andrew Werner
2023-07-07 16:44 ` Alexei Starovoitov
2023-07-07 18:08   ` Andrii Nakryiko
2023-07-07 18:21     ` Andrew Werner
2023-09-17 21:37     ` Eduard Zingerman
2023-09-17 22:09       ` Kumar Kartikeya Dwivedi
2023-09-18 13:06         ` Eduard Zingerman
2023-09-19 16:28           ` Eduard Zingerman
2023-09-19 23:02             ` Andrii Nakryiko
2023-09-20  0:19               ` Eduard Zingerman
2023-09-20 16:20                 ` Eduard Zingerman
2023-09-20 16:57                   ` Andrii Nakryiko
2023-09-21  9:14                 ` Alexei Starovoitov
2023-09-21 11:03                   ` Eduard Zingerman
2023-09-21 12:56                     ` Alexei Starovoitov
2023-09-21 16:23                       ` Eduard Zingerman [this message]
2023-09-21 16:35                         ` Andrii Nakryiko
2023-09-21 18:16                           ` Eduard Zingerman
2023-09-22  1:01                             ` Eduard Zingerman
2023-09-22  2:48                               ` Andrii Nakryiko
2023-09-22 18:36                                 ` Eduard Zingerman
2023-09-22 20:52                                   ` Andrii Nakryiko
2023-09-25  1:01                                     ` Eduard Zingerman
2023-09-26  0:33                                       ` Andrii Nakryiko
2023-09-26 15:55                                         ` Eduard Zingerman
2023-09-26 16:25                                           ` Andrii Nakryiko
2023-09-28  1:09                                             ` Eduard Zingerman
2023-09-28 18:30                                               ` Andrii Nakryiko
2023-10-02  3:26                                                 ` Eduard Zingerman
2023-09-30  0:41                                               ` Alexei Starovoitov
2023-10-02  1:40                                                 ` Eduard Zingerman
2023-10-02 16:29                                                   ` Alexei Starovoitov
2023-10-02 17:18                                                     ` Eduard Zingerman
2023-10-03  0:05                                                       ` Alexei Starovoitov
2023-10-03  2:00                                                         ` Alexei Starovoitov
2023-10-03 15:33                                                         ` Eduard Zingerman
2023-10-03 16:07                                                           ` Alexei Starovoitov
2023-10-03 18:50                                                           ` Alexei Starovoitov
2023-10-03 21:52                                                             ` Eduard Zingerman
2023-10-03 22:03                                                               ` Eduard Zingerman
2023-10-03 23:08                                                               ` Alexei Starovoitov
2023-10-03 23:14                                                                 ` Eduard Zingerman
2023-10-04  0:22                                                                   ` Andrii Nakryiko
2023-10-04  1:05                                                                 ` Eduard Zingerman
2023-10-04  2:57                                                                   ` Alexei Starovoitov
2023-10-04  5:50                                                                     ` Alexei Starovoitov
2023-10-04  9:49                                                                       ` Eduard Zingerman
2023-10-04 11:52                                                                     ` Eduard Zingerman
2023-09-19 23:14       ` Andrii Nakryiko
2023-09-20  0:06         ` Eduard Zingerman
2023-09-20 16:37           ` Andrii Nakryiko
2023-09-20 17:13             ` Eduard Zingerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4b121c3b96dcc0322ea111062ed2260d2d1d0ed7.camel@gmail.com \
    --to=eddyz87@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andreimatei1@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=awerner32@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=joannelkoong@gmail.com \
    --cc=kernel-team@dataexmachina.dev \
    --cc=memxor@gmail.com \
    --cc=song@kernel.org \
    --cc=tamird@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox