Re: [PATCH net 0/8] net: fix uninit-values in networking stack

[David Miller]

From: Eric Dumazet 
Date: Sun, 8 Apr 2018 09:55:58 -0700

> I also have a report of a WARN() in ip_rt_bug(), added in commit
> c378a9c019cf5e017d1ed24954b54fae7bebd2bc by Dave Jones.
> 
> Not sure what to do, maybe revert, since ip_rt_bug() is not catastrophic.

Let's not do the revert, I wouldn't have seen the backtrace which
points where this bug is if we had.

icmp_route_lookup(), in one branch, does an input route lookup and
uses the result of that to send the icmp message.

That can't be right, input routes should never be used for
transmitting traffice and that's how we end up at ip_rt_bug().

Re: [PATCH net 0/8] net: fix uninit-values in networking stack

[Eric Dumazet]

On 04/08/2018 09:49 AM, David Miller wrote:
> From: Eric Dumazet 
> Date: Sun, 8 Apr 2018 09:38:13 -0700
> 
>> On 04/07/2018 07:40 PM, David Miller wrote:
>>> From: Eric Dumazet 
>>> Date: Sat,  7 Apr 2018 13:42:35 -0700
>>>
 It seems syzbot got new features enabled, and fired some interesting
 reports. Oh well.
>>>
>>> Series applied, however in patch #7 the condition syzbot detects
>>> cannot happen.
>>>
>>> In all code paths that lead to __mkroute_output() with res->type
>>> uninitialized, __mkroute_output() will reassign the local variable
>>> 'type' before reading it.
>>
>> Well, we have :
>>
>> u16 type = res->type;
>> ...
>>
>>if (ipv4_is_lbcast(fl4->daddr))
>> type = RTN_BROADCAST;
>> else if (ipv4_is_multicast(fl4->daddr))
>> type = RTN_MULTICAST;
>> else if (ipv4_is_zeronet(fl4->daddr))
>> return ERR_PTR(-EINVAL);
>>
>> ...
>>
>> if (type == RTN_BROADCAST) {  /* This is where KMSAN complained */
>>
>> So it looks like type could have been random at this point.
> 
> Ok, then.  It seems that the requirement is:
> 
>   fl4->flowi4_oif is non-zero
>   fl4->daddr is neither local multicast nor lbcast
>   fl4->flowi4_proto is IPPROTO_IGMP
> 
> Then we can trigger such a sequence of events.
> 

OK, maybe some more work then ;)


I also have a report of a WARN() in ip_rt_bug(), added in commit 
c378a9c019cf5e017d1ed24954b54fae7bebd2bc
by Dave Jones.

Not sure what to do, maybe revert, since ip_rt_bug() is not catastrophic.

WARNING: CPU: 0 PID: 11678 at net/ipv4/route.c:1213 ip_rt_bug+0x15/0x20 
net/ipv4/route.c:1212
Kernel panic - not syncing: panic_on_warn set ...

CPU: 0 PID: 11678 Comm: kworker/u4:7 Not tainted 4.16.0-rc6+ #289
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x194/0x24d lib/dump_stack.c:53
 panic+0x1e4/0x41c kernel/panic.c:183
 __warn+0x1dc/0x200 kernel/panic.c:547
 report_bug+0x1f4/0x2b0 lib/bug.c:186
 fixup_bug.part.10+0x37/0x80 arch/x86/kernel/traps.c:178
 fixup_bug arch/x86/kernel/traps.c:247 [inline]
 do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
 invalid_op+0x1b/0x40 arch/x86/entry/entry_64.S:986
RIP: 0010:ip_rt_bug+0x15/0x20 net/ipv4/route.c:1212
RSP: 0018:8801db007290 EFLAGS: 00010282
RAX: dc00 RBX: 8801d8dda3c0 RCX: 856c31ca
RDX: 0100 RSI: 8858c300 RDI: 0282
RBP: 8801db007298 R08: 11003b600de1 R09: 
R10:  R11:  R12: 8801d8dda3c0
R13: 88019bdb2200 R14: 88019bdeed80 R15: 8801d8dda418
 dst_output include/net/dst.h:444 [inline]
 ip_local_out+0x95/0x160 net/ipv4/ip_output.c:124
 ip_send_skb+0x3c/0xc0 net/ipv4/ip_output.c:1414
 ip_push_pending_frames+0x64/0x80 net/ipv4/ip_output.c:1434
 icmp_push_reply+0x395/0x4f0 net/ipv4/icmp.c:394
 icmp_send+0x1136/0x19b0 net/ipv4/icmp.c:741
 ipv4_link_failure+0x2a/0x1b0 net/ipv4/route.c:1200
 dst_link_failure include/net/dst.h:427 [inline]
 arp_error_report+0xae/0x180 net/ipv4/arp.c:297
 neigh_invalidate+0x225/0x530 net/core/neighbour.c:883
 neigh_timer_handler+0x897/0xd60 net/core/neighbour.c:969
 call_timer_fn+0x228/0x820 kernel/time/timer.c:1326
 expire_timers kernel/time/timer.c:1363 [inline]
 __run_timers+0x7ee/0xb70 kernel/time/timer.c:1666
 run_timer_softirq+0x4c/0x70 kernel/time/timer.c:1692
 __do_softirq+0x2d7/0xb85 kernel/softirq.c:285
 invoke_softirq kernel/softirq.c:365 [inline]
 irq_exit+0x1cc/0x200 kernel/softirq.c:405
 exiting_irq arch/x86/include/asm/apic.h:541 [inline]
 smp_apic_timer_interrupt+0x16b/0x700 arch/x86/kernel/apic/apic.c:1052
 apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:857
 

