gre: fix a regression in ioctl

PATCH Superseded Cong Wang 5 participants 12 messages Jun 29, 2013 – Jul 2, 2013

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Cong Wang

author

2013-06-29 02:04 UTC #0 lore raw

PATCH 1/1 gre: fix a regression in ioctl

From: Cong Wang <[email protected]>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.

So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

Cc Pravin B Shelar <[email protected]>

Cc "David S. Miller" <[email protected]>

Signed-off-by Cong Wang <[email protected]>

---

net/ipv4/ip_gre.c +0 -5

index c326e86..354d78c 100644
@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
-	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

net/ipv4/ip_tunnel.c +5 -0

index 394cebc..eb9dd96 100644
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
 	case SIOCADDTUNNEL:
 	case SIOCCHGTUNNEL:
+		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+		    ((p->i_flags|p->o_flags)&(GRE_VERSION|GRE_ROUTING)))
+			return -EINVAL;
+
 		err = -EPERM;
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			goto done;

Cong Wang

author

2013-06-29 02:16 UTC #1 lore raw

Quoted 31 lines · Cong Wang

From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:
 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.
So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.
After this patch I got:
 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 
Cc Pravin B Shelar <[email protected]>
Cc "David S. Miller" <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
net/ipv4/ip_gre.c +0 -0
index c326e86..354d78c 100644

net/ipv4/ip_gre.c static int ipgre_tunnel_ioctl(struct net_device *dev, · line 318

Quoted 10 lines

net/ipv4/ip_gre.c +0 -5

@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
-	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

Self-NAK. p.i_flags is converted to tunnel flags, therefore should probably check TUNNEL_* instead of GRE_*.

I will send v2.

Cong Wang

author

2013-06-29 02:24 UTC #2 lore raw

PATCH v2 1/1 gre: fix a regression in ioctl

In reply to Cong Wang

From: Cong Wang <[email protected]>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

Cc Pravin B Shelar <[email protected]>

Cc "David S. Miller" <[email protected]>

Signed-off-by Cong Wang <[email protected]>

---
v2: check TUNNEL_* flags

net/ipv4/ip_gre.c +0 -5

index c326e86..354d78c 100644
@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
-	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

net/ipv4/ip_tunnel.c +5 -0

index 394cebc..dc7d7ac 100644
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
 	case SIOCADDTUNNEL:
 	case SIOCCHGTUNNEL:
+		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+		    ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))
+			return -EINVAL;
+
 		err = -EPERM;
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			goto done;

Pravin Shelar

2013-06-29 03:28 UTC #3 lore raw

Quoted 4 lines · Cong Wang

From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:

Quoted 9 lines

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.

right, that API got changed. But these checks can not be added to generic ip_tunnel layer, which is also used by other tunnel modules. Can you keep that check in ip_gre module but do it only for add and del tunnel commands?

Quoted 46 lines

So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

Cc Pravin B Shelar <[email protected]>

Cc "David S. Miller" <[email protected]>

Signed-off-by Cong Wang <[email protected]>

---
v2: check TUNNEL_* flags

net/ipv4/ip_gre.c +0 -5

index c326e86..354d78c 100644
@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
        if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
                return -EFAULT;
-       if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-           p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-           ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-               return -EINVAL;
-       }
        p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
        p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

net/ipv4/ip_tunnel.c +5 -0

index 394cebc..dc7d7ac 100644
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
        case SIOCADDTUNNEL:
        case SIOCCHGTUNNEL:
+               if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+                   p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+                   ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))
+                       return -EINVAL;
+
                err = -EPERM;
                if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
                        goto done;

Cong Wang

author

2013-06-29 03:33 UTC #4 lore raw

Quoted 15 lines · Pravin Shelar

Quoted 4 lines · Cong Wang
From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:
Quoted 9 lines
 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.

Quoted 4 lines

right, that API got changed. But these checks can not be added to generic ip_tunnel layer, which is also used by other tunnel modules. Can you keep that check in ip_gre module but do it only for add and del tunnel commands?

Yeah.

