netdev - Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER

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: <20150911202033.GE7029@orbit.nwl.cc>
Date:	Fri, 11 Sep 2015 22:20:33 +0200
From:	Phil Sutter <phil@....cc>
To:	Stephen Hemminger <stephen@...workplumber.org>
Cc:	bridge@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without
 REORDER_HDR

On Fri, Sep 11, 2015 at 12:24:45PM -0700, Stephen Hemminger wrote:
> On Fri, 11 Sep 2015 21:22:03 +0200
> Phil Sutter <phil@....cc> wrote:
> 
> > When forwarding packets from an 802.1Q interface with REORDER_HDR set to
> > zero, the VLAN header previously inserted by vlan_do_receive() needs to
> > be stripped from the packet and the mac_header adjustment undone,
> > otherwise a tagged frame with first four bytes missing will be
> > transmitted.
> > 
> > Signed-off-by: Phil Sutter <phil@....cc>
> > ---
> >  net/bridge/br_input.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index f921a5d..e4e3fc7 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -288,6 +288,16 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  	}
> >  
> >  forward:
> > +	if (is_vlan_dev(skb->dev) &&
> > +	    !(vlan_dev_priv(skb->dev)->flags & VLAN_FLAG_REORDER_HDR)) {
> > +		unsigned int offset = skb->data - skb_mac_header(skb);
> > +
> > +		skb_push(skb, offset);
> > +		memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> > +		skb->mac_header += VLAN_HLEN;
> > +		skb_pull(skb, offset);
> > +		skb_reset_mac_len(skb);
> > +	}
> >  	switch (p->state) {
> >  	case BR_STATE_FORWARDING:
> >  		rhook = rcu_dereference(br_should_route_hook);
> 
> Thanks for finding this. Is this a new thing or has it always been there?

Sorry, I didn't check if this is a regression or not. Seen initially
with RHEL7's kernel-3.10.0-229.7.2, which due to the massive backporting
is by far not as old as it might seem. But it's surely not a brand new
problem of net-next or so.

Since nowadays no sane mind touches REORDER_HDR (there was originally a
bug in NetworkManager which defaulted this to 0), it may very well be
there for a long time already.

> Sorry, this looks so special case it doesn't seem like a good idea.
> Something is broken in VLAN handling if this is required.

It is so ugly, I wish I had found a better way to fix the problem. Well,
maybe I miss something:

- packet enters __netif_receive_skb_core():
  - skb->protocol is set to ETH_P_8021Q, so:
    - packet is untagged
    - skb->vlan_tci set
    - skb->protocol set to 'real' protocol
  - skb_vlan_tag_present(skb) == true, so:
    - vlan_do_receive() is called:
      - tags the packet again
      - zeroes vlan_tci
    - goto another_round
- __netif_receive_skb_core(), round 2:
  - skb->protocol is not ETH_P_8021Q -> no untagging
  - skb_vlan_tag_present(skb) == false -> no vlan_do_receive()
  - rx_handler handler (== br_handle_frame) is called

IMO the root of all evil is the existence of REORDER_HDR itself. It
causes an skb which should have been untagged to being passed along with
VLAN header present and code dealing with it needs to clean up the mess.

Cheers, Phil
--
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