[PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Vlad Yasevich]

When the vlan filtering is enabled on the bridge, but
the filter is not configured on the bridge device itself,
running tcpdump on the bridge device will result in a
an Ooops with NULL pointer dereference.  The reason
is that br_pass_frame_up() will bypass the vlan
check because promisc flag is set.  It will then try
to get the table pointer and process the packet based
on the table.  Since the table pointer is NULL, we oops.
Catch this special condition in br_handle_vlan().

Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_input.c | 11 ++++++-----
 net/bridge/br_vlan.c  | 12 ++++++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 7e73c32..67fbea0 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -29,6 +29,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
 	struct net_device *indev, *brdev = BR_INPUT_SKB_CB(skb)->brdev;
 	struct net_bridge *br = netdev_priv(brdev);
 	struct br_cpu_netstats *brstats = this_cpu_ptr(br->stats);
+	struct net_port_vlans *pv;
 
 	u64_stats_update_begin(&brstats->syncp);
 	brstats->rx_packets++;
@@ -39,18 +40,18 @@ static int br_pass_frame_up(struct sk_buff *skb)
 	 * packet is allowed except in promisc modue when someone
 	 * may be running packet capture.
 	 */
+	pv = br_get_vlan_info(br);
 	if (!(brdev->flags & IFF_PROMISC) &&
-	    !br_allowed_egress(br, br_get_vlan_info(br), skb)) {
+	    !br_allowed_egress(br, pv, skb)) {
 		kfree_skb(skb);
 		return NET_RX_DROP;
 	}
 
-	skb = br_handle_vlan(br, br_get_vlan_info(br), skb);
-	if (!skb)
-		return NET_RX_DROP;
-
 	indev = skb->dev;
 	skb->dev = brdev;
+	skb = br_handle_vlan(br, pv, skb);
+	if (!skb)
+		return NET_RX_DROP;
 
 	return NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, indev, NULL,
 		       netif_receive_skb);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index af5ebd1..ea0c29b 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 	if (!br->vlan_enabled)
 		goto out;
 
+	/* Vlan filter table must be configrured at this point.  The
+	 * only exception is the bridge is set in promisc mode and the
+	 * packet is destined for the bridge device.  In this case
+	 * pass the packet as is.
+	 */
+	if (!pv) {
+       		if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)
+			goto out;
+		else
+			BUG();
+	}
+
 	/* At this point, we know that the frame was filtered and contains
 	 * a valid vlan id.  If the vlan id is set in the untagged bitmap,
 	 * send untagged; otherwise, send taged.
-- 
1.8.4.2

Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Vlad Yasevich]

On 01/03/2014 01:06 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/03/2014 07:12 PM, Vlad Yasevich wrote:
> 
>> When the vlan filtering is enabled on the bridge, but
>> the filter is not configured on the bridge device itself,
>> running tcpdump on the bridge device will result in a
>> an Ooops with NULL pointer dereference.  The reason
>> is that br_pass_frame_up() will bypass the vlan
>> check because promisc flag is set.  It will then try
>> to get the table pointer and process the packet based
>> on the table.  Since the table pointer is NULL, we oops.
>> Catch this special condition in br_handle_vlan().
> 
>> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> [...]
> 
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index af5ebd1..ea0c29b 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge
>> *br,
>>       if (!br->vlan_enabled)
>>           goto out;
>>
>> +    /* Vlan filter table must be configrured at this point.  The
> 
>    Only "configured".
> 
>> +     * only exception is the bridge is set in promisc mode and the
>> +     * packet is destined for the bridge device.  In this case
>> +     * pass the packet as is.
>> +     */
>> +    if (!pv) {
>> +               if ((br->dev->flags & IFF_PROMISC) && skb->dev ==
>> br->dev)
> 
>    You've indented this *if* too much.

Thanks, will fix in v2.  For now, will now wait for a few more reviews.

-vlad

> 
>> +            goto out;
>> +        else
>> +            BUG();
>> +    }
>> +
> 
> WBR, Sergei
> 

Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Sergei Shtylyov]

Hello.

On 01/03/2014 07:12 PM, Vlad Yasevich wrote:

> When the vlan filtering is enabled on the bridge, but
> the filter is not configured on the bridge device itself,
> running tcpdump on the bridge device will result in a
> an Ooops with NULL pointer dereference.  The reason
> is that br_pass_frame_up() will bypass the vlan
> check because promisc flag is set.  It will then try
> to get the table pointer and process the packet based
> on the table.  Since the table pointer is NULL, we oops.
> Catch this special condition in br_handle_vlan().

> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
[...]

> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..ea0c29b 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>   	if (!br->vlan_enabled)
>   		goto out;
>
> +	/* Vlan filter table must be configrured at this point.  The

    Only "configured".

> +	 * only exception is the bridge is set in promisc mode and the
> +	 * packet is destined for the bridge device.  In this case
> +	 * pass the packet as is.
> +	 */
> +	if (!pv) {
> +       		if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)

    You've indented this *if* too much.

> +			goto out;
> +		else
> +			BUG();
> +	}
> +

WBR, Sergei

Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Toshiaki Makita]

On Fri, 2014-01-03 at 11:12 -0500, Vlad Yasevich wrote:
> When the vlan filtering is enabled on the bridge, but
> the filter is not configured on the bridge device itself,
> running tcpdump on the bridge device will result in a
> an Ooops with NULL pointer dereference.  The reason
    ^^^^^
