openvswitch: fix a use after free

PATCH In review [email protected] 3 participants 3 messages Oct 16, 2014

Subsystems (3)

Networking Network drivers Openvswitch

lore

Share & tools

lore

[email protected]

author

2014-10-16 09:01 UTC #0 lore raw

PATCH 1/1 openvswitch: fix a use after free

From: Li RongQing <[email protected]>

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 <[email protected]>

---
 net/openvswitch/flow.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/openvswitch/flow.c +2 -1

index 62db02b..b13ba5e 100644
@@ -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 &&

Signature · 1 line

1.7.10.4

Eric Dumazet

2014-10-16 14:12 UTC #1 lore raw

Quoted 15 lines · On Thu, 2014-10-16 at 17:01 +0800, [email protected] wrote:

From: Li RongQing <[email protected]>
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 <[email protected]>
---
 net/openvswitch/flow.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/openvswitch/flow.c +0 -0
index 62db02b..b13ba5e 100644

net/openvswitch/flow.c static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) · line 560

Quoted 4 lines

net/openvswitch/flow.c +1 -0

@@ -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.

net/openvswitch/flow.c static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) · line 567

Quoted 8 lines

net/openvswitch/flow.c +1 -1

 		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:59 UTC #2 lore raw

Quoted 10 lines · Eric Dumazet

net/openvswitch/flow.c static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) · line 567
Quoted 8 lines · On Thu, 2014-10-16 at 17:01 +0800, [email protected] wrote:
              arp = (struct arp_eth_header *)skb_network_header(skb);
net/openvswitch/flow.c +1 -1
-             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 &&

Quoted 4 lines

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.