Patchwork BUG: unable to handle kernel NULL pointer dereference at br_pass_frame_up

login
register
mail settings
Submitter michael-dev@fami-braun.de
Date March 14, 2010, 2:30 a.m.
Message ID <4B9C4A2F.8040203@fami-braun.de>
Download mbox | patch
Permalink /patch/47726/
State RFC
Delegated to: David Miller
Headers show

Comments

michael-dev@fami-braun.de - March 14, 2010, 2:30 a.m.
Hi,

I'm currently using linux-next and have been running into an OOPs which
I think might be caused by a patch submitted on 2010-02-27.

It's a linux-next kernel from 2010-03-12 on an x86 system and it
OOPs in the bridge module in br_pass_frame_up (called by
br_handle_frame_finish) because brdev cannot be dereferenced (its set to
a non-null value).

Adding some BUG_ON statements revealed that
 BR_INPUT_SKB_CB(skb)->brdev == br-dev
(as set in br_handle_frame_finish first)
only holds until br_forward is called.
The next call to br_pass_frame_up then fails.

Digging deeper it seems that br_forward either frees the skb or passes
it to NF_HOOK which will in turn take care of freeing the skb. The same
is holds for br_pass_frame_ip. So it seems as if two independent skb
allocations are required. As far as I can see, commit
b33084be192ee1e347d98bb5c9e38a53d98d35e2 removed skb duplication and so
likely causes this crash. This crash does not happen on 2.6.33.

I've therefore modified br_forward the same way br_flood has been
modified so that the skb is not freed if skb0 is going to be used
and I can confirm that the attached patch resolves the issue for me.

Please don't hesitate to contact me if there are any questions.

Sincerely,
 M. Braun

// gdb output //

(gdb) bt full

#0  br_pass_frame_up (skb=0xcf9627c0) at net/bridge/br_input.c:27

        indev = <value optimised out>

        brdev = 0x1012
#1  br_handle_frame_finish (skb=0xcf9627c0) at net/bridge/br_input.c:99
        dest = 0xcfa59dae "ɊU", <incomplete sequence \307>
        br = 0xcec46300
        dst = <value optimised out>
        mdst = <value optimised out>
        skb2 = 0xcf9627c0
#2  0xd0cbfef8 in NF_HOOK_THRESH (p=0xcfb3b620, skb=0xcf9627c0) at
include/linux/netfilter.h:206
        ret = <value optimised out>
#3  NF_HOOK (p=0xcfb3b620, skb=0xcf9627c0) at include/linux/netfilter.h:228
No locals.
#4  br_handle_frame (p=0xcfb3b620, skb=0xcf9627c0) at
net/bridge/br_input.c:177
        dest = 0xcfa59dae "ɊU", <incomplete sequence \307>
#5  0xc035d70f in handle_bridge (skb=0xcf9627c0) at net/core/dev.c:2344
No locals.
#6  netif_receive_skb (skb=0xcf9627c0) at net/core/dev.c:2546
        ptype = <value optimised out>
        pt_prev = 0x0
        orig_dev = 0xcfb65020
        null_or_orig = 0x0
        null_or_bond = 0x0
        ret = -810209540
        type = <value optimised out>
#7  0xc035d8c5 in process_backlog (napi=<value optimised out>,
quota=<value optimised out>) at net/core/dev.c:2926
        skb = 0x1012
        work = 1
        start_time = 487863
#8  0xc035da70 in net_rx_action (h=<value optimised out>) at
net/core/dev.c:3057
        n = 0xc04fea5c
        work = 0
        weight = 64
        time_limit = 487865
        budget = 300
#9  0xc0126041 in __do_softirq () at kernel/softirq.c:219
        prev_count = 257
        h = 0x1012
        pending = 1
        max_restart = 10
        cpu = 0
#10 0xc01260f2 in do_softirq () at kernel/softirq.c:266
        flags = 582
#11 0xc035df5f in netif_rx_ni (skb=<value optimised out>) at
net/core/dev.c:2241
        err = 0
#12 0xd0cdc226 in tun_get_user (iocb=<value optimised out>,
iv=0xcfb2ff54, count=<value optimised out>, pos=0) at drivers/net/tun.c:666
        len = 64
        align = 2
        gso = {flags = 0 '\000', gso_type = 0 '\000', hdr_len = 0,
gso_size = 0, csum_start = 0, csum_offset = 0}
        pi = {flags = 0, proto = 8}
        skb = 0xcf9627c0
        offset = 0
