| Message ID | 1559650290-17054-1-git-send-email-92siuyang@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | rose: af_rose: avoid overflows in rose_setsockopt() | expand |
Am 04.06.2019 14:11, schrieb Young Xiao: > Check setsockopt arguments to avoid overflows and return -EINVAL for > too large arguments. > > See commit 32288eb4d940 ("netrom: avoid overflows in nr_setsockopt()") > for details. > > Signed-off-by: Young Xiao <92siuyang@gmail.com> > --- > net/rose/af_rose.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c > index e274bc6..af831ee9 100644 > --- a/net/rose/af_rose.c > +++ b/net/rose/af_rose.c > @@ -372,15 +372,15 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, > { > struct sock *sk = sock->sk; > struct rose_sock *rose = rose_sk(sk); > - int opt; > + unsigned long opt; > > if (level != SOL_ROSE) > return -ENOPROTOOPT; > > - if (optlen < sizeof(int)) > + if (optlen < sizeof(unsigned int)) > return -EINVAL; I do not thing that this will change much, but maybe you would like to check against the sizeof (opt) here ? > > - if (get_user(opt, (int __user *)optval)) > + if (get_user(opt, (unsigned int __user *)optval)) > return -EFAULT; > > switch (optname) { > @@ -389,31 +389,31 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, > return 0; > > case ROSE_T1: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t1 = opt * HZ; > return 0; > > case ROSE_T2: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t2 = opt * HZ; > return 0; > > case ROSE_T3: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t3 = opt * HZ; > return 0; > > case ROSE_HOLDBACK: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->hb = opt * HZ; > return 0; you can simplify this jungle by checking and calculation first and then set the correct rose->XX > > case ROSE_IDLE: > - if (opt < 0) > + if (opt < 0 || opt > ULONG_MAX / HZ) > return -EINVAL; You made opt unsigned or ? > rose->idle = opt * 60 * HZ; > return 0; my 2 cents, re, wh
On 6/4/19 5:11 AM, Young Xiao wrote: > Check setsockopt arguments to avoid overflows and return -EINVAL for > too large arguments. > > See commit 32288eb4d940 ("netrom: avoid overflows in nr_setsockopt()") > for details. > > Signed-off-by: Young Xiao <92siuyang@gmail.com> > --- > net/rose/af_rose.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c > index e274bc6..af831ee9 100644 > --- a/net/rose/af_rose.c > +++ b/net/rose/af_rose.c > @@ -372,15 +372,15 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, > { > struct sock *sk = sock->sk; > struct rose_sock *rose = rose_sk(sk); > - int opt; > + unsigned long opt; > > if (level != SOL_ROSE) > return -ENOPROTOOPT; > > - if (optlen < sizeof(int)) > + if (optlen < sizeof(unsigned int)) > return -EINVAL; > > - if (get_user(opt, (int __user *)optval)) > + if (get_user(opt, (unsigned int __user *)optval)) > return -EFAULT; > > switch (optname) { > @@ -389,31 +389,31 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, > return 0; > > case ROSE_T1: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t1 = opt * HZ; > return 0; > > case ROSE_T2: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t2 = opt * HZ; > return 0; > > case ROSE_T3: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->t3 = opt * HZ; > return 0; > > case ROSE_HOLDBACK: > - if (opt < 1) > + if (opt < 1 || opt > ULONG_MAX / HZ) > return -EINVAL; > rose->hb = opt * HZ; > return 0; > > case ROSE_IDLE: > - if (opt < 0) > + if (opt < 0 || opt > ULONG_MAX / HZ) Buggy check. > return -EINVAL; > rose->idle = opt * 60 * HZ; > return 0; >
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c index e274bc6..af831ee9 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c @@ -372,15 +372,15 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, { struct sock *sk = sock->sk; struct rose_sock *rose = rose_sk(sk); - int opt; + unsigned long opt; if (level != SOL_ROSE) return -ENOPROTOOPT; - if (optlen < sizeof(int)) + if (optlen < sizeof(unsigned int)) return -EINVAL; - if (get_user(opt, (int __user *)optval)) + if (get_user(opt, (unsigned int __user *)optval)) return -EFAULT; switch (optname) { @@ -389,31 +389,31 @@ static int rose_setsockopt(struct socket *sock, int level, int optname, return 0; case ROSE_T1: - if (opt < 1) + if (opt < 1 || opt > ULONG_MAX / HZ) return -EINVAL; rose->t1 = opt * HZ; return 0; case ROSE_T2: - if (opt < 1) + if (opt < 1 || opt > ULONG_MAX / HZ) return -EINVAL; rose->t2 = opt * HZ; return 0; case ROSE_T3: - if (opt < 1) + if (opt < 1 || opt > ULONG_MAX / HZ) return -EINVAL; rose->t3 = opt * HZ; return 0; case ROSE_HOLDBACK: - if (opt < 1) + if (opt < 1 || opt > ULONG_MAX / HZ) return -EINVAL; rose->hb = opt * HZ; return 0; case ROSE_IDLE: - if (opt < 0) + if (opt < 0 || opt > ULONG_MAX / HZ) return -EINVAL; rose->idle = opt * 60 * HZ; return 0;
Check setsockopt arguments to avoid overflows and return -EINVAL for too large arguments. See commit 32288eb4d940 ("netrom: avoid overflows in nr_setsockopt()") for details. Signed-off-by: Young Xiao <92siuyang@gmail.com> --- net/rose/af_rose.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)