[PATCH net-next 0/2] bridge: fix NULL pointer deref of br_port_get_rcu

[PATCH net-next 0/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Hong Zhiguo]

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)

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. So 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 want to remove the NULL checks following call to br_port_get_rcu in
Br_netfilter and ebtables code. Could someone confirm that's OK?

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

Hong Zhiguo (2):
  bridge: use br_port_get_rtnl within rtnl lock
  bridge: fix NULL pointer deref of br_port_get_rcu

 net/bridge/br_netlink.c | 4 ++--
 net/bridge/br_private.h | 7 ++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

-- 
1.8.1.2

[PATCH net-next 1/2] bridge: use br_port_get_rtnl within rtnl lock

[Hong Zhiguo]

From: Hong Zhiguo <zhiguohong@tencent.com>

current br_port_get_rcu is problematic in bridging path
(NULL deref). Change these calls in netlink path first.

Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
 net/bridge/br_netlink.c | 4 ++--
 net/bridge/br_private.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index b9259ef..e74ddc1 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -207,7 +207,7 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 	       struct net_device *dev, u32 filter_mask)
 {
 	int err = 0;
-	struct net_bridge_port *port = br_port_get_rcu(dev);
+	struct net_bridge_port *port = br_port_get_rtnl(dev);
 
 	/* not a bridge port and  */
 	if (!port && !(filter_mask & RTEXT_FILTER_BRVLAN))
@@ -451,7 +451,7 @@ static size_t br_get_link_af_size(const struct net_device *dev)
 	struct net_port_vlans *pv;
 
 	if (br_port_exists(dev))
-		pv = nbp_get_vlan_info(br_port_get_rcu(dev));
+		pv = nbp_get_vlan_info(br_port_get_rtnl(dev));
 	else if (dev->priv_flags & IFF_EBRIDGE)
 		pv = br_get_vlan_info((struct net_bridge *)netdev_priv(dev));
 	else
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 598cb0b..49fb43e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -208,7 +208,7 @@ static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *d
 	return br_port_exists(dev) ? port : NULL;
 }
 
-static inline struct net_bridge_port *br_port_get_rtnl(struct net_device *dev)
+static inline struct net_bridge_port *br_port_get_rtnl(const struct net_device *dev)
 {
 	return br_port_exists(dev) ?
 		rtnl_dereference(dev->rx_handler_data) : NULL;
-- 
1.8.1.2

[PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Hong Zhiguo]

From: Hong Zhiguo <zhiguohong@tencent.com>

The NULL deref happens 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)

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. So remove the testing of IFF_BRIDGE_PORT
and by the previous patch, make sure br_port_get_rcu is called in
bridging code.

Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
 net/bridge/br_private.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 49fb43e..1aaca0e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -202,10 +202,7 @@ struct net_bridge_port
 
 static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
 {
-	struct net_bridge_port *port =
-			rcu_dereference_rtnl(dev->rx_handler_data);
-
-	return br_port_exists(dev) ? port : NULL;
+	return rcu_dereference(dev->rx_handler_data);
 }
 
 static inline struct net_bridge_port *br_port_get_rtnl(const struct net_device *dev)
-- 
1.8.1.2

Re: [PATCH net-next 1/2] bridge: use br_port_get_rtnl within rtnl lock

[Eric Dumazet]

On Sat, 2013-09-14 at 22:42 +0800, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
> 
> current br_port_get_rcu is problematic in bridging path
> (NULL deref). Change these calls in netlink path first.
> 
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
>  net/bridge/br_netlink.c | 4 ++--
>  net/bridge/br_private.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

This is not net-next patch, but suitable for net tree, as the following
one fixes a potential panic.

Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Eric Dumazet]

On Sat, 2013-09-14 at 22:42 +0800, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
> 
> The NULL deref happens 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)
> 
> 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. So remove the testing of IFF_BRIDGE_PORT
> and by the previous patch, make sure br_port_get_rcu is called in
> bridging code.
> 
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
>  net/bridge/br_private.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

Again this is suitable for net tree.

Thanks !

Re: [PATCH net-next 0/2] bridge: fix NULL pointer deref of br_port_get_rcu

[David Miller]

From: Hong Zhiguo <honkiko@gmail.com>
Date: Sat, 14 Sep 2013 22:42:26 +0800

> Hong Zhiguo (2):
>   bridge: use br_port_get_rtnl within rtnl lock
>   bridge: fix NULL pointer deref of br_port_get_rcu

Both applied and queued up for -stable, thanks.

Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Vlad Yasevich]

On 09/14/2013 10:42 AM, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
>
> The NULL deref happens 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)
>
> 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. So remove the testing of IFF_BRIDGE_PORT
> and by the previous patch, make sure br_port_get_rcu is called in
> bridging code.
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>

I think would be better to also include your initial patch to move the
call netdev_rx_handler_unregister(dev) up higher as it would reduce the
racy nature of input processing and port removal.

