* [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
@ 2014-01-22 18:24 Vlad Yasevich
2014-01-23 12:38 ` Toshiaki Makita
0 siblings, 1 reply; 6+ messages in thread
From: Vlad Yasevich @ 2014-01-22 18:24 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich, Toshiaki Makita
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 Oops 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>
---
Changes since v1:
- Do not use a BUG or BUG_ON as it is possible to trigger the
false BUG condition when thrashing the vlan_enable toggle.
Instead just drop the skb.
net/bridge/br_input.c | 11 ++++++-----
net/bridge/br_vlan.c | 14 ++++++++++++++
2 files changed, 20 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..abc841c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -144,6 +144,20 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
if (!br->vlan_enabled)
goto out;
+ /* Vlan filter table must be configured 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 {
+ kfree_skb_list(skb);
+ return NULL;
+ }
+ }
+
/* 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
2014-01-22 18:24 [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump Vlad Yasevich
@ 2014-01-23 12:38 ` Toshiaki Makita
2014-01-23 14:57 ` Vlad Yasevich
0 siblings, 1 reply; 6+ messages in thread
From: Toshiaki Makita @ 2014-01-23 12:38 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
On Wed, 2014-01-22 at 13:24 -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 Oops 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>
> ---
> Changes since v1:
> - Do not use a BUG or BUG_ON as it is possible to trigger the
> false BUG condition when thrashing the vlan_enable toggle.
> Instead just drop the skb.
Maybe that frame went through should_deliver() while vlan_filtering was
disabled?
I hope it doesn't affect other codes in an adverse way...
>
> net/bridge/br_input.c | 11 ++++++-----
> net/bridge/br_vlan.c | 14 ++++++++++++++
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
...
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..abc841c 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -144,6 +144,20 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
> if (!br->vlan_enabled)
> goto out;
>
> + /* Vlan filter table must be configured 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 {
> + kfree_skb_list(skb);
Why is this not kfree_skb() but kfree_skb_list()?
I haven't seen kfree_skb_list() in the bridge code.
Thanks,
Toshiaki Makita
> + return NULL;
> + }
> + }
> +
> /* 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
2014-01-23 12:38 ` Toshiaki Makita
@ 2014-01-23 14:57 ` Vlad Yasevich
0 siblings, 0 replies; 6+ messages in thread
From: Vlad Yasevich @ 2014-01-23 14:57 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: netdev
On 01/23/2014 07:38 AM, Toshiaki Makita wrote:
> On Wed, 2014-01-22 at 13:24 -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 Oops 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>
>> ---
>> Changes since v1:
>> - Do not use a BUG or BUG_ON as it is possible to trigger the
>> false BUG condition when thrashing the vlan_enable toggle.
>> Instead just drop the skb.
>
> Maybe that frame went through should_deliver() while vlan_filtering was
> disabled?
yes. That's what happend.
> I hope it doesn't affect other codes in an adverse way...
I didn't see any adverse conditions while running the while(1) loop
changing vlan_flitering, but I didn't run it for all that long (about a
minute).
I am working on a series to reduce the possible races that involves
marking the skb (in the cb) and using that marking in later invocations.
>
>>
>> net/bridge/br_input.c | 11 ++++++-----
>> net/bridge/br_vlan.c | 14 ++++++++++++++
>> 2 files changed, 20 insertions(+), 5 deletions(-)
>>
> ...
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index af5ebd1..abc841c 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -144,6 +144,20 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>> if (!br->vlan_enabled)
>> goto out;
>>
>> + /* Vlan filter table must be configured 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 {
>> + kfree_skb_list(skb);
>
> Why is this not kfree_skb() but kfree_skb_list()?
> I haven't seen kfree_skb_list() in the bridge code.
I thought it is possible to get a list here, but upon further inspection
I don't see that happening. I'll change that to kfree_skb().
Thanks
-vlad
>
> Thanks,
> Toshiaki Makita
>
>> + return NULL;
>> + }
>> + }
>> +
>> /* 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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
2014-03-28 1:51 Vlad Yasevich
2014-03-28 3:33 ` Toshiaki Makita
@ 2014-03-28 21:14 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2014-03-28 21:14 UTC (permalink / raw)
To: vyasevic; +Cc: netdev, bridge
From: Vlad Yasevich <vyasevic@redhat.com>
Date: Thu, 27 Mar 2014 21:51:18 -0400
> 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 Oops 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>
> ---
>
> * Changed to use kfree_skb() instead of kfree_skb_list() to
> match the reset of bridge code.
> * Fix-up {} style.
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
2014-03-28 1:51 Vlad Yasevich
@ 2014-03-28 3:33 ` Toshiaki Makita
2014-03-28 21:14 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Toshiaki Makita @ 2014-03-28 3:33 UTC (permalink / raw)
To: Vlad Yasevich, netdev; +Cc: bridge
(2014/03/28 10:51), 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 Oops 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>
Thanks,
Acked-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump
@ 2014-03-28 1:51 Vlad Yasevich
2014-03-28 3:33 ` Toshiaki Makita
2014-03-28 21:14 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Vlad Yasevich @ 2014-03-28 1:51 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich, bridge
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 Oops 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>
---
* Changed to use kfree_skb() instead of kfree_skb_list() to
match the reset of bridge code.
* Fix-up {} style.
net/bridge/br_input.c | 11 ++++++-----
net/bridge/br_vlan.c | 14 ++++++++++++++
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 28d5446..d0cca3c 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 pcpu_sw_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 8249ca7..b153cc4 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -144,6 +144,20 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
if (!br->vlan_enabled)
goto out;
+ /* Vlan filter table must be configured 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 {
+ kfree_skb(skb);
+ return NULL;
+ }
+ }
+
/* 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 tagged.
--
1.8.5.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-28 21:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-22 18:24 [PATCH v2] bridge: Fix crash with vlan filtering and tcpdump Vlad Yasevich
2014-01-23 12:38 ` Toshiaki Makita
2014-01-23 14:57 ` Vlad Yasevich
2014-03-28 1:51 Vlad Yasevich
2014-03-28 3:33 ` Toshiaki Makita
2014-03-28 21:14 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).