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: <15f85fca-54bc-46c6-b016-0d1761052fe4@suse.de>
Date: Thu, 19 Oct 2023 15:06:39 +0200
From: Hannes Reinecke <hare@...e.de>
To: Moritz Wanzenböck <moritz.wanzenboeck@...bit.com>,
 netdev@...r.kernel.org, kernel-tls-handshake@...ts.linux.dev
Cc: chuck.lever@...cle.com
Subject: Re: [PATCH] net/handshake: fix file ref count in
 handshake_nl_accept_doit()

On 10/19/23 14:58, 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;
> -}
> -
>   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;
>   
??
You sure this works?
With this patch 'fd' won't be available during the 'hp_accept' call,
but I thought that we might want to read from the socket here.
Hmm?

Cheers,

Hannes