lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALx6S35OZ_48UuJcxTTXwQBnbvRGRUUDrY5rkR6vUF-k4fgDrA@mail.gmail.com>
Date:   Fri, 1 Dec 2017 08:56:17 -0800
From:   Tom Herbert <tom@...bertland.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/3] fix kcm_clone()

On Thu, Nov 30, 2017 at 4:22 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> 1) it's fput() or sock_release(), not both
> 2) don't do fd_install() until the last failure exit.
> 3) not a bug per se, but... don't attach socket to struct file
>    until it's set up.
>
> Take reserving descriptor into the caller, move fd_install() to the
> caller, sanitize failure exits and calling conventions.
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>

Acked-by: Tom Herbert <tom@...bertland.com>

Thanks for the fix! I'll try to test some today also.

> ---
>  net/kcm/kcmsock.c | 71 +++++++++++++++++++++----------------------------------
>  1 file changed, 27 insertions(+), 44 deletions(-)
>
> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 0b750a22c4b9..c5fa634e63ca 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -1625,60 +1625,35 @@ static struct proto kcm_proto = {
>  };
>
>  /* Clone a kcm socket. */
> -static int kcm_clone(struct socket *osock, struct kcm_clone *info,
> -                    struct socket **newsockp)
> +static struct file *kcm_clone(struct socket *osock)
>  {
>         struct socket *newsock;
>         struct sock *newsk;
> -       struct file *newfile;
> -       int err, newfd;
> +       struct file *file;
>
> -       err = -ENFILE;
>         newsock = sock_alloc();
>         if (!newsock)
> -               goto out;
> +               return ERR_PTR(-ENFILE);
>
>         newsock->type = osock->type;
>         newsock->ops = osock->ops;
>
>         __module_get(newsock->ops->owner);
>
> -       newfd = get_unused_fd_flags(0);
> -       if (unlikely(newfd < 0)) {
> -               err = newfd;
> -               goto out_fd_fail;
> -       }
> -
> -       newfile = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
> -       if (IS_ERR(newfile)) {
> -               err = PTR_ERR(newfile);
> -               goto out_sock_alloc_fail;
> -       }
> -
>         newsk = sk_alloc(sock_net(osock->sk), PF_KCM, GFP_KERNEL,
>                          &kcm_proto, true);
>         if (!newsk) {
> -               err = -ENOMEM;
> -               goto out_sk_alloc_fail;
> +               sock_release(newsock);
> +               return ERR_PTR(-ENOMEM);
>         }
> -
>         sock_init_data(newsock, newsk);
>         init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux);
>
> -       fd_install(newfd, newfile);
> -       *newsockp = newsock;
> -       info->fd = newfd;
> -
> -       return 0;
> +       file = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
> +       if (IS_ERR(file))
> +               sock_release(newsock);
>
> -out_sk_alloc_fail:
> -       fput(newfile);
> -out_sock_alloc_fail:
> -       put_unused_fd(newfd);
> -out_fd_fail:
> -       sock_release(newsock);
> -out:
> -       return err;
> +       return file;
>  }
>
>  static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
> @@ -1708,17 +1683,25 @@ static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>         }
>         case SIOCKCMCLONE: {
>                 struct kcm_clone info;
> -               struct socket *newsock = NULL;
> -
> -               err = kcm_clone(sock, &info, &newsock);
> -               if (!err) {
> -                       if (copy_to_user((void __user *)arg, &info,
> -                                        sizeof(info))) {
> -                               err = -EFAULT;
> -                               sys_close(info.fd);
> -                       }
> -               }
> +               struct file *file;
> +
> +               info.fd = get_unused_fd_flags(0);
> +               if (unlikely(info.fd < 0))
> +                       return info.fd;
>
> +               file = kcm_clone(sock);
> +               if (IS_ERR(file)) {
> +                       put_unused_fd(info.fd);
> +                       return PTR_ERR(file);
> +               }
> +               if (copy_to_user((void __user *)arg, &info,
> +                                sizeof(info))) {
> +                       put_unused_fd(info.fd);
> +                       fput(file);
> +                       return -EFAULT;
> +               }
> +               fd_install(info.fd, file);
> +               err = 0;
>                 break;
>         }
>         default:
> --
> 2.11.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