Messages in this thread | | | Subject | Re: KMSAN: uninit-value in can_receive | From | Oliver Hartkopp <> | Date | Tue, 19 Nov 2019 21:24:53 +0100 |
| |
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
| |