LinuxLists.cc - KMSAN: uninit-value in can

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>
> =====================================================
> 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

2019-11-18 20:34:21

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>>
>> =====================================================
>> 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 |

Attachments:

2019-11-18 20:53:18

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>>>
>>> =====================================================
>>> 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

2019-11-18 21:17:20

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>>>>
>>>> =====================================================
>>>> 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 |

Attachments:

2019-11-19 07:42:45

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>>>>>
>>>>> =====================================================
>>>>> 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

2019-11-19 09:03:19

by Oleksij Rempel

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]
>>>>>>
>>>>>> =====================================================
>>>>>> 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 |

2019-11-19 10:11:27

by Dmitry Vyukov

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

On Tue, Nov 19, 2019 at 8:36 AM Oliver Hartkopp <[email protected]> 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: [email protected]
> >>>>>
> >>>>> =====================================================
> >>>>> 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?

2019-11-19 13:09:17

by Alexander Potapenko

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

On Tue, Nov 19, 2019 at 11:09 AM Dmitry Vyukov <[email protected]> wrote:
>
> On Tue, Nov 19, 2019 at 8:36 AM Oliver Hartkopp <[email protected]> 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: [email protected]
> > >>>>>
> > >>>>> =====================================================
> > >>>>> 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

2019-11-19 16:55:17

by Eric Dumazet

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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();

2019-11-19 20:32:57

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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

2019-11-19 21:11:54

by Eric Dumazet

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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.

2019-11-20 20:12:52

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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

2019-11-26 09:02:24

by syzbot

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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: [email protected]

=====================================================
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
=====================================================

2019-12-03 10:10:07

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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 |

Attachments:

2019-12-03 10:38:55

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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
>

2019-12-03 10:42:07

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in can_receive

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 |

Attachments: