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]

Message-ID: <20130916113242.38562647@nehalam.linuxnetplumber.net>
Date:	Mon, 16 Sep 2013 11:32:42 -0700
From:	Stephen Hemminger <stephen@...workplumber.org>
To:	vyasevic@...hat.com
Cc:	Hong Zhiguo <honkiko@...il.com>, 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 Mon, 16 Sep 2013 13:58:30 -0400
Vlad Yasevich <vyasevic@...hat.com> wrote:

> 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

The change to just use rcu_dereference is safe.
The flag ordering doesn't matter. it is only valid to check it under RTNL.
--
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