Re: [PATCH net 0/8] net: fix uninit-values in networking stack

[David Miller]

From: Eric Dumazet 
Date: Sun, 8 Apr 2018 09:38:13 -0700

> On 04/07/2018 07:40 PM, David Miller wrote:
>> From: Eric Dumazet 
>> Date: Sat,  7 Apr 2018 13:42:35 -0700
>> 
>>> It seems syzbot got new features enabled, and fired some interesting
>>> reports. Oh well.
>> 
>> Series applied, however in patch #7 the condition syzbot detects
>> cannot happen.
>> 
>> In all code paths that lead to __mkroute_output() with res->type
>> uninitialized, __mkroute_output() will reassign the local variable
>> 'type' before reading it.
> 
> Well, we have :
> 
> u16 type = res->type;
> ...
> 
>if (ipv4_is_lbcast(fl4->daddr))
> type = RTN_BROADCAST;
> else if (ipv4_is_multicast(fl4->daddr))
> type = RTN_MULTICAST;
> else if (ipv4_is_zeronet(fl4->daddr))
> return ERR_PTR(-EINVAL);
> 
> ...
> 
> if (type == RTN_BROADCAST) {  /* This is where KMSAN complained */
> 
> So it looks like type could have been random at this point.

Ok, then.  It seems that the requirement is:

fl4->flowi4_oif is non-zero
fl4->daddr is neither local multicast nor lbcast
fl4->flowi4_proto is IPPROTO_IGMP

Then we can trigger such a sequence of events.

Re: [PATCH net 0/8] net: fix uninit-values in networking stack

[Eric Dumazet]

On 04/07/2018 07:40 PM, David Miller wrote:
> From: Eric Dumazet 
> Date: Sat,  7 Apr 2018 13:42:35 -0700
> 
>> It seems syzbot got new features enabled, and fired some interesting
>> reports. Oh well.
> 
> Series applied, however in patch #7 the condition syzbot detects
> cannot happen.
> 
> In all code paths that lead to __mkroute_output() with res->type
> uninitialized, __mkroute_output() will reassign the local variable
> 'type' before reading it.

Well, we have :

u16 type = res->type;
...

   if (ipv4_is_lbcast(fl4->daddr))
type = RTN_BROADCAST;
else if (ipv4_is_multicast(fl4->daddr))
type = RTN_MULTICAST;
else if (ipv4_is_zeronet(fl4->daddr))
return ERR_PTR(-EINVAL);

...

if (type == RTN_BROADCAST) {  /* This is where KMSAN complained */

So it looks like type could have been random at this point.

> 
> Furthermore, by doing a full structure initialization lots of
> unrelated things will be initialized now as well.

fib_result is 40 bytes on 64bit arches.

> 
> We explicitly are only setting up the "inputs" of the fib_result
> object before we call fib_lookup().  The prefixlen and other members
> have no business being initialized there.
> 

Yep

We might put all inputs at the beginning of the structure,
and output at the end. then replace sizeof() by offsetof(),
but this looks a bit convoluted and maybe risky.

Re: [PATCH net 0/8] net: fix uninit-values in networking stack

[David Miller]

From: Eric Dumazet 
Date: Sat,  7 Apr 2018 13:42:35 -0700

> It seems syzbot got new features enabled, and fired some interesting
> reports. Oh well.

Series applied, however in patch #7 the condition syzbot detects
cannot happen.

In all code paths that lead to __mkroute_output() with res->type
uninitialized, __mkroute_output() will reassign the local variable
'type' before reading it.

Furthermore, by doing a full structure initialization lots of
unrelated things will be initialized now as well.

We explicitly are only setting up the "inputs" of the fib_result
object before we call fib_lookup().  The prefixlen and other members
have no business being initialized there.

[PATCH net 0/8] net: fix uninit-values in networking stack

[Eric Dumazet]

It seems syzbot got new features enabled, and fired some interesting
reports. Oh well.

Eric Dumazet (8):
  crypto: af_alg - fix possible uninit-value in alg_bind()
  netlink: fix uninit-value in netlink_sendmsg
  net: fix rtnh_ok()
  net: initialize skb->peeked when cloning
  net: fix uninit-value in __hw_addr_add_ex()
  dccp: initialize ireq->ir_mark
  ipv4: fix uninit-value in ip_route_output_key_hash_rcu()
  soreuseport: initialise timewait reuseport field

 crypto/af_alg.c  |  8 
 include/net/inet_timewait_sock.h |  1 +
 include/net/nexthop.h|  2 +-
 net/core/dev_addr_lists.c|  4 ++--
 net/core/skbuff.c|  1 +
 net/dccp/ipv4.c  |  1 +
 net/dccp/ipv6.c  |  1 +
 net/ipv4/inet_timewait_sock.c|  1 +
 net/ipv4/route.c | 11 ++-
 net/netlink/af_netlink.c |  2 ++
 10 files changed, 20 insertions(+), 12 deletions(-)

-- 
2.17.0.484.g0c8726318c-goog