Oops?

> is that br_pass_frame_up() will bypass the vlan
> check because promisc flag is set.  It will then try
> to get the table pointer and process the packet based
> on the table.  Since the table pointer is NULL, we oops.
> Catch this special condition in br_handle_vlan().
> 
> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
...
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..ea0c29b 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>  	if (!br->vlan_enabled)
>  		goto out;
>  
> +	/* Vlan filter table must be configrured at this point.  The
> +	 * only exception is the bridge is set in promisc mode and the
> +	 * packet is destined for the bridge device.  In this case
> +	 * pass the packet as is.
> +	 */
> +	if (!pv) {
> +       		if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)
> +			goto out;

Currently this function is calling __vlan_put_tag() if port_idx==0.
You are going to bypass it only if br is promisc mode.

However it doesn't seem to lead any problem because
__netif_receive_skb_core() can handle hw accel vlan tag.

Instead, I doubt if __vlan_put_tag() is necessary in br_handle_vlan()...

Thanks,
Toshiaki Makita

Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Vlad Yasevich]

On 01/05/2014 10:02 AM, Toshiaki Makita wrote:
> On Fri, 2014-01-03 at 11:12 -0500, Vlad Yasevich wrote:
>> When the vlan filtering is enabled on the bridge, but
>> the filter is not configured on the bridge device itself,
>> running tcpdump on the bridge device will result in a
>> an Ooops with NULL pointer dereference.  The reason
>     ^^^^^
> Oops?
> 
>> is that br_pass_frame_up() will bypass the vlan
>> check because promisc flag is set.  It will then try
>> to get the table pointer and process the packet based
>> on the table.  Since the table pointer is NULL, we oops.
>> Catch this special condition in br_handle_vlan().
>>
>> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
> ...
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index af5ebd1..ea0c29b 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>>  	if (!br->vlan_enabled)
>>  		goto out;
>>  
>> +	/* Vlan filter table must be configrured at this point.  The
>> +	 * only exception is the bridge is set in promisc mode and the
>> +	 * packet is destined for the bridge device.  In this case
>> +	 * pass the packet as is.
>> +	 */
>> +	if (!pv) {
>> +       		if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)
>> +			goto out;
> 
> Currently this function is calling __vlan_put_tag() if port_idx==0.
> You are going to bypass it only if br is promisc mode.
> 
> However it doesn't seem to lead any problem because
> __netif_receive_skb_core() can handle hw accel vlan tag.
> 
> Instead, I doubt if __vlan_put_tag() is necessary in br_handle_vlan()...

I'll need to re-test, but I remember having had to add that code to make
it work on the bridge...

Looking at it again, I agree that special handling for forwarding to the
bridge doesn't seem to be needed.

-vlad

> 
> Thanks,
> Toshiaki Makita
> 
> 

Re: [PATCH net] bridge: Fix crash with vlan filtering and tcpdump

[Toshiaki Makita]

On Mon, 2014-01-06 at 11:02 -0500, Vlad Yasevich wrote:
> On 01/05/2014 10:02 AM, Toshiaki Makita wrote:
> > On Fri, 2014-01-03 at 11:12 -0500, Vlad Yasevich wrote:
> >> When the vlan filtering is enabled on the bridge, but
> >> the filter is not configured on the bridge device itself,
> >> running tcpdump on the bridge device will result in a
> >> an Ooops with NULL pointer dereference.  The reason
> >     ^^^^^
> > Oops?
> > 
> >> is that br_pass_frame_up() will bypass the vlan
> >> check because promisc flag is set.  It will then try
> >> to get the table pointer and process the packet based
> >> on the table.  Since the table pointer is NULL, we oops.
> >> Catch this special condition in br_handle_vlan().
> >>
> >> Reported-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >> ---
> > ...
> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >> index af5ebd1..ea0c29b 100644
> >> --- a/net/bridge/br_vlan.c
> >> +++ b/net/bridge/br_vlan.c
> >> @@ -144,6 +144,18 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
> >>  	if (!br->vlan_enabled)
> >>  		goto out;
> >>  
> >> +	/* Vlan filter table must be configrured at this point.  The
> >> +	 * only exception is the bridge is set in promisc mode and the
> >> +	 * packet is destined for the bridge device.  In this case
> >> +	 * pass the packet as is.
> >> +	 */
> >> +	if (!pv) {
> >> +       		if ((br->dev->flags & IFF_PROMISC) && skb->dev == br->dev)
> >> +			goto out;
> > 
> > Currently this function is calling __vlan_put_tag() if port_idx==0.
> > You are going to bypass it only if br is promisc mode.
> > 
> > However it doesn't seem to lead any problem because
> > __netif_receive_skb_core() can handle hw accel vlan tag.
> > 
> > Instead, I doubt if __vlan_put_tag() is necessary in br_handle_vlan()...
> 
> I'll need to re-test, but I remember having had to add that code to make
> it work on the bridge...
> 
> Looking at it again, I agree that special handling for forwarding to the
> bridge doesn't seem to be needed.
> 

Then, I'm OK with that "goto" statement :)

One more comment:
How about using BUG_ON() instead of "if-else BUG()"?
I think it could make the code slightly more efficient and simple.

Thanks,
Toshiaki Makita