| Message ID | 1372479115-7003-1-git-send-email-amwang@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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);