From: Stephen Hemminger <stephen@networkplumber•org>
To: Hong Zhiguo <honkiko@gmail•com>
Cc: netdev@vger•kernel.org, davem@davemloft•net,
zhiguohong@tencent•com, eric.dumazet@gmail•com,
vyasevic@redhat•com
Subject: Re: [PATCH net-next] bridge: fix NULL pointer deref in br_handle_frame
Date: Fri, 13 Sep 2013 09:21:51 -0700 [thread overview]
Message-ID: <20130913092151.27143ccc@samsung-9> (raw)
In-Reply-To: <1379041787-14232-1-git-send-email-zhiguohong@tencent.com>
On Fri, 13 Sep 2013 11:09:47 +0800
Hong Zhiguo <honkiko@gmail•com> wrote:
> From: Hong Zhiguo <zhiguohong@tencent•com>
>
> I got an Oops on my box when br_handle_frame is called between these
> 2 lines of del_nbp:
> dev->priv_flags &= ~IFF_BRIDGE_PORT;
> /* --> br_handle_frame is called at this time */
> netdev_rx_handler_unregister(dev);
>
> In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
> without check but br_port_get_rcu(dev) returns NULL if:
> !(dev->priv_flags & IFF_BRIDGE_PORT)
>
> In my first fix I moved netdev_rx_handler_unregister up. Eric Dumazet
> pointed out the testing of IFF_BRIDGE_PORT is not necessary here since
> we're in rcu_read_lock and we have synchronize_net() in
> netdev_rx_handler_unregister. This fix removed the testing of
> IFF_BRIDGE_PORT.
>
> I tested the fix on my box with script doing "brctl addif" and "brctl
> delif" repeatedly while a lot of broadcast frame present on the LAN.
> I added msleep in del_nbp between setting of priv_flags and unregister
> so it's easy to reproduce the oops without the fix.
>
> I'll send another patch to net-next to take care of br_netfilter and
> ebtable if necessary(seems there's NULL check following but I'll
> have a look).
>
> The Oops(some lines omitted):
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
> IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> Oops: 0000 [#1] PREEMPT SMP
> RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> RSP: 0018:ffff880030403c10 EFLAGS: 00010286
> Stack:
> ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
> ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
> ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
> Call Trace:
> <IRQ>
> [<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
> [<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent•com>
> ---
> net/bridge/br_input.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a2fd37e..2244049 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> struct net_bridge *br;
> struct net_bridge_fdb_entry *dst;
> struct net_bridge_mdb_entry *mdst;
> @@ -143,7 +143,7 @@ drop:
> /* note: already called with rcu_read_lock */
> static int br_handle_local_finish(struct sk_buff *skb)
> {
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> u16 vid = 0;
>
> br_vlan_get_tag(skb, &vid);
> @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> if (!skb)
> return RX_HANDLER_CONSUMED;
>
> - p = br_port_get_rcu(skb->dev);
> + p = rcu_dereference(skb->dev->rx_handler_data);
>
> if (unlikely(is_link_local_ether_addr(dest))) {
> /*
There are more uses of br_port_get_rcu that have the same problem.
For example receiving an STP packet in that window.
I bet if you look at all the callers of br_port_get_rcu() you will
see the same issue, therefore either the check should be removed from br_port_get_rcu.
A little bit of history, the bridge code orginally did not use RCU,
and there was a flag on the device. Then RCU was added, then the receive
handler stuff was added.
prev parent reply other threads:[~2013-09-13 16:21 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-12 12:16 [PATCH net-next] fix NULL pointer dereference in br_handle_frame Hong Zhiguo
2013-09-12 14:11 ` Eric Dumazet
2013-09-12 14:24 ` Hong zhi guo
2013-09-12 14:33 ` Eric Dumazet
2013-09-12 14:42 ` Vlad Yasevich
2013-09-12 15:24 ` Eric Dumazet
2013-09-12 15:50 ` Vlad Yasevich
2013-09-12 16:08 ` Eric Dumazet
2013-09-12 15:57 ` Hong Zhiguo
2013-09-12 16:06 ` Hong zhi guo
2013-09-12 16:11 ` Eric Dumazet
2013-09-12 16:18 ` Hong zhi guo
2013-09-12 16:19 ` Eric Dumazet
2013-09-12 16:12 ` Eric Dumazet
2013-09-12 19:18 ` David Miller
2013-09-13 3:09 ` [PATCH net-next] bridge: fix NULL pointer deref " Hong Zhiguo
2013-09-13 16:21 ` Stephen Hemminger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130913092151.27143ccc@samsung-9 \
--to=stephen@networkplumber$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=honkiko@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=vyasevic@redhat$(echo .)com \
--cc=zhiguohong@tencent$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox