netdev - Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr

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: <20170203225355.GA26227@ast-mbp.thefacebook.com>
Date:   Fri, 3 Feb 2017 14:53:58 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     William Tu <u9012063@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Mihai Budiu <mbudiu@...are.com>
Subject: Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr

On Fri, Feb 03, 2017 at 11:29:19PM +0100, Daniel Borkmann wrote:
> On 02/03/2017 10:10 PM, William Tu wrote:
> >Hi Alexei,
> >
> >why it is bogus? on my system, it fails without the patch applied.
> >
> >--William
> >
> >On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov
> ><alexei.starovoitov@...il.com> wrote:
> >>On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
> >>>The patch fixes the case when adding a zero value to the packet
> >>>pointer.  The verifer reports the following error:
> >>>   [...]
> >>>     R0=imm0,min_value=0,max_value=0
> >>>     R1=pkt(id=0,off=0,r=4)
> >>>     R2=pkt_end R3=fp-12
> >>>     R4=imm4,min_value=4,max_value=4
> >>>     R5=pkt(id=0,off=4,r=4)
> >>>   269: (bf) r2 = r0   // r2 becomes imm0
> >>>   270: (77) r2 >>= 3
> >>>   271: (bf) r4 = r1   // r4 becomes pkt ptr
> >>>   272: (0f) r4 += r2  // r4 += 0
> >>>   addition of negative constant to packet pointer is not allowed
> >>>
> >>>Signed-off-by: William Tu <u9012063@...il.com>
> >>>Signed-off-by: Mihai Budiu <mbudiu@...are.com>
> [...]
> >>>       {
> >>>+             "direct packet access: test14 (pkt_ptr += 0, good access)",
> >>>+             .insns = {
> >>>+                     BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> >>>+                                 offsetof(struct __sk_buff, data)),
> >>>+                     BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> >>>+                                 offsetof(struct __sk_buff, data_end)),
> >>>+                     BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> >>>+                     BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
> >>
> >>wait. the test is bogus.
> >>please write the proper test for the feature
> >>and check that it fails before the patch and passes afterwards.
> 
> But still same code path that is executed in verifier as BPF_K and
> CONST_IMM tracked reg both share the same path under add_imm label
> in check_packet_ptr_add(), no? So it becomes r2=pkt(id=0,off=0,r=0);
> r0 = r2; r0 += 0 here in this test. Probably okay as well, though
> there could be risk that in future both don't share the same path
> for some reason. I guess you were referring to either adding tests
> for BPF_K /and/ CONST_IMM reg or just the latter, right?

yes. Sorry I wasn't clear.
imo the 'r0 += 0' is not something that verifier should recognize,
since such nop insns shouldn't be generated by the compiler.
It happened that the code path in verifier covers that case
as well, but I think we really need to test 'rX += rY' case
where rY is recognized as imm0, since that what the original
use case was about.