netdev - Re: [PATCH stable 3.11+] can: bcm: add skb destructor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1390953066.28432.26.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Tue, 28 Jan 2014 15:51:06 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Oliver Hartkopp <socketcan@...tkopp.net>
Cc:	David Miller <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>,
	Andre Naujoks <nautsch2@...il.com>
Subject: Re: [PATCH stable 3.11+] can: bcm: add skb destructor

On Tue, 2014-01-28 at 23:49 +0100, Oliver Hartkopp wrote:

> The sbk->sk reference is used to make sure in AF_CAN to identify the
> originating socket (if any) to not deliver echoed CAN frames to the
> originating application.
> 
> See first check in raw_rcv() in net/can/raw.c

Nice, this is buggy.

> 
> > 
> > Instead of understanding the issue, it seems this patch exactly shutup
> > the useful warning.
> 
> I would have been happy to have this a warning and not a bug as you
> implemented it.
> 

Yes, I understand you are not happy of our work to discover CAN bugs.

> I don't need this warning as I'm using skb_alloc in the cases where CAN frames
> are generated autonomously. They are not triggered through a direct socket
> write operation nor do they need to take case about any sock wmem.
> 
> The useful warning/bug might be nice for common use cases. I'm using plain
> skb_alloc here for fire-and-forget skbs.
> 
> So I need to shutup the useful warning or revert the two commits at
> skb_orphan(). I would prefer the latter.
> 
> > 
> > If you set skb->sk, then you expect a future reader of skb->sk to be
> > 100% sure the socket did not disappear.
> 
> It's a fire-and-forget skb. I don't need to care if the socket disappears.
> If it disappears no new traffic is generated. That's enough.
> 
> > 
> > I do not see this explained in the changelog.
> > 
> 
> I hopefully was able to make it more clearly.
> See Documentation/networking/can.txt
> 


Just take a reference on the damn socket, and we do not have to worry.

bcm_tx_send() suffers from the same problem

can_send() is buggy as well :

newskb->sk = skb->sk; // line 293

dev_queue_xmit() can queue a packet a long time, and some packet qdisc
even look at skb->sk.

So this is really wrong to assume only net/can can assume things about
skb->sk, and not care of net/core or net/sched users.

I absolutely disagree with your patch. You need quite different _real_
fixes.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html