Patchwork ipv4/ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally

login
register
mail settings
Submitter Isaku Yamahata
Date Dec. 21, 2012, 1:12 a.m.
Message ID <9ee0cd8f94e1ee866b40ee7b6755e8d8705325c9.1356052319.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/207751/
State Accepted
Delegated to: David Miller
Headers show

Comments

Isaku Yamahata - Dec. 21, 2012, 1:12 a.m.
ipgre_tunnel_xmit() parses network header as IP unconditionally.
But transmitting packets are not always IP packet. For example such packet
can be sent by packet socket with sockaddr_ll.sll_protocol set.
So make the function check if skb->protocol is IP.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 net/ipv4/ip_gre.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Eric Dumazet - Dec. 21, 2012, 1:48 a.m.
On Fri, 2012-12-21 at 10:12 +0900, Isaku Yamahata wrote:
> ipgre_tunnel_xmit() parses network header as IP unconditionally.
> But transmitting packets are not always IP packet. For example such packet
> can be sent by packet socket with sockaddr_ll.sll_protocol set.
> So make the function check if skb->protocol is IP.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  net/ipv4/ip_gre.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index a85ae2f..8fcf0ed 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -760,7 +760,10 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
>  
>  	if (dev->header_ops && dev->type == ARPHRD_IPGRE) {
>  		gre_hlen = 0;
> -		tiph = (const struct iphdr *)skb->data;
> +		if (skb->protocol == htons(ETH_P_IP))
> +			tiph = (const struct iphdr *)skb->data;
> +		else
> +			tiph = &tunnel->parms.iph;
>  	} else {
>  		gre_hlen = tunnel->hlen;
>  		tiph = &tunnel->parms.iph;

Seems good to me thanks !

Acked-by: Eric Dumazet <edumazet@google.com>

BTW, it seems another bug exists at line 931 : We dereference tiph while
it could point to freed memory because of the skb_realloc_headroom() at
line 893

I'll send a patch.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 21, 2012, 9:15 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 20 Dec 2012 17:48:41 -0800

> On Fri, 2012-12-21 at 10:12 +0900, Isaku Yamahata wrote:
>> ipgre_tunnel_xmit() parses network header as IP unconditionally.
>> But transmitting packets are not always IP packet. For example such packet
>> can be sent by packet socket with sockaddr_ll.sll_protocol set.
>> So make the function check if skb->protocol is IP.
>> 
>> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> ---
>>  net/ipv4/ip_gre.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> index a85ae2f..8fcf0ed 100644
>> --- a/net/ipv4/ip_gre.c
>> +++ b/net/ipv4/ip_gre.c
>> @@ -760,7 +760,10 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
>>  
>>  	if (dev->header_ops && dev->type == ARPHRD_IPGRE) {
>>  		gre_hlen = 0;
>> -		tiph = (const struct iphdr *)skb->data;
>> +		if (skb->protocol == htons(ETH_P_IP))
>> +			tiph = (const struct iphdr *)skb->data;
>> +		else
>> +			tiph = &tunnel->parms.iph;
>>  	} else {
>>  		gre_hlen = tunnel->hlen;
>>  		tiph = &tunnel->parms.iph;
> 
> Seems good to me thanks !
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index a85ae2f..8fcf0ed 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -760,7 +760,10 @@  static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
 
 	if (dev->header_ops && dev->type == ARPHRD_IPGRE) {
 		gre_hlen = 0;
-		tiph = (const struct iphdr *)skb->data;
+		if (skb->protocol == htons(ETH_P_IP))
+			tiph = (const struct iphdr *)skb->data;
+		else
+			tiph = &tunnel->parms.iph;
 	} else {
 		gre_hlen = tunnel->hlen;
 		tiph = &tunnel->parms.iph;