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] [day] [month] [year] [list]

Message-ID: <20130916114405.74966af4@nehalam.linuxnetplumber.net>
Date:	Mon, 16 Sep 2013 11:44:05 -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 14:37:19 -0400
Vlad Yasevich <vyasevic@...hat.com> wrote:

> On 09/16/2013 02:32 PM, Stephen Hemminger wrote:
> > 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.
> >
> 
> 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.


--
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