| Message ID | 20130815125257.GA16932@elgon.mountain |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: > The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is > not totally correct. Because "len" and "sizeof()" are size_t type, that > means they are never less than zero. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index af987f0..7ed13cc 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > u32 rxhash; > > if (!(tun->flags & TUN_NO_PI)) { > - if ((len -= sizeof(pi)) < 0) > + if (len < sizeof(pi)) > return -EINVAL; > + len -= sizeof(pi); > > if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) > return -EFAULT; > @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > } > > if (tun->flags & TUN_VNET_HDR) { > - if ((len -= tun->vnet_hdr_sz) < 0) > + if (len < tun->vnet_hdr_sz) > return -EINVAL; > + len -= tun->vnet_hdr_sz; > > if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) > return -EFAULT; -- 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 Thu, Aug 15, 2013 at 05:04:49PM +0300, Michael S. Tsirkin wrote: > On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: > > The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is > > not totally correct. Because "len" and "sizeof()" are size_t type, that > > means they are never less than zero. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> Alternatively how about we revert the original patch? This is not the only issue it introduced and it doesn't actually fix any bugs. > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index af987f0..7ed13cc 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > u32 rxhash; > > > > if (!(tun->flags & TUN_NO_PI)) { > > - if ((len -= sizeof(pi)) < 0) > > + if (len < sizeof(pi)) > > return -EINVAL; > > + len -= sizeof(pi); > > > > if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) > > return -EFAULT; > > @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > } > > > > if (tun->flags & TUN_VNET_HDR) { > > - if ((len -= tun->vnet_hdr_sz) < 0) > > + if (len < tun->vnet_hdr_sz) > > return -EINVAL; > > + len -= tun->vnet_hdr_sz; > > > > if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) > > return -EFAULT; -- 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 Thu, Aug 15, 2013 at 05:58:40PM +0300, Michael S. Tsirkin wrote: > On Thu, Aug 15, 2013 at 05:04:49PM +0300, Michael S. Tsirkin wrote: > > On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: > > > The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is > > > not totally correct. Because "len" and "sizeof()" are size_t type, that > > > means they are never less than zero. > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Alternatively how about we revert the original patch? > This is not the only issue it introduced and it doesn't > actually fix any bugs. > > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > index af987f0..7ed13cc 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > > u32 rxhash; > > > > > > if (!(tun->flags & TUN_NO_PI)) { > > > - if ((len -= sizeof(pi)) < 0) > > > + if (len < sizeof(pi)) > > > return -EINVAL; > > > + len -= sizeof(pi); > > > > > > if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) > > > return -EFAULT; > > > @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > > } > > > > > > if (tun->flags & TUN_VNET_HDR) { > > > - if ((len -= tun->vnet_hdr_sz) < 0) > > > + if (len < tun->vnet_hdr_sz) > > > return -EINVAL; And to be even more explicit, this still doesn't handle the case vnet_hdr_sz < 0 properly. > > > + len -= tun->vnet_hdr_sz; > > > > > > if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) > > > return -EFAULT; -- 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 Thu, Aug 15, 2013 at 06:02:14PM +0300, Michael S. Tsirkin wrote: > On Thu, Aug 15, 2013 at 05:58:40PM +0300, Michael S. Tsirkin wrote: > > On Thu, Aug 15, 2013 at 05:04:49PM +0300, Michael S. Tsirkin wrote: > > > On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: > > > > The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is > > > > not totally correct. Because "len" and "sizeof()" are size_t type, that > > > > means they are never less than zero. > > > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > Alternatively how about we revert the original patch? > > This is not the only issue it introduced and it doesn't > > actually fix any bugs. > > > > > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > > index af987f0..7ed13cc 100644 > > > > --- a/drivers/net/tun.c > > > > +++ b/drivers/net/tun.c > > > > @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > > > u32 rxhash; > > > > > > > > if (!(tun->flags & TUN_NO_PI)) { > > > > - if ((len -= sizeof(pi)) < 0) > > > > + if (len < sizeof(pi)) > > > > return -EINVAL; > > > > + len -= sizeof(pi); > > > > > > > > if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) > > > > return -EFAULT; > > > > @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > > > } > > > > > > > > if (tun->flags & TUN_VNET_HDR) { > > > > - if ((len -= tun->vnet_hdr_sz) < 0) > > > > + if (len < tun->vnet_hdr_sz) > > > > return -EINVAL; > > And to be even more explicit, this still doesn't handle the > case vnet_hdr_sz < 0 properly. Hmm ENOCOFFEE. User can't make vnet_hdr_sz < 0 - we already catch that. So let's apply Dan's patch, it does fix all issues after all. Sorry about the noise. > > > > > + len -= tun->vnet_hdr_sz; > > > > > > > > if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) > > > > return -EFAULT; -- 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 Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: > The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is > not totally correct. Because "len" and "sizeof()" are size_t type, that > means they are never less than zero. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index af987f0..7ed13cc 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > u32 rxhash; > > if (!(tun->flags & TUN_NO_PI)) { > - if ((len -= sizeof(pi)) < 0) > + if (len < sizeof(pi)) > return -EINVAL; > + len -= sizeof(pi); > > if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) > return -EFAULT; > @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > } > > if (tun->flags & TUN_VNET_HDR) { > - if ((len -= tun->vnet_hdr_sz) < 0) > + if (len < tun->vnet_hdr_sz) > return -EINVAL; > + len -= tun->vnet_hdr_sz; > > if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) > return -EFAULT; > Acked-by: Neil Horman <nhorman@tuxdriver.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
From: Neil Horman <nhorman@tuxdriver.com> Date: Thu, 15 Aug 2013 16:08:35 -0400 > On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote: >> The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is >> not totally correct. Because "len" and "sizeof()" are size_t type, that >> means they are never less than zero. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> ... > Acked-by: Neil Horman <nhorman@tuxdriver.com> Applied, thanks everyone. -- 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/drivers/net/tun.c b/drivers/net/tun.c index af987f0..7ed13cc 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, u32 rxhash; if (!(tun->flags & TUN_NO_PI)) { - if ((len -= sizeof(pi)) < 0) + if (len < sizeof(pi)) return -EINVAL; + len -= sizeof(pi); if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi))) return -EFAULT; @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, } if (tun->flags & TUN_VNET_HDR) { - if ((len -= tun->vnet_hdr_sz) < 0) + if (len < tun->vnet_hdr_sz) return -EINVAL; + len -= tun->vnet_hdr_sz; if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso))) return -EFAULT;
The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.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