Hello,

On 1/15/25 18:04, Su Hui wrote:

On 2025/1/16 07:29, David Laight wrote:

...

On Wed, 15 Jan 2025 08:42:20 -0800 Nikita Zhandarovich n.zhandarovich@fintech.ru wrote:

...

In case of possible unpredictably large arguments passed to rose_setsockopt() and multiplied by extra values on top of that, integer overflows may occur.

Do the safest minimum and fix these issues by checking the contents of 'opt' and returning -EINVAL if they are too large. Also, switch to unsigned int and remove useless check for negative 'opt' in ROSE_IDLE case.

Found by Linux Verification Center (linuxtesting.org) with static analysis tool SVACE.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Nikita Zhandarovich n.zhandarovich@fintech.ru

---

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 59050caab65c..72c65d938a15 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c @@ -397,15 +397,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 int opt;         if (level != SOL_ROSE)           return -ENOPROTOOPT;   -    if (optlen < sizeof(int)) +    if (optlen < sizeof(unsigned int))           return -EINVAL;   -    if (copy_from_sockptr(&opt, optval, sizeof(int))) +    if (copy_from_sockptr(&opt, optval, sizeof(unsigned int)))

Shouldn't all those be 'sizeof (opt)' ?

David

Agreed, but my thinking was to keep it somewhat symmetrical to other similar checks in XXX_setsockopt(). For instance, in net/ax25/af_ax25.c, courtesy of commit 7b75c5a8c41 ("net: pass a sockptr_t into ->setsockopt") an explicit type is used.

I don't mind sending v2, as it would be a bit neater.

...

...

return -EFAULT;         switch (optname) { @@ -414,31 +414,31 @@ static int rose_setsockopt(struct socket *sock, int level, int optname,           return 0;         case ROSE_T1: -        if (opt < 1) +        if (opt < 1 || opt > UINT_MAX / HZ)

'rose->t1' is unsigned long, how about 'opt > ULONG_MAX / HZ' ?

BTW, I think only in 32bit or 16bit machine when 'sizeof(int) == sizeof(unsigned long)', this integer overflows may occur..

Su Hui

Here I was influenced by commits dc35616e6c29 ("netrom: fix api breakage in nr_setsockopt()") and 9371937092d5 ("ax25: uninitialized variable in ax25_setsockopt()") that essentially state that we only copy 4 bytes from userspace so opt being ulong is not desired. Even if the result of * HZ ends up stored in ulong 'XXX->t1'.

I may be wrong but I think same principle applies to rose_setsockopt().

All we need to do here is to enable a sanity check that there is no int/uint overflow in right hand expression before the result gets stored in ulong.

...

...

return -EINVAL;           rose->t1 = opt * HZ;           return 0;         case ROSE_T2: -        if (opt < 1) +        if (opt < 1 || opt > UINT_MAX / HZ)               return -EINVAL;           rose->t2 = opt * HZ;           return 0;         case ROSE_T3: -        if (opt < 1) +        if (opt < 1 || opt > UINT_MAX / HZ)               return -EINVAL;           rose->t3 = opt * HZ;           return 0;         case ROSE_HOLDBACK: -        if (opt < 1) +        if (opt < 1 || opt > UINT_MAX / HZ)               return -EINVAL;           rose->hb = opt * HZ;           return 0;         case ROSE_IDLE: -        if (opt < 0) +        if (opt > UINT_MAX / (60 * HZ))               return -EINVAL;           rose->idle = opt * 60 * HZ;           return 0;

Regards, Nikita