BTW, ipip has the same bug... I will send a patch for ipip too.

Cong Wang

author

2013-06-29 03:55 UTC #5 lore raw

Quoted 1 line · Cong Wang

BTW, ipip has the same bug... I will send a patch for ipip too.

Pravin,

Any reason to add the following check for ipip?

       if (p.i_key || p.o_key || p.i_flags || p.o_flags)
               return -EINVAL;

It seems the old code doesn't have this check, you added it in commit fd58156e456d9f68fe04486be378d0bc93641532 (IPIP: Use ip-tunneling code.).

This breaks the API?

Cong Wang

author

2013-06-29 04:03 UTC #6 lore raw

PATCH v3 1/1 gre: fix a regression in ioctl

In reply to Pravin Shelar

From: Cong Wang <[email protected]>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

Cc Pravin B Shelar <[email protected]>

Cc "David S. Miller" <[email protected]>

Signed-off-by Cong Wang <[email protected]>

---
v2: check TUNNEL_* flags
v3: the check should be kept in gre module

net/ipv4/ip_gre.c +5 -4

index c326e86..1f6eab6 100644
@@ -314,10 +314,11 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
+	if (cmd == SIOCADDTUNNEL || cmd == SIOCCHGTUNNEL) {
+		if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
+		    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
+		    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING)))
+			return -EINVAL;
 	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

Dmitry Kravkov

2013-06-29 18:42 UTC #7 lore raw

Quoted 21 lines · Cong Wang

From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:
 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.
So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.
After this patch I got:
 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

Quoted 7 lines

Cc Pravin B Shelar <[email protected]>
Cc "David S. Miller" <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
v2: check TUNNEL_* flags
v3: the check should be kept in gre module

Shouldn't this be addressed in net-tree?

Cong Wang

author

2013-07-01 02:06 UTC #8 lore raw

Quoted 2 lines · Dmitry Kravkov

Shouldn't this be addressed in net-tree?

Oh, yeah, I thought only net-next contains that commit...

David, please apply this to net (I tried, the patch can be applied to net).

Thanks!

David Miller

maintainer

2013-07-02 06:36 UTC #9 lore raw

In reply to Cong Wang

From: Cong Wang <[email protected]>
Date: Sat, 29 Jun 2013 12:02:59 +0800

Quoted 17 lines

From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:
 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.
So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.
After this patch I got:

Quoted 7 lines

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 
Cc Pravin B Shelar <[email protected]>
Cc "David S. Miller" <[email protected]>
Signed-off-by Cong Wang <[email protected]>

Applied and queued up for -stable, thanks.

Sergei Shtylyov

2013-06-29 15:52 UTC #10 lore raw

In reply to Cong Wang

Hello.

Quoted 26 lines · Cong Wang

From: Cong Wang <[email protected]>
When testing GRE tunnel, I got:
  # ip tunnel show
  get tunnel gre0 failed: Invalid argument
  get tunnel gre1 failed: Invalid argument
This is a regression introduced by commit c54419321455631079c7d ("GRE: Refactor GRE tunneling code.") because previously we only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL, after that commit, the check is moved for all commands.
So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.
After this patch I got:
  # ip tunnel show
  gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
  gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit
Cc Pravin B Shelar <[email protected]>
Cc "David S. Miller" <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
v2: check TUNNEL_* flags

[...]

net/ipv4/ip_tunnel.c int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd) · line 717

Quoted 11 lines

net/ipv4/ip_tunnel.c +3 -0

index 394cebc..dc7d7ac 100644
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
  	case SIOCADDTUNNEL:
  	case SIOCCHGTUNNEL:
+		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+		    ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))

Maybe it's time to insert spaces around & to make the code formatted
consistently?

WBR, Sergei

Cong Wang

author

2013-07-01 02:08 UTC #11 lore raw

Quoted 3 lines · Sergei Shtylyov

Maybe it's time to insert spaces around & to make the code formatted
consistently?

Hi, Sergei

You are always welcome to send a _pure_ coding-style fix, not just for these few lines. ;)