netdev - Re: [PATCH] net/handshake: fix file ref count in handshake_nl_accept

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: <ZTQR4J9ECvvwvZgM@tissot.1015granger.net>
Date: Sat, 21 Oct 2023 14:01:04 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Moritz Wanzenböck <moritz.wanzenboeck@...bit.com>
Cc: netdev@...r.kernel.org, kernel-tls-handshake@...ts.linux.dev
Subject: Re: [PATCH] net/handshake: fix file ref count in
 handshake_nl_accept_doit()

On Thu, Oct 19, 2023 at 02:58:47PM +0200, Moritz Wanzenböck wrote:
> If req->hr_proto->hp_accept() fail, we call fput() twice:
> Once in the error path, but also a second time because sock->file
> is at that point already associated with the file descriptor. Once
> the task exits, as it would probably do after receiving an error
> reading from netlink, the fd is closed, calling fput() a second time.
> 
> To fix, we move installing the file after the error path for the
> hp_accept() call. In the case of errors we simply put the unused fd.
> In case of success we can use fd_install() to link the sock->file
> to the reserved fd.
> 
> Fixes: 7ea9c1ec66bc ("net/handshake: Fix handshake_dup() ref counting")
> Signed-off-by: Moritz Wanzenböck <moritz.wanzenboeck@...bit.com>
> ---
>  net/handshake/netlink.c | 30 +++++-------------------------
>  1 file changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 64a0046dd611..89637e732866 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -87,29 +87,6 @@ struct nlmsghdr *handshake_genl_put(struct sk_buff *msg,
>  }
>  EXPORT_SYMBOL(handshake_genl_put);
>  
> -/*
> - * dup() a kernel socket for use as a user space file descriptor
> - * in the current process. The kernel socket must have an
> - * instatiated struct file.
> - *
> - * Implicit argument: "current()"
> - */
> -static int handshake_dup(struct socket *sock)
> -{
> -	struct file *file;
> -	int newfd;
> -
> -	file = get_file(sock->file);
> -	newfd = get_unused_fd_flags(O_CLOEXEC);
> -	if (newfd < 0) {
> -		fput(file);
> -		return newfd;
> -	}
> -
> -	fd_install(newfd, file);
> -	return newfd;
> -}
> -

The point of the handshake_dup() helper was to underscore the
similarities between dup(2) and handshake ACCEPT. That similarity
gets fainter over time.

Reviewed-by: Chuck Lever <chuck.lever@...cle.com>


>  int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct net *net = sock_net(skb->sk);
> @@ -133,17 +110,20 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto out_status;
>  
>  	sock = req->hr_sk->sk_socket;
> -	fd = handshake_dup(sock);
> +	fd = get_unused_fd_flags(O_CLOEXEC);
>  	if (fd < 0) {
>  		err = fd;
>  		goto out_complete;
>  	}
> +
>  	err = req->hr_proto->hp_accept(req, info, fd);
>  	if (err) {
> -		fput(sock->file);
> +		put_unused_fd(fd);
>  		goto out_complete;
>  	}
>  
> +	fd_install(fd, get_file(sock->file));
> +
>  	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
>  	return 0;
>  
> -- 
> 2.41.0
> 
> 

-- 
Chuck Lever