#13 tun_chr_aio_write (iocb=<value optimised out>, iv=0xcfb2ff54,
count=<value optimised out>, pos=0) at drivers/net/tun.c:686
        file = <value optimised out>
        tun = 0xcfb10300
        result = 64
#14 0xc017bb12 in do_sync_write (filp=0xcf978300, buf=<value optimised
out>, len=<value optimised out>, ppos=0xcfb2ff98) at fs/read_write.c:318
        iov = {iov_base = 0xbfb4b478, iov_len = 64}
        kiocb = {ki_run_list = {next = 0xcfb2fee0, prev = 0x4}, ki_flags
= 0, ki_users = 1, ki_key = 4294967295, ki_filp = 0xcf978300, ki_ctx =
0x0, ki_cancel = 0, ki_retry = 0, ki_dtor = 0, ki_obj = {user =
0xcfb089c0, tsk = 0xcfb089c0}, ki_user_data = 0, ki_pos = 0,
          private = 0xc04d9e40, ki_opcode = 787, ki_nbytes = 3484614480,
ki_buf = 0xc013c430
"\203\304\024[^_]\303U\211\345WVS\203\354\020\203=\fdP\300", ki_left =
64, ki_inline_vec = {iov_base = 0xcfb2e000, iov_len = 0}, ki_iovec =
0xcfb2ff50, ki_nr_segs = 3484614336,
          ki_cur_seg = 16, ki_list = {next = 0xcfb2ff5c, prev = 0x1},
ki_eventfd = 0xcf382730}
        ret = <value optimised out>
#15 0xc017c43c in vfs_write (file=0xcf978300, buf=0xbfb4b478 "",
count=3484757756, pos=0xcfb2ff98) at fs/read_write.c:347
        ret = <value optimised out>
#16 0xc017c561 in sys_write (fd=4, buf=0xbfb4b478 "", count=64) at
fs/read_write.c:399
        pos = 0
        file = 0xcf978300
        ret = -9
        fput_needed = 0
#17 0xc03e8471 in ?? () at arch/x86/kernel/entry_32.S:541
No locals.
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) print *brdev
**Cannot access memory at address 0x1012**


// commit //
  commit b33084be192ee1e347d98bb5c9e38a53d98d35e2
  Author: Herbert Xu <herbert@gondor.apana.org.au>
  Date:   Sat Feb 27 19:41:41 2010 +0000

    bridge: Avoid unnecessary clone on forward path

    When the packet is delivered to the local bridge device we may
    end up cloning it unnecessarily if no bridge port can receive
    the packet in br_flood.

    This patch avoids this by moving the skb_clone into br_flood.

    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Patch

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index d61e6f7..8347916 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -19,6 +19,10 @@ 
 #include <linux/netfilter_bridge.h>
 #include "br_private.h"
 
+static int deliver_clone(struct net_bridge_port *prev, struct sk_buff *skb,
+			 void (*__packet_hook)(const struct net_bridge_port *p,
+					       struct sk_buff *skb));
+
 /* Don't forward packets to originating port or forwarding diasabled */
 static inline int should_deliver(const struct net_bridge_port *p,
 				 const struct sk_buff *skb)
@@ -94,14 +98,18 @@  void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
 }
 
 /* called with rcu_read_lock */
-void br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
+void br_forward(const struct net_bridge_port *to, struct sk_buff *skb, struct sk_buff *skb0)
 {
 	if (should_deliver(to, skb)) {
-		__br_forward(to, skb);
+		if (skb0)
+			deliver_clone(to, skb, __br_forward);
+		else
+			__br_forward(to, skb);
 		return;
 	}
 
-	kfree_skb(skb);
+	if (!skb0)
+		kfree_skb(skb);
 }
 
 static int deliver_clone(struct net_bridge_port *prev, struct sk_buff *skb,
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 53b3985..08a72e6 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -90,7 +90,7 @@  int br_handle_frame_finish(struct sk_buff *skb)
 
 	if (skb) {
 		if (dst)
-			br_forward(dst->dst, skb);
+			br_forward(dst->dst, skb, skb2);
 		else
 			br_flood_forward(br, skb, skb2);
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index fef0384..bfb8feb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -252,7 +252,7 @@  extern void br_deliver(const struct net_bridge_port *to,
 		struct sk_buff *skb);
 extern int br_dev_queue_push_xmit(struct sk_buff *skb);
 extern void br_forward(const struct net_bridge_port *to,
-		struct sk_buff *skb);
+		struct sk_buff *skb, struct sk_buff *skb0);
 extern int br_forward_finish(struct sk_buff *skb);
 extern void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb);
 extern void br_flood_forward(struct net_bridge *br, struct sk_buff *skb,