bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: John Fastabend <john.fastabend@gmail.com>,
	davem@davemloft.net, netdev@vger.kernel.org, bpf@vger.kernel.org,
	kernel-team@fb.com
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: Support for pointers beyond pkt_end.
Date: Fri, 13 Nov 2020 01:50:08 +0100	[thread overview]
Message-ID: <99077ce7-8988-2a63-6663-c282e2007589@iogearbox.net> (raw)
In-Reply-To: <20201113000941.azxyv523bl45z6s5@ast-mbp>

On 11/13/20 1:09 AM, Alexei Starovoitov wrote:
> On Fri, Nov 13, 2020 at 12:56:52AM +0100, Daniel Borkmann wrote:
>> On 11/12/20 8:16 PM, John Fastabend wrote:
>>> Alexei Starovoitov wrote:
>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>
>>>> This patch adds the verifier support to recognize inlined branch conditions.
>>>> The LLVM knows that the branch evaluates to the same value, but the verifier
>>>> couldn't track it. Hence causing valid programs to be rejected.
>>>> The potential LLVM workaround: https://reviews.llvm.org/D87428
>>>> can have undesired side effects, since LLVM doesn't know that
>>>> skb->data/data_end are being compared. LLVM has to introduce extra boolean
>>>> variable and use inline_asm trick to force easier for the verifier assembly.
>>>>
>>>> Instead teach the verifier to recognize that
>>>> r1 = skb->data;
>>>> r1 += 10;
>>>> r2 = skb->data_end;
>>>> if (r1 > r2) {
>>>>     here r1 points beyond packet_end and
>>>>     subsequent
>>>>     if (r1 > r2) // always evaluates to "true".
>>>> }
>>>>
>>>> Tested-by: Jiri Olsa <jolsa@redhat.com>
>>>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>>>> ---
>>>>    include/linux/bpf_verifier.h |   2 +-
>>>>    kernel/bpf/verifier.c        | 129 +++++++++++++++++++++++++++++------
>>>>    2 files changed, 108 insertions(+), 23 deletions(-)
>>>
>>> Thanks, we can remove another set of inline asm logic.
>>>
>>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>>>>    	if (pred >= 0) {
>>>> @@ -7517,7 +7601,8 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>>>>    		 */
>>>>    		if (!__is_pointer_value(false, dst_reg))
>>>>    			err = mark_chain_precision(env, insn->dst_reg);
>>>> -		if (BPF_SRC(insn->code) == BPF_X && !err)
>>>> +		if (BPF_SRC(insn->code) == BPF_X && !err &&
>>>> +		    !__is_pointer_value(false, src_reg))
>>>
>>> This could have been more specific with !type_is_pkt_pointer() correct? I
>>> think its fine as is though.
>>>
>>>>    			err = mark_chain_precision(env, insn->src_reg);
>>>>    		if (err)
>>>>    			return err;
>>
>> Given the reg->range could now be negative, I wonder whether for the regsafe()
>> pruning logic we should now better add a >=0 sanity check in there before we
>> attempt to test on rold->range > rcur->range?
> 
> I thought about it and specifically picked negative range value to keep
> regsafe() check as-is.
> The check is this:
>                  if (rold->range > rcur->range)
>                          return false;
> rold is the one that was safe in the past.
> If rold was positive and the current is negative we fail here
> which is ok. State pruning is conservative.
> 
> If rold was negative it means the previous state was safe even though that pointer
> was pointing beyond packet end. So it's ok for rcur->range to be anything.
> Whether rcur is positive or negative doesn't matter. Everything is still ok.
> If rold->range == -1 and rcur->range == -2 we fail here.
> It's minor annoyance. State pruning is tiny bit more conservative than necessary.
> 
> So I think no extra checks in regsafe() are neeeded.
> Does it make sense?

Yeah, same conclusion here. We still might want to add more BPF asm based tests
on this in general, but either way logic lgtm, so applied, thanks.

  reply	other threads:[~2020-11-13  0:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-11  3:12 [PATCH v2 bpf-next 0/3] bpf: Pointers beyond packet end Alexei Starovoitov
2020-11-11  3:12 ` [PATCH v2 bpf-next 1/3] bpf: Support for pointers beyond pkt_end Alexei Starovoitov
2020-11-12 19:16   ` John Fastabend
2020-11-12 23:56     ` Daniel Borkmann
2020-11-13  0:09       ` Alexei Starovoitov
2020-11-13  0:50         ` Daniel Borkmann [this message]
2020-11-12 23:59     ` Alexei Starovoitov
2020-11-13  0:35       ` John Fastabend
2020-11-11  3:12 ` [PATCH v2 bpf-next 2/3] selftests/bpf: Add skb_pkt_end test Alexei Starovoitov
2020-11-12 19:19   ` John Fastabend
2020-11-11  3:12 ` [PATCH v2 bpf-next 3/3] selftests/bpf: Add asm tests for pkt vs pkt_end comparison Alexei Starovoitov
2020-11-12 19:24   ` John Fastabend
2021-01-20 20:51   ` Jiri Olsa
2020-11-13  0:50 ` [PATCH v2 bpf-next 0/3] bpf: Pointers beyond packet end patchwork-bot+netdevbpf

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=99077ce7-8988-2a63-6663-c282e2007589@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=alexei.starovoitov@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=davem@davemloft.net \
    --cc=john.fastabend@gmail.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /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;
as well as URLs for NNTP newsgroup(s).