* [patch] tun: signedness bug in tun_get_user() @ 2013-08-15 12:52 Dan Carpenter 2013-08-15 14:04 ` Michael S. Tsirkin 2013-08-15 20:08 ` Neil Horman 0 siblings, 2 replies; 7+ messages in thread From: Dan Carpenter @ 2013-08-15 12:52 UTC (permalink / raw) To: David S. Miller Cc: Jason Wang, Michael S. Tsirkin, Eric Dumazet, Neil Horman, netdev, kernel-janitors 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 12:52 [patch] tun: signedness bug in tun_get_user() Dan Carpenter @ 2013-08-15 14:04 ` Michael S. Tsirkin 2013-08-15 14:58 ` Michael S. Tsirkin 2013-08-15 20:08 ` Neil Horman 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2013-08-15 14:04 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jason Wang, Eric Dumazet, Neil Horman, netdev, kernel-janitors 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 14:04 ` Michael S. Tsirkin @ 2013-08-15 14:58 ` Michael S. Tsirkin 2013-08-15 15:02 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2013-08-15 14:58 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jason Wang, Eric Dumazet, Neil Horman, netdev, kernel-janitors 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 14:58 ` Michael S. Tsirkin @ 2013-08-15 15:02 ` Michael S. Tsirkin 2013-08-15 15:05 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2013-08-15 15:02 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jason Wang, Eric Dumazet, Neil Horman, netdev, kernel-janitors 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 15:02 ` Michael S. Tsirkin @ 2013-08-15 15:05 ` Michael S. Tsirkin 0 siblings, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2013-08-15 15:05 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jason Wang, Eric Dumazet, Neil Horman, netdev, kernel-janitors 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 12:52 [patch] tun: signedness bug in tun_get_user() Dan Carpenter 2013-08-15 14:04 ` Michael S. Tsirkin @ 2013-08-15 20:08 ` Neil Horman 2013-08-15 21:51 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: Neil Horman @ 2013-08-15 20:08 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jason Wang, Michael S. Tsirkin, Eric Dumazet, netdev, kernel-janitors 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] tun: signedness bug in tun_get_user() 2013-08-15 20:08 ` Neil Horman @ 2013-08-15 21:51 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2013-08-15 21:51 UTC (permalink / raw) To: nhorman; +Cc: dan.carpenter, jasowang, mst, edumazet, netdev, kernel-janitors 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-08-15 21:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-15 12:52 [patch] tun: signedness bug in tun_get_user() Dan Carpenter 2013-08-15 14:04 ` Michael S. Tsirkin 2013-08-15 14:58 ` Michael S. Tsirkin 2013-08-15 15:02 ` Michael S. Tsirkin 2013-08-15 15:05 ` Michael S. Tsirkin 2013-08-15 20:08 ` Neil Horman 2013-08-15 21:51 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).