Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Bryam Vargas <hexlabsecurity@proton.me>]

Alexandra,

> Excuse my ignorance, if it is obvious to other readers, but is the worst
> thing that the output of tcpdump is not correct?

Not obvious, and my description is why: it led with tcpdump, which is the
mildest end of this.

The order is the other way round. __netif_receive_skb_core() walks
ptype_base[] at net/core/dev.c:6160, before net->ptype_specific (:6169)
and orig_dev->ptype_specific (:6173). iucv_packet_type sets no .dev and no
.af_packet_net, so it sits in ptype_base[] while a packet socket for
ETH_P_AF_IUCV lands in one of the later lists. af_iucv runs first, and the
AF_PACKET reader gets the frame after EBCASC() has rewritten the four name
fields. The capture is wrong, but it was already wrong before the reader
was reached.

That isn't what I'd defend the patch on. Because af_iucv isn't the last
matching handler in that configuration, deliver_ptype_list_skb() hands it
over through deliver_skb(), which does refcount_inc(&skb->users) before
calling us (dev.c:2492, :2507). We run with users == 2, and on that skb we
rewrite the header in place, skb_push() 14 bytes in afiucv_swap_src_dest()
and pass the same skb to dev_queue_xmit() (af_iucv.c:1876, :1888, :1914) --
including for a frame that matched no socket (:1872).

What hides it in review is a guard asymmetry. deliver_skb() leaves
users == 2 with skb->cloned == 0, so skb_shared() is true while
skb_cloned() is false, and the copy-on-write guards all test skb_cloned()
-- __pskb_pull_tail() at skbuff.c:2886 among them -- so they read the skb
as already writable. The one that does test it is BUG_ON(skb_shared(skb))
at the top of pskb_expand_head() (skbuff.c:2305); skb_expand_head() carries
"/* pskb_expand_head() might crash, if skb is shared. */" (:2456) for the
same reason.

What I don't have is a panic. On the qeth geometry the first
pskb_may_pull() finds enough tailroom in the napi_get_frags() head and
copies out of the frags without expanding, so it doesn't reach
pskb_expand_head that way. By inspection; not reproduced.

> Is this really a problem fix then? Or should it go to net-next?

If the bar is a failure I can show you, net-next is right. I sent it to net
because a handler that writes a shared skb and then gives it to the
transmit path is a rule violation with a BUG_ON behind it, not because I
can fire that BUG_ON. Your call either way, and net-next is fine by me.

Worth having in the record: reaching the shared state costs one syscall --
socket(AF_PACKET, SOCK_RAW, htons(0xFBFB)), no bind, no ETH_P_ALL -- since
ptype_base[] is walked before the per-namespace list.

If Hidayath's version is further along, take his. I'd rather the check land
than land mine.

Thanks,
Bryam