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