On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
...
> Only SCTP_SOCKOPT_CONNECTX3 contains an indirect pointer.
> It is also the only getsockopt() that wants to return a buffer
> and an error code. It is also definitely abusing getsockopt().
...
> @@ -1375,11 +1350,11 @@ struct compat_sctp_getaddrs_old {
>  #endif
>  
>  static int sctp_getsockopt_connectx3(struct sock *sk, int len,
> -                                  char __user *optval,
> -                                  int __user *optlen)
> +                                  struct sctp_getaddrs_old *param,
> +                                  int *optlen)
>  {
> -     struct sctp_getaddrs_old param;
>       sctp_assoc_t assoc_id = 0;
> +     struct sockaddr *addrs;
>       int err = 0;
>  
>  #ifdef CONFIG_COMPAT
> @@ -1388,29 +1363,28 @@ static int sctp_getsockopt_connectx3(struct sock *sk, 
> int len,
>  
>               if (len < sizeof(param32))
>                       return -EINVAL;
> -             if (copy_from_user(&param32, optval, sizeof(param32)))
> -                     return -EFAULT;
> +             param32 = *(struct compat_sctp_getaddrs_old *)param;
>  
> -             param.assoc_id = param32.assoc_id;
> -             param.addr_num = param32.addr_num;
> -             param.addrs = compat_ptr(param32.addrs);
> +             param->assoc_id = param32.assoc_id;
> +             param->addr_num = param32.addr_num;
> +             param->addrs = compat_ptr(param32.addrs);
>       } else
>  #endif
>       {
> -             if (len < sizeof(param))
> +             if (len < sizeof(*param))
>                       return -EINVAL;
> -             if (copy_from_user(&param, optval, sizeof(param)))
> -                     return -EFAULT;
>       }
>  
> -     err = __sctp_setsockopt_connectx(sk, (struct sockaddr __user *)
> -                                      param.addrs, param.addr_num,
> +     addrs = memdup_user(param->addrs, param->addr_num);

I'm staring at this for a while now but I don't get this memdup_user.
AFAICT, params->addrs is not __user anymore here, because
sctp_getsockopt() copied the whole thing already, no?
Also weird because it is being called from kernel_sctp_getsockopt(),
which now has no knowledge of __user buffers.
Maybe I didn't get something from the patch description.

> +     if (IS_ERR(addrs))
> +             return PTR_ERR(addrs);
> +
> +     err = __sctp_setsockopt_connectx(sk, addrs, param->addr_num,
>                                        &assoc_id);
> +     kfree(addrs);
>       if (err == 0 || err == -EINPROGRESS) {
> -             if (copy_to_user(optval, &assoc_id, sizeof(assoc_id)))
> -                     return -EFAULT;
> -             if (put_user(sizeof(assoc_id), optlen))
> -                     return -EFAULT;
> +             *(sctp_assoc_t *)param = assoc_id;
> +             *optlen = sizeof(assoc_id);
>       }
>  
>       return err;

Reply via email to