netdev - Re: [PATCH net] net: filter: initialize A and X registers

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]

Date:	Wed, 23 Apr 2014 19:55:09 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	David Miller <davem@...emloft.net>,
	Daniel Borkmann <dborkman@...hat.com>,
	Network Development <netdev@...r.kernel.org>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>
Subject: Re: [PATCH net] net: filter: initialize A and X registers

On Wed, 2014-04-23 at 15:19 -0700, Alexei Starovoitov wrote:

> yes, x86 jit is smart to deal around this problem in an efficient way.
> sparc/arm jits copy pasted the idea correctly.

Well... not really smart, but sufficient enough (even if I am sure we
probably have some bugs)

> but s390 jit seems to have a bug, since it's not clearing A
> when 1st insn is BPF_S_LDX_B_MSH

Sure, JIT is hard and quite risky (You forgot to CC Heiko & Martin, they
probably can send the needed patch)

Its not yet enabled by default for this reason.

I am worried that all recent bpf changes raised the bar so high that
nobody but you can really understand what is going on, unless spending
a _lot_ of time on it.

net/core/filter.c was about 880 lines in previous release, its now 1800+
lines.

netdev became a list discussing compiler technology, 
assembly code and ABI level stuff lately.

Say I want to check why we have this code, potentially modifying
arbitrary kernel memory :

        BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
                atomic_add((u32) X, (atomic_t *)(unsigned long)
                           (A + insn->off));
                CONT;
        BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
                atomic64_add((u64) X, (atomic64_t *)(unsigned long)
                             (A + insn->off));
                CONT;



No comments on this scary code, and I couldn't find why it is there.

$ git grep -n2 BPF_XADD
include/linux/filter.h-19-/* ld/ldx fields */
include/linux/filter.h-20-#define BPF_DW                0x18    /* double word */
include/linux/filter.h:21:#define BPF_XADD      0xc0    /* exclusive add */
include/linux/filter.h-22-
include/linux/filter.h-23-/* alu/jmp fields */
--
net/core/filter.c-222-          DL(BPF_STX, BPF_MEM, BPF_W),
net/core/filter.c-223-          DL(BPF_STX, BPF_MEM, BPF_DW),
net/core/filter.c:224:          DL(BPF_STX, BPF_XADD, BPF_W),
net/core/filter.c:225:          DL(BPF_STX, BPF_XADD, BPF_DW),
net/core/filter.c-226-          DL(BPF_ST, BPF_MEM, BPF_B),
net/core/filter.c-227-          DL(BPF_ST, BPF_MEM, BPF_H),
--
net/core/filter.c-479-  LDST(BPF_DW, u64)
net/core/filter.c-480-#undef LDST
net/core/filter.c:481:  BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
net/core/filter.c-482-          atomic_add((u32) X, (atomic_t *)(unsigned long)
net/core/filter.c-483-                     (A + insn->off));
net/core/filter.c-484-          CONT;
net/core/filter.c:485:  BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
net/core/filter.c-486-          atomic64_add((u64) X, (atomic64_t *)(unsigned long)
net/core/filter.c-487-                       (A + insn->off));

This looks like premature inclusion, at very minimum.

Sure, I probably can find answer reading past months netdev/lkml traffic ?

Sorry if this looks like ranting, but I would rather complain before thousand
of other lines of code are merged.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html