Netdev Archive on lore.kernel.org
 help / color / Atom feed
* KMSAN: uninit-value in can_receive
@ 2019-11-18 19:05 syzbot
  2019-11-18 20:25 ` Oliver Hartkopp
  2019-11-26  9:00 ` syzbot
  0 siblings, 2 replies; 17+ messages in thread
From: syzbot @ 2019-11-18 19:05 UTC (permalink / raw)
  To: davem, glider, linux-can, linux-kernel, mkl, netdev, socketcan,
	syzkaller-bugs

Hello,

syzbot found the following crash on:

HEAD commit:    9c6a7162 kmsan: remove unneeded annotations in bio
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
dashboard link: https://syzkaller.appspot.com/bug?extid=b02ff0707a97e4e79ebb
compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
80fee25776c2fb61e74c1ecb1a523375c2500b69)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  <IRQ>
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
  can_receive+0x23c/0x5e0 net/can/af_can.c:649
  can_rcv+0x188/0x3a0 net/can/af_can.c:685
  __netif_receive_skb_one_core net/core/dev.c:5010 [inline]
  __netif_receive_skb net/core/dev.c:5124 [inline]
  process_backlog+0x12e8/0x1410 net/core/dev.c:5955
  napi_poll net/core/dev.c:6392 [inline]
  net_rx_action+0x7a6/0x1aa0 net/core/dev.c:6460
  __do_softirq+0x4a1/0x83a kernel/softirq.c:293
  do_softirq_own_stack+0x49/0x80 arch/x86/entry/entry_64.S:1093
  </IRQ>
  do_softirq kernel/softirq.c:338 [inline]
  __local_bh_enable_ip+0x184/0x1d0 kernel/softirq.c:190
  local_bh_enable+0x36/0x40 include/linux/bottom_half.h:32
  rcu_read_unlock_bh include/linux/rcupdate.h:688 [inline]
  __dev_queue_xmit+0x38e8/0x4200 net/core/dev.c:3900
  dev_queue_xmit+0x4b/0x60 net/core/dev.c:3906
  packet_snd net/packet/af_packet.c:2959 [inline]
  packet_sendmsg+0x82d7/0x92e0 net/packet/af_packet.c:2984
  sock_sendmsg_nosec net/socket.c:637 [inline]
  sock_sendmsg net/socket.c:657 [inline]
  ___sys_sendmsg+0x14ff/0x1590 net/socket.c:2311
  __sys_sendmsg net/socket.c:2356 [inline]
  __do_sys_sendmsg net/socket.c:2365 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2363
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2363
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x45a639
Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ff1b9c14c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 000000000045a639
RDX: 0000000000000050 RSI: 0000000020000100 RDI: 0000000000000003
RBP: 000000000075bfc8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff1b9c156d4
R13: 00000000004c8acf R14: 00000000004df078 R15: 00000000ffffffff

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:151 [inline]
  kmsan_internal_poison_shadow+0x60/0x120 mm/kmsan/kmsan.c:134
  kmsan_slab_alloc+0xaa/0x120 mm/kmsan/kmsan_hooks.c:88
  slab_alloc_node mm/slub.c:2799 [inline]
  __kmalloc_node_track_caller+0xd7b/0x1390 mm/slub.c:4407
  __kmalloc_reserve net/core/skbuff.c:141 [inline]
  __alloc_skb+0x306/0xa10 net/core/skbuff.c:209
  alloc_skb include/linux/skbuff.h:1050 [inline]
  alloc_skb_with_frags+0x18c/0xa80 net/core/skbuff.c:5662
  sock_alloc_send_pskb+0xafd/0x10a0 net/core/sock.c:2244
  packet_alloc_skb net/packet/af_packet.c:2807 [inline]
  packet_snd net/packet/af_packet.c:2902 [inline]
  packet_sendmsg+0x6785/0x92e0 net/packet/af_packet.c:2984
  sock_sendmsg_nosec net/socket.c:637 [inline]
  sock_sendmsg net/socket.c:657 [inline]
  ___sys_sendmsg+0x14ff/0x1590 net/socket.c:2311
  __sys_sendmsg net/socket.c:2356 [inline]
  __do_sys_sendmsg net/socket.c:2365 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2363
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2363
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7
=====================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 19:05 KMSAN: uninit-value in can_receive syzbot
@ 2019-11-18 20:25 ` Oliver Hartkopp
  2019-11-18 20:29   ` Marc Kleine-Budde
  2019-11-26  9:00 ` syzbot
  1 sibling, 1 reply; 17+ messages in thread
From: Oliver Hartkopp @ 2019-11-18 20:25 UTC (permalink / raw)
  To: syzbot, davem, glider, linux-can, linux-kernel, mkl, netdev,
	syzkaller-bugs

On 18/11/2019 20.05, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    9c6a7162 kmsan: remove unneeded annotations in bio
> git tree:       https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
> dashboard link: 
> https://syzkaller.appspot.com/bug?extid=b02ff0707a97e4e79ebb
> compiler:       clang version 9.0.0 (/home/glider/llvm/clang 
> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> 
> Unfortunately, I don't have any reproducer for this crash yet.
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
> 
> =====================================================
> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
> Google 01/01/2011
> Call Trace:
>   <IRQ>
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
>   kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
>   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
>   can_receive+0x23c/0x5e0 net/can/af_can.c:649
>   can_rcv+0x188/0x3a0 net/can/af_can.c:685

In line 649 of 5.4.0-rc5+ we can find a while() statement:

while (!(can_skb_prv(skb)->skbcnt))
	can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);

In linux/include/linux/can/skb.h we see:

static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
{
	return (struct can_skb_priv *)(skb->head);
}

IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid 
operation which has no uninitialized value.

Can this probably be a false positive of KMSAN?

Regards,
Oliver

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 20:25 ` Oliver Hartkopp
@ 2019-11-18 20:29   ` Marc Kleine-Budde
  2019-11-18 20:49     ` Oliver Hartkopp
  0 siblings, 1 reply; 17+ messages in thread