As it is now, up until netdev_rx_handler_unregister(dev) call, the input
process may call into the bridge code only to drop the packet.  With 
ebtables, that can consume considerable time that is wasted.  By 
performing the unregister() sooner we reduce the racy nature of the two
calls.

-vlad

> ---
>   net/bridge/br_private.h | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 49fb43e..1aaca0e 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -202,10 +202,7 @@ struct net_bridge_port
>
>   static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
>   {
> -	struct net_bridge_port *port =
> -			rcu_dereference_rtnl(dev->rx_handler_data);
> -
> -	return br_port_exists(dev) ? port : NULL;
> +	return rcu_dereference(dev->rx_handler_data);
>   }
>
>   static inline struct net_bridge_port *br_port_get_rtnl(const struct net_device *dev)
>

Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Stephen Hemminger]

On Mon, 16 Sep 2013 13:58:30 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> On 09/14/2013 10:42 AM, Hong Zhiguo wrote:
> > From: Hong Zhiguo <zhiguohong@tencent.com>
> >
> > The NULL deref happens 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)
> >
> > 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. So remove the testing of IFF_BRIDGE_PORT
> > and by the previous patch, make sure br_port_get_rcu is called in
> > bridging code.
> >
> > Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> 
> I think would be better to also include your initial patch to move the
> call netdev_rx_handler_unregister(dev) up higher as it would reduce the
> racy nature of input processing and port removal.
> 
> As it is now, up until netdev_rx_handler_unregister(dev) call, the input
> process may call into the bridge code only to drop the packet.  With 
> ebtables, that can consume considerable time that is wasted.  By 
> performing the unregister() sooner we reduce the racy nature of the two
> calls.
> 
> -vlad

The change to just use rcu_dereference is safe.
The flag ordering doesn't matter. it is only valid to check it under RTNL.

Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Vlad Yasevich]

On 09/16/2013 02:32 PM, Stephen Hemminger wrote:
> On Mon, 16 Sep 2013 13:58:30 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> On 09/14/2013 10:42 AM, Hong Zhiguo wrote:
>>> From: Hong Zhiguo <zhiguohong@tencent.com>
>>>
>>> The NULL deref happens 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)
>>>
>>> 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. So remove the testing of IFF_BRIDGE_PORT
>>> and by the previous patch, make sure br_port_get_rcu is called in
>>> bridging code.
>>>
>>> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
>>
>> I think would be better to also include your initial patch to move the
>> call netdev_rx_handler_unregister(dev) up higher as it would reduce the
>> racy nature of input processing and port removal.
>>
>> As it is now, up until netdev_rx_handler_unregister(dev) call, the input
>> process may call into the bridge code only to drop the packet.  With
>> ebtables, that can consume considerable time that is wasted.  By
>> performing the unregister() sooner we reduce the racy nature of the two
>> calls.
>>
>> -vlad
>
> The change to just use rcu_dereference is safe.
> The flag ordering doesn't matter. it is only valid to check it under RTNL.
>

Yes, I agree.  I am just saying that that there are other things that 
will be happening at the same time as input processing like port state 
change and fdb table change and it might be worthwhile to easily prevent 
this racy processing.

-vlad

Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

[Stephen Hemminger]

On Mon, 16 Sep 2013 14:37:19 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> On 09/16/2013 02:32 PM, Stephen Hemminger wrote:
> > On Mon, 16 Sep 2013 13:58:30 -0400
> > Vlad Yasevich <vyasevic@redhat.com> wrote:
> >
> >> On 09/14/2013 10:42 AM, Hong Zhiguo wrote:
> >>> From: Hong Zhiguo <zhiguohong@tencent.com>
> >>>
> >>> The NULL deref happens 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)
> >>>
> >>> 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. So remove the testing of IFF_BRIDGE_PORT
> >>> and by the previous patch, make sure br_port_get_rcu is called in
> >>> bridging code.
> >>>
> >>> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> >>
> >> I think would be better to also include your initial patch to move the
> >> call netdev_rx_handler_unregister(dev) up higher as it would reduce the
> >> racy nature of input processing and port removal.
> >>
> >> As it is now, up until netdev_rx_handler_unregister(dev) call, the input
> >> process may call into the bridge code only to drop the packet.  With
> >> ebtables, that can consume considerable time that is wasted.  By
> >> performing the unregister() sooner we reduce the racy nature of the two
> >> calls.
> >>
> >> -vlad
> >
> > The change to just use rcu_dereference is safe.
> > The flag ordering doesn't matter. it is only valid to check it under RTNL.
> >
> 
> Yes, I agree.  I am just saying that that there are other things that 
> will be happening at the same time as input processing like port state 
> change and fdb table change and it might be worthwhile to easily prevent 
> this racy processing.

Port state change is protected by RTNL.
FDB table is fine, it is RCU protected.