[PATCH] openvswitch: fix a use after free

Discussion:

r***@gmail.com

2014-10-16 09:01:15 UTC

From: Li RongQing <***@gmail.com>

pskb_may_pull() called by arphdr_ok can change skb->data, so put the arp
setting after arphdr_ok to avoid the use the freed memory

Signed-off-by: Li RongQing <***@gmail.com>
---
net/openvswitch/flow.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 62db02b..b13ba5e 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -557,10 +557,11 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
} else if (key->eth.type == htons(ETH_P_ARP) ||
key->eth.type == htons(ETH_P_RARP)) {
struct arp_eth_header *arp;
+ bool arp_t = arphdr_ok(skb);

arp = (struct arp_eth_header *)skb_network_header(skb);

- if (arphdr_ok(skb) &&
+ if (arp_t &&
arp->ar_hrd == htons(ARPHRD_ETHER) &&
arp->ar_pro == htons(ETH_P_IP) &&
arp->ar_hln == ETH_ALEN &&

--
1.7.10.4

Eric Dumazet

2014-10-16 14:12:27 UTC

Permalink

Post by r***@gmail.com
pskb_may_pull() called by arphdr_ok can change skb->data, so put the arp
setting after arphdr_ok to avoid the use the freed memory
---
net/openvswitch/flow.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 62db02b..b13ba5e 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -557,10 +557,11 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
} else if (key->eth.type == htons(ETH_P_ARP) ||
key->eth.type == htons(ETH_P_RARP)) {
struct arp_eth_header *arp;
+ bool arp_t = arphdr_ok(skb);

This is a strange name for a bool. _t suffix is normally used by
typedef.

Post by r***@gmail.com
arp = (struct arp_eth_header *)skb_network_header(skb);
- if (arphdr_ok(skb) &&
+ if (arp_t &&
arp->ar_hrd == htons(ARPHRD_ETHER) &&
arp->ar_pro == htons(ETH_P_IP) &&
arp->ar_hln == ETH_ALEN &&

Fixes: 0714812134d7d ("openvswitch: Eliminate memset() from flow_extract.")

By doing this bug hunting, you also can CC bug author so that he knows
what happened. Maybe he will avoid similar mistakes in the future.

Jesse Gross

2014-10-16 14:58:49 UTC

Permalink

Post by Eric Dumazet

Fixes: 0714812134d7d ("openvswitch: Eliminate memset() from flow_extract.")
By doing this bug hunting, you also can CC bug author so that he knows
what happened. Maybe he will avoid similar mistakes in the future.

Yes, this one is my fault. Thanks for tracking it down.

r***@gmail.com

2014-10-17 06:03:08 UTC

Permalink

From: Li RongQing <***@gmail.com>

pskb_may_pull() called by arphdr_ok can change skb->data, so put the arp
setting after arphdr_ok to avoid the use the freed memory

Fixes: 0714812134d7d ("openvswitch: Eliminate memset() from flow_extract.")
Cc: Jesse Gross <***@nicira.com>
Cc: Eric Dumazet <***@google.com>
Signed-off-by: Li RongQing <***@gmail.com>
---
net/openvswitch/flow.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 62db02b..c5cfc72 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -557,10 +557,11 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
} else if (key->eth.type == htons(ETH_P_ARP) ||
key->eth.type == htons(ETH_P_RARP)) {
struct arp_eth_header *arp;
+ bool arp_available = arphdr_ok(skb);

arp = (struct arp_eth_header *)skb_network_header(skb);

- if (arphdr_ok(skb) &&
+ if (arp_available &&
arp->ar_hrd == htons(ARPHRD_ETHER) &&
arp->ar_pro == htons(ETH_P_IP) &&
arp->ar_hln == ETH_ALEN &&

--
1.7.10.4

Jesse Gross

2014-10-17 15:59:20 UTC

Permalink

Post by r***@gmail.com
pskb_may_pull() called by arphdr_ok can change skb->data, so put the arp
setting after arphdr_ok to avoid the use the freed memory
Fixes: 0714812134d7d ("openvswitch: Eliminate memset() from flow_extract.")

Acked-by: Jesse Gross <***@nicira.com>

David Miller

2014-10-17 20:23:08 UTC

Permalink

From: ***@gmail.com
Date: Fri, 17 Oct 2014 14:03:08 +0800

Applied, thanks.