From: Marc Kleine-Budde @ 2019-11-18 20:29 UTC (permalink / raw)
  To: Oliver Hartkopp, syzbot, davem, glider, linux-can, linux-kernel,
	netdev, syzkaller-bugs

[-- Attachment #1.1: Type: text/plain, Size: 3526 bytes --]

On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
> On 18/11/2019 20.05, syzbot wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit:    9c6a7162 kmsan: remove unneeded annotations in bio
>> git tree:       https://github.com/google/kmsan.git master
>> console output: https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
>> dashboard link: 
>> https://syzkaller.appspot.com/bug?extid=b02ff0707a97e4e79ebb
>> compiler:       clang version 9.0.0 (/home/glider/llvm/clang 
>> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
>>
>> =====================================================
>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
>> Google 01/01/2011
>> Call Trace:
>>   <IRQ>
>>   __dump_stack lib/dump_stack.c:77 [inline]
>>   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
>>   kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
>>   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
>>   can_receive+0x23c/0x5e0 net/can/af_can.c:649
>>   can_rcv+0x188/0x3a0 net/can/af_can.c:685
> 
> In line 649 of 5.4.0-rc5+ we can find a while() statement:
> 
> while (!(can_skb_prv(skb)->skbcnt))
> 	can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
> 
> In linux/include/linux/can/skb.h we see:
> 
> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
> {
> 	return (struct can_skb_priv *)(skb->head);
> }
> 
> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid 
> operation which has no uninitialized value.
> 
> Can this probably be a false positive of KMSAN?

The packet is injected via the packet socket into the kernel. Where does
skb->head point to in this case? When the skb is a proper
kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
maybe properly initialized?

>   do_softirq kernel/softirq.c:338 [inline]
>   __local_bh_enable_ip+0x184/0x1d0 kernel/softirq.c:190
>   local_bh_enable+0x36/0x40 include/linux/bottom_half.h:32
>   rcu_read_unlock_bh include/linux/rcupdate.h:688 [inline]
>   __dev_queue_xmit+0x38e8/0x4200 net/core/dev.c:3900
>   dev_queue_xmit+0x4b/0x60 net/core/dev.c:3906
>   packet_snd net/packet/af_packet.c:2959 [inline]
>   packet_sendmsg+0x82d7/0x92e0 net/packet/af_packet.c:2984
                                 ^^^^^^^^^^^^^^^^^^^^^^
>   sock_sendmsg_nosec net/socket.c:637 [inline]
>   sock_sendmsg net/socket.c:657 [inline]
>   ___sys_sendmsg+0x14ff/0x1590 net/socket.c:2311
>   __sys_sendmsg net/socket.c:2356 [inline]
>   __do_sys_sendmsg net/socket.c:2365 [inline]
>   __se_sys_sendmsg+0x305/0x460 net/socket.c:2363
>   __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2363
>   do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
>   entry_SYSCALL_64_after_hwframe+0x63/0xe7

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 20:29   ` Marc Kleine-Budde
@ 2019-11-18 20:49     ` Oliver Hartkopp
  2019-11-18 21:15       ` Marc Kleine-Budde
  0 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp @ 2019-11-18 20:49 UTC (permalink / raw)
  To: Marc Kleine-Budde, syzbot, davem, glider, linux-can,
	linux-kernel, netdev, syzkaller-bugs



On 18/11/2019 21.29, Marc Kleine-Budde wrote:
> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:

>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
>>>
>>> =====================================================
>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0

>>
>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
>>
>> while (!(can_skb_prv(skb)->skbcnt))
>> 	can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
>>
>> In linux/include/linux/can/skb.h we see:
>>
>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>> {
>> 	return (struct can_skb_priv *)(skb->head);
>> }
>>
>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
>> operation which has no uninitialized value.
>>
>> Can this probably be a false positive of KMSAN?
> 
> The packet is injected via the packet socket into the kernel. Where does
> skb->head point to in this case? When the skb is a proper
> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
> maybe properly initialized?

The packet is either received via vcan or vxcan which checks via 
can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.

We additionally might think about introducing a check whether we have a 
can_skb_reserve() created skbuff.

But even if someone forged a skbuff without this reserved space the 
access to can_skb_prv(skb)->skbcnt would point into some CAN frame 
content - which is still no access to uninitialized content, right?

Regards,
Oliver

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 20:49     ` Oliver Hartkopp
@ 2019-11-18 21:15       ` Marc Kleine-Budde
  2019-11-19  7:35         ` Oliver Hartkopp
  0 siblings, 1 reply; 17+ messages in thread
From: Marc Kleine-Budde @ 2019-11-18 21:15 UTC (permalink / raw)
  To: Oliver Hartkopp, syzbot, davem, glider, linux-can, linux-kernel,
	netdev, syzkaller-bugs

[-- Attachment #1.1: Type: text/plain, Size: 2188 bytes --]

On 11/18/19 9:49 PM, Oliver Hartkopp wrote:
> 
> 
> On 18/11/2019 21.29, Marc Kleine-Budde wrote:
>> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
> 
>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
>>>>
>>>> =====================================================
>>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
>>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
> 
>>>
>>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
>>>
>>> while (!(can_skb_prv(skb)->skbcnt))
>>> 	can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
>>>
>>> In linux/include/linux/can/skb.h we see:
>>>
>>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>>> {
>>> 	return (struct can_skb_priv *)(skb->head);
>>> }
>>>
>>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
>>> operation which has no uninitialized value.
>>>
>>> Can this probably be a false positive of KMSAN?
>>
>> The packet is injected via the packet socket into the kernel. Where does
>> skb->head point to in this case? When the skb is a proper
>> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
>> maybe properly initialized?
> 
> The packet is either received via vcan or vxcan which checks via 
> can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.

According to the call stack it's injected into the kernel via a packet
socket and not via v(x)can.

> We additionally might think about introducing a check whether we have a 
> can_skb_reserve() created skbuff.
> 
> But even if someone forged a skbuff without this reserved space the 
> access to can_skb_prv(skb)->skbcnt would point into some CAN frame 
> content - which is still no access to uninitialized content, right?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 21:15       ` Marc Kleine-Budde
@ 2019-11-19  7:35         ` Oliver Hartkopp
  2019-11-19  9:00           ` Oleksij Rempel
                             ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Oliver Hartkopp @ 2019-11-19  7:35 UTC (permalink / raw)
  To: Marc Kleine-Budde, syzbot, davem, glider, linux-can,
	linux-kernel, netdev, syzkaller-bugs



On 18/11/2019 22.15, Marc Kleine-Budde wrote:
> On 11/18/19 9:49 PM, Oliver Hartkopp wrote:
>>
>>
>> On 18/11/2019 21.29, Marc Kleine-Budde wrote:
>>> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
>>
>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
>>>>>
>>>>> =====================================================
>>>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
>>>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
>>
>>>>
>>>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
>>>>
>>>> while (!(can_skb_prv(skb)->skbcnt))
>>>> 	can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
>>>>
>>>> In linux/include/linux/can/skb.h we see:
>>>>
>>>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>>>> {
>>>> 	return (struct can_skb_priv *)(skb->head);
>>>> }
>>>>
>>>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
>>>> operation which has no uninitialized value.
>>>>
>>>> Can this probably be a false positive of KMSAN?
>>>
>>> The packet is injected via the packet socket into the kernel. Where does
>>> skb->head point to in this case? When the skb is a proper
>>> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
>>> maybe properly initialized?
>>
>> The packet is either received via vcan or vxcan which checks via
>> can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.
> 
> According to the call stack it's injected into the kernel via a packet
> socket and not via v(x)can.

See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000

23:11:34 executing program 2:
r0 = socket(0x200000000000011, 0x3, 0x0)
ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933, 
&(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)

We only can receive skbs from (v(x))can devices.
No matter if someone wrote to them via PF_CAN or PF_PACKET.
We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.

>> We additionally might think about introducing a check whether we have a
>> can_skb_reserve() created skbuff.
>>
>> But even if someone forged a skbuff without this reserved space the
>> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
>> content - which is still no access to uninitialized content, right?

So this question remains still valid whether we have a false positive 
from KMSAN here.

Regards,
Oliver


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19  7:35         ` Oliver Hartkopp
@ 2019-11-19  9:00           ` Oleksij Rempel
  2019-11-19 10:08           ` Dmitry Vyukov
  2019-11-19 16:53           ` Eric Dumazet
  2 siblings, 0 replies; 17+ messages in thread
From: Oleksij Rempel @ 2019-11-19  9:00 UTC (permalink / raw)
  To: Oliver Hartkopp, Marc Kleine-Budde, syzbot, davem, glider,
	linux-can, linux-kernel, netdev, syzkaller-bugs

Hi,

On 19.11.19 08:35, Oliver Hartkopp wrote:
> 
> 
> On 18/11/2019 22.15, Marc Kleine-Budde wrote:
>> On 11/18/19 9:49 PM, Oliver Hartkopp wrote:
>>>
>>>
>>> On 18/11/2019 21.29, Marc Kleine-Budde wrote:
>>>> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
>>>
>>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
>>>>>>
>>>>>> =====================================================
>>>>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
>>>>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
>>>
>>>>>
>>>>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
>>>>>
>>>>> while (!(can_skb_prv(skb)->skbcnt))
>>>>>     can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
>>>>>
>>>>> In linux/include/linux/can/skb.h we see:
>>>>>
>>>>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>>>>> {
>>>>>     return (struct can_skb_priv *)(skb->head);
>>>>> }
>>>>>
>>>>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
>>>>> operation which has no uninitialized value.
>>>>>
>>>>> Can this probably be a false positive of KMSAN?
>>>>
>>>> The packet is injected via the packet socket into the kernel. Where does
>>>> skb->head point to in this case? When the skb is a proper
>>>> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
>>>> maybe properly initialized?
>>>
>>> The packet is either received via vcan or vxcan which checks via
>>> can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.
>>
>> According to the call stack it's injected into the kernel via a packet
>> socket and not via v(x)can.
> 
> See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
> 
> 23:11:34 executing program 2:
> r0 = socket(0x200000000000011, 0x3, 0x0)
> ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933, &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
> bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
> sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
> 
> We only can receive skbs from (v(x))can devices.
> No matter if someone wrote to them via PF_CAN or PF_PACKET.
> We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.
> 
>>> We additionally might think about introducing a check whether we have a
>>> can_skb_reserve() created skbuff.
>>>
>>> But even if someone forged a skbuff without this reserved space the
>>> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
>>> content - which is still no access to uninitialized content, right?
> 
> So this question remains still valid whether we have a false positive from KMSAN here.

It can be other incornation of this bug:
https://github.com/linux-can/linux/issues/1

The echo skd was free, because socket which send this skb was closed before it was received.

Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19  7:35         ` Oliver Hartkopp
  2019-11-19  9:00           ` Oleksij Rempel
@ 2019-11-19 10:08           ` Dmitry Vyukov
  2019-11-19 13:06             ` Alexander Potapenko
  2019-11-19 16:53           ` Eric Dumazet
  2 siblings, 1 reply; 17+ messages in thread
From: Dmitry Vyukov @ 2019-11-19 10:08 UTC (permalink / raw)
  To: Oliver Hartkopp, Alexander Potapenko
  Cc: Marc Kleine-Budde, syzbot, David Miller, linux-can, LKML, netdev,
	syzkaller-bugs

On Tue, Nov 19, 2019 at 8:36 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 18/11/2019 22.15, Marc Kleine-Budde wrote:
> > On 11/18/19 9:49 PM, Oliver Hartkopp wrote:
> >>
> >>
> >> On 18/11/2019 21.29, Marc Kleine-Budde wrote:
> >>> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
> >>
> >>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >>>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
> >>>>>
> >>>>> =====================================================
> >>>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
> >>>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
> >>
> >>>>
> >>>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
> >>>>
> >>>> while (!(can_skb_prv(skb)->skbcnt))
> >>>>    can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
> >>>>
> >>>> In linux/include/linux/can/skb.h we see:
> >>>>
> >>>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
> >>>> {
> >>>>    return (struct can_skb_priv *)(skb->head);
> >>>> }
> >>>>
> >>>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
> >>>> operation which has no uninitialized value.
> >>>>
> >>>> Can this probably be a false positive of KMSAN?
> >>>
> >>> The packet is injected via the packet socket into the kernel. Where does
> >>> skb->head point to in this case? When the skb is a proper
> >>> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
> >>> maybe properly initialized?
> >>
> >> The packet is either received via vcan or vxcan which checks via
> >> can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.
> >
> > According to the call stack it's injected into the kernel via a packet
> > socket and not via v(x)can.
>
> See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
>
> 23:11:34 executing program 2:
> r0 = socket(0x200000000000011, 0x3, 0x0)
> ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933,
> &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
> bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
> sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
>
> We only can receive skbs from (v(x))can devices.
> No matter if someone wrote to them via PF_CAN or PF_PACKET.
> We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.
>
> >> We additionally might think about introducing a check whether we have a
> >> can_skb_reserve() created skbuff.
> >>
> >> But even if someone forged a skbuff without this reserved space the
> >> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
> >> content - which is still no access to uninitialized content, right?
>
> So this question remains still valid whether we have a false positive
> from KMSAN here.

+Alex, please check re KMSAN false positive.
Oliver, Marc, where this skbcnt should have been initialized in this case?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19 10:08           ` Dmitry Vyukov
@ 2019-11-19 13:06             ` Alexander Potapenko
  0 siblings, 0 replies; 17+ messages in thread
From: Alexander Potapenko @ 2019-11-19 13:06 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Oliver Hartkopp, Marc Kleine-Budde, syzbot, David Miller,
	linux-can, LKML, netdev, syzkaller-bugs

On Tue, Nov 19, 2019 at 11:09 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Tue, Nov 19, 2019 at 8:36 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > On 18/11/2019 22.15, Marc Kleine-Budde wrote:
> > > On 11/18/19 9:49 PM, Oliver Hartkopp wrote:
> > >>
> > >>
> > >> On 18/11/2019 21.29, Marc Kleine-Budde wrote:
> > >>> On 11/18/19 9:25 PM, Oliver Hartkopp wrote:
> > >>
> > >>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > >>>>> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
> > >>>>>
> > >>>>> =====================================================
> > >>>>> BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:649
> > >>>>> CPU: 1 PID: 3490 Comm: syz-executor.2 Not tainted 5.4.0-rc5+ #0
> > >>
> > >>>>
> > >>>> In line 649 of 5.4.0-rc5+ we can find a while() statement:
> > >>>>
> > >>>> while (!(can_skb_prv(skb)->skbcnt))
> > >>>>    can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
> > >>>>
> > >>>> In linux/include/linux/can/skb.h we see:
> > >>>>
> > >>>> static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
> > >>>> {
> > >>>>    return (struct can_skb_priv *)(skb->head);
> > >>>> }
> > >>>>
> > >>>> IMO accessing can_skb_prv(skb)->skbcnt at this point is a valid
> > >>>> operation which has no uninitialized value.
> > >>>>
> > >>>> Can this probably be a false positive of KMSAN?
> > >>>
> > >>> The packet is injected via the packet socket into the kernel. Where does
> > >>> skb->head point to in this case? When the skb is a proper
> > >>> kernel-generated skb containing a CAN-2.0 or CAN-FD frame skb->head is
> > >>> maybe properly initialized?
> > >>
> > >> The packet is either received via vcan or vxcan which checks via
> > >> can_dropped_invalid_skb() if we have a valid ETH_P_CAN type skb.
> > >
> > > According to the call stack it's injected into the kernel via a packet
> > > socket and not via v(x)can.
> >
> > See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
> >
> > 23:11:34 executing program 2:
> > r0 = socket(0x200000000000011, 0x3, 0x0)
> > ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933,
> > &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
> > bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
> > sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
> >
> > We only can receive skbs from (v(x))can devices.
> > No matter if someone wrote to them via PF_CAN or PF_PACKET.
> > We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.
> >
> > >> We additionally might think about introducing a check whether we have a
> > >> can_skb_reserve() created skbuff.
> > >>
> > >> But even if someone forged a skbuff without this reserved space the
> > >> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
> > >> content - which is still no access to uninitialized content, right?
> >
> > So this question remains still valid whether we have a false positive
> > from KMSAN here.
>
> +Alex, please check re KMSAN false positive.
Unfortunately syzbot didn't give a repro for this bug. I've tried
replaying the log, but it didn't work (or maybe the bug is fixed
already).
> Oliver, Marc, where this skbcnt should have been initialized in this case?



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19  7:35         ` Oliver Hartkopp
  2019-11-19  9:00           ` Oleksij Rempel
  2019-11-19 10:08           ` Dmitry Vyukov
@ 2019-11-19 16:53           ` Eric Dumazet
  2019-11-19 20:24             ` Oliver Hartkopp
  2 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2019-11-19 16:53 UTC (permalink / raw)
  To: Oliver Hartkopp, Marc Kleine-Budde, syzbot, davem, glider,
	linux-can, linux-kernel, netdev, syzkaller-bugs



On 11/18/19 11:35 PM, Oliver Hartkopp wrote:
> 

> 
> See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
> 
> 23:11:34 executing program 2:
> r0 = socket(0x200000000000011, 0x3, 0x0)
> ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933, &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
> bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
> sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
> 
> We only can receive skbs from (v(x))can devices.
> No matter if someone wrote to them via PF_CAN or PF_PACKET.
> We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.

And what entity sets the can_skb_prv(skb)->skbcnt to zero exactly ?

> 
>>> We additionally might think about introducing a check whether we have a
>>> can_skb_reserve() created skbuff.
>>>
>>> But even if someone forged a skbuff without this reserved space the
>>> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
>>> content - which is still no access to uninitialized content, right?
> 
> So this question remains still valid whether we have a false positive from KMSAN here.

I do not believe it is a false positive.

It seems CAN relies on some properties of low level drivers using alloc_can_skb() or similar function.

Why not simply fix this like that ?

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 128d37a4c2e0ba5d8db69fcceec8cbd6a79380df..3e71a78d82af84caaacd0ef512b5e894efbf4852 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -647,8 +647,9 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
        pkg_stats->rx_frames_delta++;
 
        /* create non-zero unique skb identifier together with *skb */
-       while (!(can_skb_prv(skb)->skbcnt))
+       do {
                can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
+       } while (!(can_skb_prv(skb)->skbcnt));
 
        rcu_read_lock();
 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19 16:53           ` Eric Dumazet
@ 2019-11-19 20:24             ` Oliver Hartkopp
  2019-11-19 21:09               ` Eric Dumazet
  0 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp @ 2019-11-19 20:24 UTC (permalink / raw)
  To: Eric Dumazet, Marc Kleine-Budde, syzbot, davem, glider,
	linux-can, linux-kernel, netdev, syzkaller-bugs

Hi Eric,

On 19/11/2019 17.53, Eric Dumazet wrote:
> 
> 
> On 11/18/19 11:35 PM, Oliver Hartkopp wrote:
>>
> 
>>
>> See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
>>
>> 23:11:34 executing program 2:
>> r0 = socket(0x200000000000011, 0x3, 0x0)
>> ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933, &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
>> bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
>> sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
>>
>> We only can receive skbs from (v(x))can devices.
>> No matter if someone wrote to them via PF_CAN or PF_PACKET.
>> We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.
> 
> And what entity sets the can_skb_prv(skb)->skbcnt to zero exactly ?
> 
>>
>>>> We additionally might think about introducing a check whether we have a
>>>> can_skb_reserve() created skbuff.
>>>>
>>>> But even if someone forged a skbuff without this reserved space the
>>>> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
>>>> content - which is still no access to uninitialized content, right?
>>
>> So this question remains still valid whether we have a false positive from KMSAN here.
> 
> I do not believe it is a false positive.
> 
> It seems CAN relies on some properties of low level drivers using alloc_can_skb() or similar function.
> 
> Why not simply fix this like that ?
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 128d37a4c2e0ba5d8db69fcceec8cbd6a79380df..3e71a78d82af84caaacd0ef512b5e894efbf4852 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -647,8 +647,9 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>          pkg_stats->rx_frames_delta++;
>   
>          /* create non-zero unique skb identifier together with *skb */
> -       while (!(can_skb_prv(skb)->skbcnt))
> +       do {
>                  can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
> +       } while (!(can_skb_prv(skb)->skbcnt));
>   
>          rcu_read_lock();
>   

Please check commit d3b58c47d330d ("can: replace timestamp as unique skb 
attribute").

can_skb_prv(skb)->skbcnt is set to 0 at skb creation time when sending 
CAN frames from local host or receiving CAN frames from a real CAN 
interface.

When a CAN skb is received by the net layer the *first* time it gets a 
unique value which we need for a per-cpu filter mechanism in raw_rcv().

So where's the problem to check for (!(can_skb_prv(skb)->skbcnt)) in a 
while statement? I can't see a chance for an uninitialized value there.

Regards,
Oliver

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19 20:24             ` Oliver Hartkopp
@ 2019-11-19 21:09               ` Eric Dumazet
  2019-11-20 20:10                 ` Oliver Hartkopp
  0 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2019-11-19 21:09 UTC (permalink / raw)
  To: Oliver Hartkopp, Eric Dumazet, Marc Kleine-Budde, syzbot, davem,
	glider, linux-can, linux-kernel, netdev, syzkaller-bugs



On 11/19/19 12:24 PM, Oliver Hartkopp wrote:
> Hi Eric,
> 
> On 19/11/2019 17.53, Eric Dumazet wrote:
>>
>>
>> On 11/18/19 11:35 PM, Oliver Hartkopp wrote:
>>>
>>
>>>
>>> See ioctl$ifreq https://syzkaller.appspot.com/x/log.txt?x=14563416e00000
>>>
>>> 23:11:34 executing program 2:
>>> r0 = socket(0x200000000000011, 0x3, 0x0)
>>> ioctl$ifreq_SIOCGIFINDEX_vcan(r0, 0x8933, &(0x7f0000000040)={'vxcan1\x00', <r1=>0x0})
>>> bind$packet(r0, &(0x7f0000000300)={0x11, 0xc, r1}, 0x14)
>>> sendmmsg(r0, &(0x7f0000000d00), 0x400004e, 0x0)
>>>
>>> We only can receive skbs from (v(x))can devices.
>>> No matter if someone wrote to them via PF_CAN or PF_PACKET.
>>> We check for ETH_P_CAN(FD) type and ARPHRD_CAN dev type at rx time.
>>
>> And what entity sets the can_skb_prv(skb)->skbcnt to zero exactly ?
>>
>>>
>>>>> We additionally might think about introducing a check whether we have a
>>>>> can_skb_reserve() created skbuff.
>>>>>
>>>>> But even if someone forged a skbuff without this reserved space the
>>>>> access to can_skb_prv(skb)->skbcnt would point into some CAN frame
>>>>> content - which is still no access to uninitialized content, right?
>>>
>>> So this question remains still valid whether we have a false positive from KMSAN here.
>>
>> I do not believe it is a false positive.
>>
>> It seems CAN relies on some properties of low level drivers using alloc_can_skb() or similar function.
>>
>> Why not simply fix this like that ?
>>
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 128d37a4c2e0ba5d8db69fcceec8cbd6a79380df..3e71a78d82af84caaacd0ef512b5e894efbf4852 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -647,8 +647,9 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>>          pkg_stats->rx_frames_delta++;
>>            /* create non-zero unique skb identifier together with *skb */
>> -       while (!(can_skb_prv(skb)->skbcnt))
>> +       do {
>>                  can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
>> +       } while (!(can_skb_prv(skb)->skbcnt));
>>            rcu_read_lock();
>>   
> 
> Please check commit d3b58c47d330d ("can: replace timestamp as unique skb attribute").

Oh well... This notion of 'unique skb attribute' is interesting...

> 
> can_skb_prv(skb)->skbcnt is set to 0 at skb creation time when sending CAN frames from local host or receiving CAN frames from a real CAN interface.

We can not enforce this to happen with a virtual interface.

> 
> When a CAN skb is received by the net layer the *first* time it gets a unique value which we need for a per-cpu filter mechanism in raw_rcv().
> 
> So where's the problem to check for (!(can_skb_prv(skb)->skbcnt)) in a while statement? I can't see a chance for an uninitialized value there.

You have to make sure the packet has been properly cooked by a 'real CAN interface' then, and reject them if not.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-19 21:09               ` Eric Dumazet
@ 2019-11-20 20:10                 ` Oliver Hartkopp
  2019-12-03 10:09                   ` Marc Kleine-Budde
  0 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp @ 2019-11-20 20:10 UTC (permalink / raw)
  To: Eric Dumazet, Marc Kleine-Budde, syzbot, davem, glider,
	linux-can, linux-kernel, netdev, syzkaller-bugs

On 19/11/2019 22.09, Eric Dumazet wrote:
> On 11/19/19 12:24 PM, Oliver Hartkopp wrote:
>> Please check commit d3b58c47d330d ("can: replace timestamp as unique skb attribute").
> 
> Oh well... This notion of 'unique skb attribute' is interesting...

Yes. The problem is that the joined filter needs to detect the identical 
skb which is delivered several times to raw_rcv() to process filters 
that are logical ANDed.

>> can_skb_prv(skb)->skbcnt is set to 0 at skb creation time when sending CAN frames from local host or receiving CAN frames from a real CAN interface.
> 
> We can not enforce this to happen with a virtual interface.

You are right. I just discovered that I'm not able to send CAN frames 
via PF_PACKET sockets anymore.

Receiving with a simple test program and Wireshark is fine - but sending 
does not work. PF_PACKET is not creating the same kind of skbs as e.g. 
the CAN_RAW socket does.

So the KMSAN detection was right at the end :-(

I'll take a closer look to enable PF_PACKET to send CAN frames again 
which will fix up the entire  problem.

Thanks for your feedback!

Best,
Oliver

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-18 19:05 KMSAN: uninit-value in can_receive syzbot
  2019-11-18 20:25 ` Oliver Hartkopp
@ 2019-11-26  9:00 ` syzbot
  1 sibling, 0 replies; 17+ messages in thread
From: syzbot @ 2019-11-26  9:00 UTC (permalink / raw)
  To: davem, dvyukov, eric.dumazet, glider, linux-can, linux-kernel,
	mkl, netdev, o.rempel, socketcan, syzkaller-bugs

syzbot has found a reproducer for the following crash on:

HEAD commit:    4a1d41e3 net: kasan: kmsan: support CONFIG_GENERIC_CSUM on..
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1632f75ae00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=fde150fb1e865232
dashboard link: https://syzkaller.appspot.com/bug?extid=b02ff0707a97e4e79ebb
compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15696e36e00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=132b3636e00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in can_receive+0x23c/0x5e0 net/can/af_can.c:650
CPU: 0 PID: 11833 Comm: syz-executor463 Not tainted 5.4.0-rc8-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  <IRQ>
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1c9/0x220 lib/dump_stack.c:118
  kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x64/0xc0 mm/kmsan/kmsan_instr.c:245
  can_receive+0x23c/0x5e0 net/can/af_can.c:650
  canfd_rcv+0x188/0x3a0 net/can/af_can.c:703
  __netif_receive_skb_one_core net/core/dev.c:4929 [inline]
  __netif_receive_skb net/core/dev.c:5043 [inline]
  process_backlog+0x12a6/0x13c0 net/core/dev.c:5874
  napi_poll net/core/dev.c:6311 [inline]
  net_rx_action+0x7a6/0x1aa0 net/core/dev.c:6379
  __do_softirq+0x4a1/0x83a kernel/softirq.c:293
  do_softirq_own_stack+0x49/0x80 arch/x86/entry/entry_64.S:1091
  </IRQ>
  do_softirq kernel/softirq.c:338 [inline]
  __local_bh_enable_ip+0x184/0x1d0 kernel/softirq.c:190
  local_bh_enable+0x36/0x40 include/linux/bottom_half.h:32
  rcu_read_unlock_bh include/linux/rcupdate.h:688 [inline]
  __dev_queue_xmit+0x38e8/0x4200 net/core/dev.c:3819
  dev_queue_xmit+0x4b/0x60 net/core/dev.c:3825
  packet_snd net/packet/af_packet.c:2959 [inline]
  packet_sendmsg+0x8234/0x9100 net/packet/af_packet.c:2984
  sock_sendmsg_nosec net/socket.c:637 [inline]
  sock_sendmsg net/socket.c:657 [inline]
  ___sys_sendmsg+0x14ff/0x1590 net/socket.c:2311
  __sys_sendmmsg+0x53a/0xae0 net/socket.c:2413
  __do_sys_sendmmsg net/socket.c:2442 [inline]
  __se_sys_sendmmsg+0xbd/0xe0 net/socket.c:2439
  __x64_sys_sendmmsg+0x56/0x70 net/socket.c:2439
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x442129
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 5b 10 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fffef5083a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000442129
RDX: 000000000400004e RSI: 0000000020000d00 RDI: 0000000000000003
RBP: 0000000000000004 R08: 0000000000000025 R09: 0000000000000025
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00000000004036a0 R14: 0000000000000000 R15: 0000000000000000

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:149 [inline]
  kmsan_internal_poison_shadow+0x60/0x120 mm/kmsan/kmsan.c:132
  kmsan_slab_alloc+0x97/0x100 mm/kmsan/kmsan_hooks.c:86
  slab_alloc_node mm/slub.c:2773 [inline]
  __kmalloc_node_track_caller+0xe27/0x11a0 mm/slub.c:4381
  __kmalloc_reserve net/core/skbuff.c:141 [inline]
  __alloc_skb+0x306/0xa10 net/core/skbuff.c:209
  alloc_skb include/linux/skbuff.h:1049 [inline]
  alloc_skb_with_frags+0x18c/0xa80 net/core/skbuff.c:5662
  sock_alloc_send_pskb+0xafd/0x10a0 net/core/sock.c:2244
  packet_alloc_skb net/packet/af_packet.c:2807 [inline]
  packet_snd net/packet/af_packet.c:2902 [inline]
  packet_sendmsg+0x63a6/0x9100 net/packet/af_packet.c:2984
  sock_sendmsg_nosec net/socket.c:637 [inline]
  sock_sendmsg net/socket.c:657 [inline]
  ___sys_sendmsg+0x14ff/0x1590 net/socket.c:2311
  __sys_sendmmsg+0x53a/0xae0 net/socket.c:2413
  __do_sys_sendmmsg net/socket.c:2442 [inline]
  __se_sys_sendmmsg+0xbd/0xe0 net/socket.c:2439
  __x64_sys_sendmmsg+0x56/0x70 net/socket.c:2439
  do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
=====================================================


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-11-20 20:10                 ` Oliver Hartkopp
@ 2019-12-03 10:09                   ` Marc Kleine-Budde
  2019-12-03 10:37                     ` Oliver Hartkopp
  0 siblings, 1 reply; 17+ messages in thread
From: Marc Kleine-Budde @ 2019-12-03 10:09 UTC (permalink / raw)
  To: Oliver Hartkopp, Eric Dumazet, syzbot, davem, glider, linux-can,
	linux-kernel, netdev, syzkaller-bugs

[-- Attachment #1.1: Type: text/plain, Size: 592 bytes --]

On 11/20/19 9:10 PM, Oliver Hartkopp wrote:
[...]
> So the KMSAN detection was right at the end :-(
> 
> I'll take a closer look to enable PF_PACKET to send CAN frames again 
> which will fix up the entire  problem.

I'm going to send a pull request today. Do you already have a fix for this?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-12-03 10:09                   ` Marc Kleine-Budde
@ 2019-12-03 10:37                     ` Oliver Hartkopp
  2019-12-03 10:40                       ` Marc Kleine-Budde
  0 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp @ 2019-12-03 10:37 UTC (permalink / raw)
  To: Marc Kleine-Budde, Eric Dumazet, syzbot, davem, glider,
	linux-can, linux-kernel, netdev, syzkaller-bugs

No. I have analyzed several solutions which turn out to be either unsafe 
in processing or need some changes in af_packet :-(

I'm currently very busy @work but will come up with a discussion until 
end of this week.

There is no big pressure as the problem is more unpleasant than causing 
a real problem right now.

Best regards,
Oliver

On 03/12/2019 11.09, Marc Kleine-Budde wrote:
> On 11/20/19 9:10 PM, Oliver Hartkopp wrote:
> [...]
>> So the KMSAN detection was right at the end :-(
>>
>> I'll take a closer look to enable PF_PACKET to send CAN frames again
>> which will fix up the entire  problem.
> 
> I'm going to send a pull request today. Do you already have a fix for this?
> 
> regards,
> Marc
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: KMSAN: uninit-value in can_receive
  2019-12-03 10:37                     ` Oliver Hartkopp
@ 2019-12-03 10:40                       ` Marc Kleine-Budde
  0 siblings, 0 replies; 17+ messages in thread
From: Marc Kleine-Budde @ 2019-12-03 10:40 UTC (permalink / raw)
  To: Oliver Hartkopp, Eric Dumazet, syzbot, davem, glider, linux-can,
	linux-kernel, netdev, syzkaller-bugs

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

Hey Oliver,

On 12/3/19 11:37 AM, Oliver Hartkopp wrote:
> No. I have analyzed several solutions which turn out to be either unsafe 
> in processing or need some changes in af_packet :-(
> 
> I'm currently very busy @work

I know this problem :/
Thanks for your quick feedback, although your busy.

> but will come up with a discussion until end of this week.

Looking forward to this.

> There is no big pressure as the problem is more unpleasant than causing 
> a real problem right now.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 19:05 KMSAN: uninit-value in can_receive syzbot
2019-11-18 20:25 ` Oliver Hartkopp
2019-11-18 20:29   ` Marc Kleine-Budde
2019-11-18 20:49     ` Oliver Hartkopp
2019-11-18 21:15       ` Marc Kleine-Budde
2019-11-19  7:35         ` Oliver Hartkopp
2019-11-19  9:00           ` Oleksij Rempel
2019-11-19 10:08           ` Dmitry Vyukov
2019-11-19 13:06             ` Alexander Potapenko
2019-11-19 16:53           ` Eric Dumazet
2019-11-19 20:24             ` Oliver Hartkopp
2019-11-19 21:09               ` Eric Dumazet
2019-11-20 20:10                 ` Oliver Hartkopp
2019-12-03 10:09                   ` Marc Kleine-Budde
2019-12-03 10:37                     ` Oliver Hartkopp
2019-12-03 10:40                       ` Marc Kleine-Budde
2019-11-26  9:00 ` syzbot

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git