On Thu, Nov 30, 2017 at 4:22 PM, Al Viro <v...@zeniv.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 <v...@zeniv.linux.org.uk>
Acked-by: Tom Herbert <t...@herbertland.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
>
