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

login
register
mail settings
Submitter Amerigo Wang
Date June 29, 2013, 4:11 a.m.
Message ID <1372479115-7003-1-git-send-email-amwang@redhat.com>
Download mbox | patch
Permalink /patch/255716/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Amerigo Wang - June 29, 2013, 4:11 a.m.
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>

---
--
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
Sergei Shtylyov - June 29, 2013, 3:58 p.m.
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

--
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
Pravin B Shelar - June 30, 2013, 3:43 a.m.
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);
>
--
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
Amerigo Wang - July 1, 2013, 2:11 a.m.
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.


--
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
Pravin B Shelar - July 1, 2013, 3:03 p.m.
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.

>
--
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 - July 2, 2013, 6:36 a.m.
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!
--
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
Amerigo Wang - July 2, 2013, 6:38 a.m.
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!

--
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/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);