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

Submitted by Amerigo Wang on June 29, 2013, 4:11 a.m.

Details

Message ID 1372479115-7003-1-git-send-email-amwang@redhat.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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