netdev - Re: more test

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, 26 Apr 2017 10:14:42 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <ast@...com>, David Miller <davem@...emloft.net>
CC:     netdev@...r.kernel.org
Subject: Re: more test_progs...

On 04/26/2017 05:42 AM, Alexei Starovoitov wrote:
> On 4/25/17 9:52 AM, David Miller wrote:
>>
>> 10: (15) if r3 == 0xdd86 goto pc+9
>>  R0=imm2,min_value=2,max_value=2 R1=pkt(id=0,off=0,r=14) R2=pkt_end R3=inv R4=pkt(id=0,off=14,r=14) R5=inv56 R10=fp
>>
>> Hmmm, endianness looks wrong here.  "-target bpf" defaults to the
>> endianness of whatever cpu that llvm was built for, right?
>
> yeah. so here the code comes from:
>          } else if (eth->h_proto == _htons(ETH_P_IPV6)) {
>
> and in the beginning of that .c file:
> #define _htons __builtin_bswap16
> ^^^ that's the bug.
> It should have been nop for sparc.
> We cannot use htons() from system header, since it uses x86 inline asm.

Ahh yes, you're right! Wouldn't it be much easier for the above
case to just include <asm/byteorder.h> and use __constant_htons()?
(In fact, all htons() users in this file are constants.)

>>  R0=imm2,min_value=2,max_value=2 R1=inv56,min_value=6,max_value=6 R2=pkt_end R3=imm3,min_value=3,max_value=3 R4=pkt(id=1,off=14,r=34) R5=pkt(id=1,off=34,r=34) R10=fp
>> 36: (07) r4 += 18
>> 37: (2d) if r4 > r2 goto pc+5
>>  R0=imm2,min_value=2,max_value=2 R1=inv56,min_value=6,max_value=6 R2=pkt_end R3=imm3,min_value=3,max_value=3 R4=pkt(id=1,off=32,r=34) R5=pkt(id=1,off=34,r=34) R10=fp
>> 38: (b7) r0 = 0
>> 39: (69) r1 = *(u16 *)(r4 +0)
>> Unknown alignment. Only byte-sized access allowed in packet access.
>>
>> And this seems to load the urgent pointer as a u16 which is what the verifier rejects.
>>
>> Oh I see, this is guarded by CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
>
> yes. exactly.
> Your analysis is correct and offending 16-bit load is this C code:
>                  if (tcp->urg_ptr == 123)
>
> the parsing code before that line deals with variable length ip header:
>                  tcp = (struct tcphdr *)((void *)(iph) + ihl_len);
>
> and at that point verifier cannot track the alignment of the pointer
> anymore. It knows 'iph' alignment, but as soon as some variable is added
> to it cannot assume good alignment anymore and from there on it
> allows >1 byte accesses only on HAVE_EFFICIENT_UNALIGNED_ACCESS
> architectures.
> That's the 'if' that you found:
>   'if (reg->id && size != 1) {'
>
> ref->id > 0 means that some variable was added to ptr_to_packet.
>
> That sucks for sparc, of course. I don't have good suggestions for
> workaround other than marking tcphdr as packed :(
>  From code efficiency point of view it still will be faster than
> LD_ABS insn.

Plus, ld_abs would also only work on skbs right now, and would
need JIT support for XDP. But it's also cumbersome to use with
f.e. IPv6, etc, so should not be encouraged, imo.

One other option for !HAVE_EFFICIENT_UNALIGNED_ACCESS archs could
be to provide a bpf_skb_load_bytes() helper equivalent for XDP,
where you eventually do the memcpy() inside. We could see to inline
the helper itself to optimize it slightly.

> Another idea is to try to track that ihl_len variable is also
> somehow multiple of 4, but it will be quite complicated on
> the verifier side in its existing architecture and maybe? worth
> waiting until the verifier has proper dataflow analysis.