netdev - Re: [PATCH v2 2/6] net/handshake: Fix handshake

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 09 May 2023 09:04:39 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Leon Romanovsky <leon@...nel.org>, Chuck Lever <cel@...nel.org>
Cc: kernel-tls-handshake@...ts.linux.dev, netdev@...r.kernel.org, 
	dan.carpenter@...aro.org
Subject: Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

On Sun, 2023-05-07 at 11:25 +0300, Leon Romanovsky wrote:
> On Fri, May 05, 2023 at 08:46:01PM -0400, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@...cle.com>
> > 
> > If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> > twice.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> > Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
> > ---
> >  net/handshake/netlink.c |    4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> > index 7ec8a76c3c8a..032d96152e2f 100644
> > --- a/net/handshake/netlink.c
> > +++ b/net/handshake/netlink.c
> > @@ -101,10 +101,8 @@ static int handshake_dup(struct socket *sock)
> >  
> >  	file = get_file(sock->file);
> >  	newfd = get_unused_fd_flags(O_CLOEXEC);
> > -	if (newfd < 0) {
> > -		fput(file);
> > +	if (newfd < 0)
> >  		return newfd;
> 
> IMHO, the better way to fix it is to change handshake_nl_accept_doit()
> do not call to fput(sock->file) in error case. It is not right thing
> to have a call to handshake_dup() and rely on elevated get_file()
> for failure too as it will be problematic for future extension of
> handshake_dup().

I agree with the above: I think a failing helper should leave the
larger scope status unmodified. In this case a failing handshake_dup()
should not touch file refcount, and handshake_nl_accept_doit() should
be modified accordingly, something alike:

---
diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index e865fcf68433..8897a17189ad 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -138,14 +138,15 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 	err = req->hr_proto->hp_accept(req, info, fd);
 	if (err)
-		goto out_complete;
+		goto out_put;
 
 	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
 	return 0;
 
+out_put:
+	fput(sock->file);
 out_complete:
 	handshake_complete(req, -EIO, NULL);
-	fput(sock->file);
 out_status:
 	trace_handshake_cmd_accept_err(net, req, NULL, err);
 	return err;
---

Somewhat related: handshake_nl_done_doit() releases the file refcount
even if the req lookup fails. If that is caused by a concurrent
req_cancel - not sure if possible at all, possibly syzkaller could
guess if instructed about the API - such refcount will underflow, as it
is rightfully decremented by req_cancel, too.

I think it should be safer adding a chunk like:

---
diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index e865fcf68433..3e3e849f302a 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -172,7 +173,6 @@ int handshake_nl_done_doit(struct sk_buff *skb, struct genl_info *info)
 	req = handshake_req_hash_lookup(sock->sk);
 	if (!req) {
 		err = -EBUSY;
-		fput(sock->file);
 		goto out_status;
 	}
---

Possibly explicitly documenting the used ownership rules for the file
refcount in the relevant functions could help with future maintenance.

Finally it's not clear to me if we agreed to a target tree or not ;) I
see no replies so my suggestion.

Thanks!

Paolo