Message ID | 498166FB.5030104@gmail.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
Hi, On Thu, Jan 29, 2009 at 09:21:15AM +0100, Roel Kluin wrote: > Several functions with something like this occur: > > int sock_set_foo(int optlen, ...) > { > struct food foo; > > if (optlen < sizeof(foo)) > return -EINVAL; > > if (copy_from_user(&foo, optval, sizeof(foo))) > return -EFAULT; > ... > } > > see for instance: > grep -C5 -E -R -n "copy_from_user\(&([a-zA-Z0-9]*), optval, sizeof\(\1\)\)" net > > but in __dn_setsockopt, below, the checks are slightly different. > Should maybe the changes below be apllied? > > -------------->8----------------8<----------------------- > fix size checks before copy_from_user > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > diff --git a/net/decnet/af_decnet.c b/net/decnet/af_decnet.c > index cf0e184..45b9199 100644 > --- a/net/decnet/af_decnet.c > +++ b/net/decnet/af_decnet.c > @@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us > if (optlen && !optval) > return -EINVAL; > > - if (optlen > sizeof(u)) > + if (optlen < sizeof(u)) > return -EINVAL; > I don't see that this makes sense... we want to ensure that the passed length is less than the size of the union which we are going to use as a buffer. > - if (copy_from_user(&u, optval, optlen)) > + if (copy_from_user(&u, optval, sizeof(u))) > return -EFAULT; > ... and here we only want to copy the amount of data that has actually been supplied, not the whole buffer size since in many cases the amount of data is less than the total buffer size. The only sensible addition that I can see, would be to zero out the buffer before the copy to ensure that we don't land up using data from the stack in the case that the supplied data is less than that required for a particular command. Steve. -- 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: steve@chygwyn.com Date: Thu, 29 Jan 2009 08:58:58 +0000 > On Thu, Jan 29, 2009 at 09:21:15AM +0100, Roel Kluin wrote: > > @@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us > > if (optlen && !optval) > > return -EINVAL; > > > > - if (optlen > sizeof(u)) > > + if (optlen < sizeof(u)) > > return -EINVAL; > > > I don't see that this makes sense... we want to ensure that the passed > length is less than the size of the union which we are going to use > as a buffer. > > > - if (copy_from_user(&u, optval, optlen)) > > + if (copy_from_user(&u, optval, sizeof(u))) > > return -EFAULT; > > > ... and here we only want to copy the amount of data that has actually > been supplied, not the whole buffer size since in many cases the > amount of data is less than the total buffer size. This code is fine as-is, every single case statement below this code makes an explicit optlen equality check before deferencing any member of the union object we copy into. I'm therefore dropping this patch. -- 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/decnet/af_decnet.c b/net/decnet/af_decnet.c index cf0e184..45b9199 100644 --- a/net/decnet/af_decnet.c +++ b/net/decnet/af_decnet.c @@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us if (optlen && !optval) return -EINVAL; - if (optlen > sizeof(u)) + if (optlen < sizeof(u)) return -EINVAL; - if (copy_from_user(&u, optval, optlen)) + if (copy_from_user(&u, optval, sizeof(u))) return -EFAULT; switch(optname) {
Several functions with something like this occur: int sock_set_foo(int optlen, ...) { struct food foo; if (optlen < sizeof(foo)) return -EINVAL; if (copy_from_user(&foo, optval, sizeof(foo))) return -EFAULT; ... } see for instance: grep -C5 -E -R -n "copy_from_user\(&([a-zA-Z0-9]*), optval, sizeof\(\1\)\)" net but in __dn_setsockopt, below, the checks are slightly different. Should maybe the changes below be apllied? -------------->8----------------8<----------------------- fix size checks before copy_from_user Signed-off-by: Roel Kluin <roel.kluin@gmail.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