netdev - Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

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

Date:	Wed, 29 Jan 2014 07:02:25 -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: use private sk reference to detect
 originating socket

On Wed, 2014-01-29 at 14:44 +0100, Oliver Hartkopp wrote:
> Commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan())
> leads to a BUG in can_put_echo_skb() when skb_orphan() is executed.
> 
> The use of skb->sk to detect the originating socket can lead to side effects
> in net/core and net/sched. Therefore the reference needed in net/can/raw.c is
> now stored in a CAN private skbuff space in struct can_skb_priv.
> 
> The reference which comes with the CAN frame is only needed in raw_rcv() and
> checked against his own sock pointer and does only special things if it
> matches. So removing the socket does not hurt as the in-flight skbs are
> removed and the receive filters are purged anyway when the socket disappears.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
> 
> ---
> 
> Here's my idea to fix up the original skb->sk handling concept for detecting
> the originating socket instance. The patch applies properly down to 3.11.
> 
> Eric please take a look if I used sock_alloc_send_skb() correctly in bcm.c.
> Andre please check if you see any functional differences on your machine.

I think this is opening yet another can of bugs.

CAN calls dev_queue_xmit()  : This layer can zap skb->cb[] totally.

Please hold a reference on the socket instead. Thats a 10 lines patch,
instead of yet another complex patch.

void can_destructor(struct sk_buff *skb)
{
	sock_put(skb->sk);
}

void can_set_owner(struct sk_buff *skb, struct sock *sk)
{
	skb_orphan(skb);
	skb->destructor = can_skb_destructor;
	skb->sk = sk;
	sock_hold(sk);
}

Thats how every protocol does this, with variants.

Check l2tp_sock_wfree()/l2tp_skb_set_owner_w() for an example


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