[Patch net-next] ipip: fix a regression in ioctl

[Patch net-next] ipip: fix a regression in ioctl

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

This is a regression introduced by 
commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)

Similar to GRE tunnel, 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.

Also, the check for i_key, o_key etc. is suspicious too,
which did not exist before.

Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index e6905fb..9d6ca81 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -244,11 +244,11 @@ ipip_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
 
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
-			p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))
-		return -EINVAL;
-	if (p.i_key || p.o_key || p.i_flags || p.o_flags)
-		return -EINVAL;
+	if (cmd == SIOCADDTUNNEL || cmd == SIOCCHGTUNNEL) {
+		if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
+		    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))
+			return -EINVAL;
+	}
 	if (p.iph.ttl)
 		p.iph.frag_off |= htons(IP_DF);
 

Re: [Patch net-next] ipip: fix a regression in ioctl

[Sergei Shtylyov]

On 29-06-2013 8:11, Cong Wang wrote:

> From: Cong Wang <amwang@redhat.com>

> This is a regression introduced by
> commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)

> Similar to GRE tunnel, 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.

> Also, the check for i_key, o_key etc. is suspicious too,
> which did not exist before.

> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

> ---
> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index e6905fb..9d6ca81 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -244,11 +244,11 @@ ipip_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>   	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
>   		return -EFAULT;
>
> -	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
> -			p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))
> -		return -EINVAL;
> -	if (p.i_key || p.o_key || p.i_flags || p.o_flags)
> -		return -EINVAL;
> +	if (cmd == SIOCADDTUNNEL || cmd == SIOCCHGTUNNEL) {
> +		if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
> +		    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))

    Maybe it's time to put spaces around & to make code formatting more 
consistent here too?

WBR, Sergei

Re: [Patch net-next] ipip: fix a regression in ioctl

[Pravin Shelar]

On Fri, Jun 28, 2013 at 9:11 PM, Cong Wang <amwang@redhat.com> wrote:
> From: Cong Wang <amwang@redhat.com>
>
> This is a regression introduced by
> commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)
>
> Similar to GRE tunnel, 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.
>
> Also, the check for i_key, o_key etc. is suspicious too,
> which did not exist before.
>
This check is sanity check since ipip is not suppose to have these
parameters set, generic layer do allow all parameters.
Earlier ipip was not using generic layer, therefore that check was not present.

> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
>
> ---
> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index e6905fb..9d6ca81 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -244,11 +244,11 @@ ipip_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>         if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
>                 return -EFAULT;
>
> -       if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
> -                       p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))
> -               return -EINVAL;
> -       if (p.i_key || p.o_key || p.i_flags || p.o_flags)
> -               return -EINVAL;
> +       if (cmd == SIOCADDTUNNEL || cmd == SIOCCHGTUNNEL) {
> +               if (p.iph.version != 4 || p.iph.protocol != IPPROTO_IPIP ||
> +                   p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)))
> +                       return -EINVAL;
> +       }
>         if (p.iph.ttl)
>                 p.iph.frag_off |= htons(IP_DF);
>

Re: [Patch net-next] ipip: fix a regression in ioctl

[Cong Wang]

On Sat, 2013-06-29 at 20:43 -0700, Pravin Shelar wrote:
> On Fri, Jun 28, 2013 at 9:11 PM, Cong Wang <amwang@redhat.com> wrote:
> > From: Cong Wang <amwang@redhat.com>
> >
> > This is a regression introduced by
> > commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)
> >
> > Similar to GRE tunnel, 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.
> >
> > Also, the check for i_key, o_key etc. is suspicious too,
> > which did not exist before.
> >
> This check is sanity check since ipip is not suppose to have these
> parameters set, generic layer do allow all parameters.
> Earlier ipip was not using generic layer, therefore that check was not present.

So, if old code doesn't reject this case with EINVAL, then your change
_does_ break user-space applications... no matter whether ipip is
supposed to have these parameters.

Re: [Patch net-next] ipip: fix a regression in ioctl

[Pravin Shelar]

On Sun, Jun 30, 2013 at 7:11 PM, Cong Wang <amwang@redhat.com> wrote:
> On Sat, 2013-06-29 at 20:43 -0700, Pravin Shelar wrote:
>> On Fri, Jun 28, 2013 at 9:11 PM, Cong Wang <amwang@redhat.com> wrote:
>> > From: Cong Wang <amwang@redhat.com>
>> >
>> > This is a regression introduced by
>> > commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)
>> >
>> > Similar to GRE tunnel, 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.
>> >
>> > Also, the check for i_key, o_key etc. is suspicious too,
>> > which did not exist before.
>> >
>> This check is sanity check since ipip is not suppose to have these
>> parameters set, generic layer do allow all parameters.
>> Earlier ipip was not using generic layer, therefore that check was not present.
>
> So, if old code doesn't reject this case with EINVAL, then your change
> _does_ break user-space applications... no matter whether ipip is
> supposed to have these parameters.
>
ok, Then we shld reset these fields before passing them to ip_tunnels layer.

>

Re: [Patch net-next] ipip: fix a regression in ioctl

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Mon, 1 Jul 2013 08:03:33 -0700

> On Sun, Jun 30, 2013 at 7:11 PM, Cong Wang <amwang@redhat.com> wrote:
>> On Sat, 2013-06-29 at 20:43 -0700, Pravin Shelar wrote:
>>> On Fri, Jun 28, 2013 at 9:11 PM, Cong Wang <amwang@redhat.com> wrote:
>>> > From: Cong Wang <amwang@redhat.com>
>>> >
>>> > This is a regression introduced by
>>> > commit fd58156e456d9f68fe0448 (IPIP: Use ip-tunneling code.)
>>> >
>>> > Similar to GRE tunnel, 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.
>>> >
>>> > Also, the check for i_key, o_key etc. is suspicious too,
>>> > which did not exist before.
>>> >
>>> This check is sanity check since ipip is not suppose to have these
>>> parameters set, generic layer do allow all parameters.
>>> Earlier ipip was not using generic layer, therefore that check was not present.
>>
>> So, if old code doesn't reject this case with EINVAL, then your change
>> _does_ break user-space applications... no matter whether ipip is
>> supposed to have these parameters.
>>
> ok, Then we shld reset these fields before passing them to ip_tunnels layer.

Someone please respin this to clear the fields instead, thanks!

Re: [Patch net-next] ipip: fix a regression in ioctl

[Cong Wang]

On Mon, 2013-07-01 at 23:36 -0700, David Miller wrote:
> From: Pravin Shelar <pshelar@nicira.com>
> >>
> > ok, Then we shld reset these fields before passing them to ip_tunnels layer.
> 
> Someone please respin this to clear the fields instead, thanks!

I will send v2.

Thanks!