net: integer overflow in ip_idents_reserve

net: integer overflow in ip_idents_reserve

[Sasha Levin]

Hi Eric,

While fuzzing with trinity on a -next kernel with the undefined behaviour
sanitizer path, I've observed the following warning in code which was
introduced in 04ca6973f7 ("ip: make IP identifiers less predictable"):

[  234.317163] ================================================================================
[  234.320001] UBSan: Undefined behaviour in ./arch/x86/include/asm/atomic.h:157:9
[  234.321568] signed integer overflow:
[  234.322772] 1678406574 + 641542997 cannot be represented in type 'int'
[  234.324316] CPU: 2 PID: 16819 Comm: trinity-c537 Not tainted 3.18.0-next-20141216-sasha-00065-g3c56201-dirty #1609
[  234.326548]  0000000000000000 0000000000000000 ffffffffbc2e4e10 ffff8802e63137e8
[  234.327837]  ffffffffb126bd68 1ffffffff7aa2c03 ffffffffbc2e5c34 ffff8802e6313808
[  234.329117]  ffffffffb126df6f 1ffffffff7aa2c03 ffffffffbc2e5c34 ffff8802e63138c8
[  234.330755] Call Trace:
[  234.331213] dump_stack (lib/dump_stack.c:52)
[  234.332025] ubsan_epilogue (lib/ubsan.c:159)
[  234.332986] handle_overflow (lib/ubsan.c:191)
[  234.334022] ? preempt_schedule (./arch/x86/include/asm/preempt.h:77 (discriminator 1) kernel/sched/core.c:2898 (discriminator 1))
[  234.334945] ? ___preempt_schedule (arch/x86/lib/thunk_64.S:42)
[  234.335919] __ubsan_handle_add_overflow (lib/ubsan.c:200)
[  234.337211] ip_idents_reserve (./arch/x86/include/asm/atomic.h:157 net/ipv4/route.c:482)
[  234.338935] __ip_select_ident (include/uapi/linux/swab.h:49 (discriminator 3) net/ipv4/route.c:498 (discriminator 3))
[  234.340773] __ip_make_skb (include/net/ip.h:339 include/net/ip.h:345 net/ipv4/ip_output.c:1386)
[  234.342736] ip_push_pending_frames (include/net/ip.h:148 net/ipv4/ip_output.c:1430)
[  234.344707] raw_sendmsg (net/ipv4/raw.c:644)
[  234.346537] ? system_call_fastpath (arch/x86/kernel/entry_64.S:423)
[  234.348431] ? get_parent_ip (kernel/sched/core.c:2564)
[  234.350259] ? preempt_count_sub (kernel/sched/core.c:2620)
[  234.352170] inet_sendmsg (net/ipv4/af_inet.c:734)
[  234.354107] do_sock_sendmsg (net/socket.c:646 (discriminator 4))
[  234.355947] ? retint_restore_args (arch/x86/kernel/entry_64.S:844)
[  234.357962] ___sys_sendmsg (net/socket.c:653 net/socket.c:2094)
[  234.359545] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:304)
[  234.361182] ? __acct_update_integrals (kernel/tsacct.c:147)
[  234.363394] ? acct_account_cputime (kernel/tsacct.c:168)
[  234.365417] __sys_sendmsg (net/socket.c:2131)
[  234.367248] SyS_sendmsg (net/socket.c:2136)
[  234.368925] system_call_fastpath (arch/x86/kernel/entry_64.S:423)
[  234.371038] ================================================================================


Thanks,
Sasha

Re: net: integer overflow in ip_idents_reserve

[Eric Dumazet]

On Tue, 2014-12-16 at 16:19 -0500, Sasha Levin wrote:
> Hi Eric,
> 
> While fuzzing with trinity on a -next kernel with the undefined behaviour
> sanitizer path, I've observed the following warning in code which was
> introduced in 04ca6973f7 ("ip: make IP identifiers less predictable"):

This is a false positive.

We don't really care of the value if (now - old) is too big :

No packet was sent recently, so IP ID being X or Y is not a concern.

Re: net: integer overflow in ip_idents_reserve

[Hannes Frederic Sowa]

On Tue, Dec 16, 2014, at 22:47, Eric Dumazet wrote:
> On Tue, 2014-12-16 at 16:19 -0500, Sasha Levin wrote:
> > Hi Eric,
> > 
> > While fuzzing with trinity on a -next kernel with the undefined behaviour
> > sanitizer path, I've observed the following warning in code which was
> > introduced in 04ca6973f7 ("ip: make IP identifiers less predictable"):
> 
> This is a false positive.

Also we compile the whole kernel with -fno-strict-overflow, so every
report of signed overflow leading to undefined behavior is probably a
false positive. I don't know if it is worth to try to get rid of them, I
doubt it.

Bye,
Hannes

Re: net: integer overflow in ip_idents_reserve

[Eric Dumazet]

On Wed, 2014-12-17 at 00:09 +0100, Hannes Frederic Sowa wrote:


> Also we compile the whole kernel with -fno-strict-overflow, so every
> report of signed overflow leading to undefined behavior is probably a
> false positive. I don't know if it is worth to try to get rid of them, I
> doubt it.

Presumably we could have uatomic_t , or atomic_u32_t, whatever...

This particular xadd() is heavily hit in some cases, we really do not
want a cmpxchg()

Re: net: integer overflow in ip_idents_reserve

[Sasha Levin]

On 12/16/2014 06:09 PM, Hannes Frederic Sowa wrote:
> 
> On Tue, Dec 16, 2014, at 22:47, Eric Dumazet wrote:
>> > On Tue, 2014-12-16 at 16:19 -0500, Sasha Levin wrote:
>>> > > Hi Eric,
>>> > > 
>>> > > While fuzzing with trinity on a -next kernel with the undefined behaviour
>>> > > sanitizer path, I've observed the following warning in code which was
>>> > > introduced in 04ca6973f7 ("ip: make IP identifiers less predictable"):
>> > 
>> > This is a false positive.
> Also we compile the whole kernel with -fno-strict-overflow, so every
> report of signed overflow leading to undefined behavior is probably a
> false positive. I don't know if it is worth to try to get rid of them, I
> doubt it.

I reported this one because there's usually some code to handle overflow
in code that expects that and here there was none (I could see).

For example, the ntp code had a few cases where a user could generate
overflows and mess up quite a few things (he got what he asked for -
problems).


Thanks,
Sasha

Re: net: integer overflow in ip_idents_reserve

[Eric Dumazet]

On Tue, 2014-12-16 at 20:15 -0500, Sasha Levin wrote:

> I reported this one because there's usually some code to handle overflow
> in code that expects that and here there was none (I could see).

IP ID are best effort.

When sending one million IPv4 frames per second to a particular
destination, the 16bit ID space is recycled so fast that really their
precise values do not matter anymore.
You pray that IP fragments wont be needed at all.

(One of the idea I had was to detect this kind of stress and fallback to
a random generation, reducing false sharing, but this seemed a micro
optimization targeting synthetic benchmarks )

Thanks