netdev - Re: net: af_packet: skb_orphan should be avoided in TX path.

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: <1283787867.2654.600.camel@edumazet-laptop>
Date:	Mon, 06 Sep 2010 17:44:27 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	Changli Gao <xiaosuo@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: net: af_packet: skb_orphan should be avoided in TX path.

Le lundi 06 septembre 2010 à 13:35 +0300, Michael S. Tsirkin a écrit :

> I think there are bigger issues here.  As was pointed out, drivers might
> orphan skbs before they transmit them.
> And at least for tun, the reason is that we might hang on
> to skbs indefinitely because userspace is not reading them.
> 
> So in that case, if you just prevent tun from orphaning skbs, the socket
> will be prevented from sending any more packets out even if they are for
> a completely unrelated destinations, right?
> Further, module can't get unloaded and I think socket can not get
> closed, so user can't kill the task which has the socket?
> 
> And thinking about this, I think I see
> another issue related to the use of the destructor callback:
> 
> static void tpacket_destruct_skb(struct sk_buff *skb)
> {
>         struct packet_sock *po = pkt_sk(skb->sk);
>         void *ph;
> 
>         BUG_ON(skb == NULL);
> 
>         if (likely(po->tx_ring.pg_vec)) {
>                 ph = skb_shinfo(skb)->destructor_arg;
>                 BUG_ON(__packet_get_status(po, ph) != TP_STATUS_SENDING);
>                 BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
>                 atomic_dec(&po->tx_ring.pending);
>                 __packet_set_status(po, ph, TP_STATUS_AVAILABLE);
>         }
> 
>         sock_wfree(skb);
> 
> <-----
> at this point we still have to execute instructions
> in this function to return from it. However
> socket and thus module reference count
> got already dropped to 0, so I think module could get unloaded
> and these instructions could get overwritten.
> 
> }
> 
> I conclude that destructor callback should never point to a function residing
> in a module, always to a function that is guaranteed to be builtin, this
> function must be the one that drops the last module reference.

It would be a surprise to use tx mmap (presumably to get high
performance), and a modular af_unix ;)

> 
> Comments?

The whole thing (packet / tx mmap) is broken, if you ask me.

skb_orphan() is not about protecting data, but doing per socket memory
accounting.

We have to skb_orphan() while data is still in use by skb, not only in
drivers but in core network stack. (loopback case for example, no need
to think about TUN being special ;) )

So I believe using mmap and tx on af_unix is racy in its current design.

We probably can remove some skb_orphan() calls (now its done in core
network, no real need to make it from some drivers), but not have a
complete solution to the problem Changli raised, without adding yet
another field into skb_shared_info...



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