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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 16 Sep 2013 13:58:30 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Hong Zhiguo <honkiko@...il.com>
CC:	netdev@...r.kernel.org, davem@...emloft.net,
	eric.dumazet@...il.com, Hong Zhiguo <zhiguohong@...cent.com>
Subject: Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu

On 09/14/2013 10:42 AM, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@...cent.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@...cent.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)
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html