Re: [BUG] skb corruption and kernel panic at forwarding with fragmentation

On Thu, Jan 7, 2016 at 2:49 AM, Florian Westphal [off-list ref] wrote:

Florian Westphal [off-list ref] wrote:

quoted

Thadeu Lima de Souza Cascardo [off-list ref] wrote:

quoted

On Wed, Jan 06, 2016 at 11:11:41PM +0300, Konstantin Khlebnikov wrote:

[ skb_gso_segment uses skb->cb[], causes oops if ip_fragment is invoked
  on segmented skbs ]

quoted

quoted

I have hit this as well, this fixes it for me on an older kernel. Can you try it
on latest kernel?

quoted

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index d8a1745..f44bc91 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb)
    netdev_features_t features;
    struct sk_buff *segs;
    int ret = 0;
+   struct inet_skb_parm ipcb;

    if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb))
            return ip_finish_output2(skb);

@@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb)
     * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly
     * from host network stack.
     */
+   /* We need to save IPCB here because skb_gso_segment will use
+    * SKB_GSO_CB.
+    */
+   ipcb = *IPCB(skb);
    features = netif_skb_features(skb);
    segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
    if (IS_ERR_OR_NULL(segs)) {

@@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb)
            int err;

            segs->next = NULL;
+           *IPCB(segs) = ipcb;
            err = ip_fragment(segs, ip_finish_output2);

            if (err && ret == 0)

I'm worried that this doesn't solve all cases. f.e. xfrm may also
call skb_gso_segment(), and it will call into ipv4/ipv6 netfilter
postrouting + ipv4 output functions...

nfqnl_enqueue_packet() is also affected.

... but it seems that those three are the only affected callers
of skb_gso_segment (tbf is ok since skb isn't owned by anyone,
ovs does save/restore already).

I think this patch is the right way, we just need similar
save/restore in nfqnl_enqueue_packet and xfrm_output_gso().

Which CB could be here? at this point skb isn't owned by netlink yet.

The latter two can be used by either ipv4 or ipv6 so it might
be preferable to just save/restore sizeof(struct skb_gso_cb);
or a union of inet_skb_parm+inet6_skb_parm.

Or just shift GSO CB and add couple checks like
BUILD_BUG_ON(sizeof(SKB_GSO_CB(skb)->room) < sizeof(*IPCB(skb)));