[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <461C6FF2.2000700@trash.net>
Date: Wed, 11 Apr 2007 07:19:46 +0200
From: Patrick McHardy <kaber@...sh.net>
To: Bart De Schuymer <bdschuym@...dora.be>
CC: Stephen Hemminger <shemminger@...ux-foundation.org>,
netdev@...r.kernel.org
Subject: Re: [GIT PULL] bridge update for 2.6.22
Stephen Hemminger wrote:
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a260679..8a55276 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> if (unlikely(is_link_local(dest))) {
> skb->pkt_type = PACKET_HOST;
> - return NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
> - NULL, br_handle_local_finish) != 0;
> +
> + return (NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
> + NULL, br_handle_local_finish) == 0) ? skb : NULL;
> }
I Just want to note, this is broken in multiple ways (not by this
patch, it was already broken before). When a packet is stolen or
queued, NF_HOOK will return 0, but the packet is not owned by the
caller anymore, so we have a potential use-after-free. Additionally
the okfn owns the skb and needs to make sure it continues its path,
which br_handle_local_finish doesn't do, resulting in leaks and
broken queueing. The fix looks quite ugly, bf_handle_local_finish
would need to pass the skb back to netif_receive_skb just after the
handle_bridge call.
All this is not a problem for mainline currently since ebtables
doesn't support QUEUE yet, but since its mentioned on the TODO
list and in any case is incorrect use of NF_HOOK it feels worth
mentioning.
-
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
Powered by blists - more mailing lists