[PATCH v2 0/6] Bug fixes for net/handshake

[PATCH v2 0/6] Bug fixes for net/handshake

[Chuck Lever]

Build tested only, but these should address Jakub's review comments.

Please consider these for merge via net-next.

Changes since v1:
- Rework "Fix handshake_dup() ref counting"
- Unpin sock->file when a handshake is cancelled

---

Chuck Lever (6):
      net/handshake: Remove unneeded check from handshake_dup()
      net/handshake: Fix handshake_dup() ref counting
      net/handshake: Fix uninitialized local variable
      net/handshake: handshake_genl_notify() shouldn't ignore @flags
      net/handshake: Unpin sock->file if a handshake is cancelled
      net/handshake: Enable the SNI extension to work properly


 Documentation/netlink/specs/handshake.yaml |  4 ++++
 Documentation/networking/tls-handshake.rst |  5 +++++
 include/net/handshake.h                    |  1 +
 include/uapi/linux/handshake.h             |  1 +
 net/handshake/handshake.h                  |  1 +
 net/handshake/netlink.c                    | 11 +++--------
 net/handshake/request.c                    |  4 ++++
 net/handshake/tlshd.c                      |  8 ++++++++
 8 files changed, 27 insertions(+), 8 deletions(-)

--
Chuck Lever

[PATCH v2 1/6] net/handshake: Remove unneeded check from handshake_dup()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

handshake_req_submit() now verifies that the socket has a file.

Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 35c9c445e0b8..7ec8a76c3c8a 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -99,9 +99,6 @@ static int handshake_dup(struct socket *sock)
 	struct file *file;
 	int newfd;
 
-	if (!sock->file)
-		return -EBADF;
-
 	file = get_file(sock->file);
 	newfd = get_unused_fd_flags(O_CLOEXEC);
 	if (newfd < 0) {

[PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
twice.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.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;
-	}
 
 	fd_install(newfd, file);
 	return newfd;

[PATCH v2 3/6] net/handshake: Fix uninitialized local variable

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

trace_handshake_cmd_done_err() simply records the pointer in @req,
so initializing it to NULL is sufficient and safe.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 032d96152e2f..11cc275d726a 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -154,8 +154,8 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 int handshake_nl_done_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
+	struct handshake_req *req = NULL;
 	struct socket *sock = NULL;
-	struct handshake_req *req;
 	int fd, status, err;
 
 	if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_DONE_SOCKFD))

[PATCH v2 4/6] net/handshake: handshake_genl_notify() shouldn't ignore @flags

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 11cc275d726a..e865fcf68433 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -48,7 +48,7 @@ int handshake_genl_notify(struct net *net, const struct handshake_proto *proto,
 				proto->hp_handler_class))
 		return -ESRCH;
 
-	msg = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	msg = genlmsg_new(GENLMSG_DEFAULT_SIZE, flags);
 	if (!msg)
 		return -ENOMEM;
 

[PATCH v2 5/6] net/handshake: Unpin sock->file if a handshake is cancelled

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

If user space never calls DONE, sock->file's reference count remains
elevated. Enable sock->file to be freed eventually in this case.

Reported-by: Jakub Kacinski <kuba@kernel.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/handshake.h |    1 +
 net/handshake/request.c   |    4 ++++
 2 files changed, 5 insertions(+)

diff --git a/net/handshake/handshake.h b/net/handshake/handshake.h
index 4dac965c99df..8aeaadca844f 100644
--- a/net/handshake/handshake.h
+++ b/net/handshake/handshake.h
@@ -31,6 +31,7 @@ struct handshake_req {
 	struct list_head		hr_list;
 	struct rhash_head		hr_rhash;
 	unsigned long			hr_flags;
+	struct file			*hr_file;
 	const struct handshake_proto	*hr_proto;
 	struct sock			*hr_sk;
 	void				(*hr_odestruct)(struct sock *sk);
diff --git a/net/handshake/request.c b/net/handshake/request.c
index 94d5cef3e048..d78d41abb3d9 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -239,6 +239,7 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
 	}
 	req->hr_odestruct = req->hr_sk->sk_destruct;
 	req->hr_sk->sk_destruct = handshake_sk_destruct;
+	req->hr_file = sock->file;
 
 	ret = -EOPNOTSUPP;
 	net = sock_net(req->hr_sk);
@@ -334,6 +335,9 @@ bool handshake_req_cancel(struct sock *sk)
 		return false;
 	}
 
+	/* Request accepted and waiting for DONE */
+	fput(req->hr_file);
+
 out_true:
 	trace_handshake_cancel(net, req, sk);
 

[PATCH v2 6/6] net/handshake: Enable the SNI extension to work properly

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Enable the upper layer protocol to specify the SNI peername. This
avoids the need for tlshd to use a DNS lookup, which can return a
hostname that doesn't match the incoming certificate's SubjectName.

Fixes: 2fd5532044a8 ("net/handshake: Add a kernel API for requesting a TLSv1.3 handshake")
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 Documentation/netlink/specs/handshake.yaml |    4 ++++
 Documentation/networking/tls-handshake.rst |    5 +++++
 include/net/handshake.h                    |    1 +
 include/uapi/linux/handshake.h             |    1 +
 net/handshake/tlshd.c                      |    8 ++++++++
 5 files changed, 19 insertions(+)

diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
index 614f1a585511..6d89e30f5fd5 100644
--- a/Documentation/netlink/specs/handshake.yaml
+++ b/Documentation/netlink/specs/handshake.yaml
@@ -68,6 +68,9 @@ attribute-sets:
         type: nest
         nested-attributes: x509
         multi-attr: true
+      -
+        name: peername
+        type: string
   -
     name: done
     attributes:
@@ -105,6 +108,7 @@ operations:
             - auth-mode
             - peer-identity
             - certificate
+            - peername
     -
       name: done
       doc: Handler reports handshake completion
diff --git a/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
index a2817a88e905..6f5ea1646a47 100644
--- a/Documentation/networking/tls-handshake.rst
+++ b/Documentation/networking/tls-handshake.rst
@@ -53,6 +53,7 @@ fills in a structure that contains the parameters of the request:
         struct socket   *ta_sock;
         tls_done_func_t ta_done;
         void            *ta_data;
+        const char      *ta_peername;
         unsigned int    ta_timeout_ms;
         key_serial_t    ta_keyring;
         key_serial_t    ta_my_cert;
@@ -71,6 +72,10 @@ instantiated a struct file in sock->file.
 has completed. Further explanation of this function is in the "Handshake
 Completion" sesction below.
 
+The consumer can provide a NUL-terminated hostname in the @ta_peername
+field that is sent as part of ClientHello. If no peername is provided,
+the DNS hostname associated with the server's IP address is used instead.
+
 The consumer can fill in the @ta_timeout_ms field to force the servicing
 handshake agent to exit after a number of milliseconds. This enables the
 socket to be fully closed once both the kernel and the handshake agent
diff --git a/include/net/handshake.h b/include/net/handshake.h
index 3352b1ab43b3..2e26e436e85f 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -24,6 +24,7 @@ struct tls_handshake_args {
 	struct socket		*ta_sock;
 	tls_done_func_t		ta_done;
 	void			*ta_data;
+	const char		*ta_peername;
 	unsigned int		ta_timeout_ms;
 	key_serial_t		ta_keyring;
 	key_serial_t		ta_my_cert;
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index 1de4d0b95325..3d7ea58778c9 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -44,6 +44,7 @@ enum {
 	HANDSHAKE_A_ACCEPT_AUTH_MODE,
 	HANDSHAKE_A_ACCEPT_PEER_IDENTITY,
 	HANDSHAKE_A_ACCEPT_CERTIFICATE,
+	HANDSHAKE_A_ACCEPT_PEERNAME,
 
 	__HANDSHAKE_A_ACCEPT_MAX,
 	HANDSHAKE_A_ACCEPT_MAX = (__HANDSHAKE_A_ACCEPT_MAX - 1)
diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c
index fcbeb63b4eb1..b735f5cced2f 100644
--- a/net/handshake/tlshd.c
+++ b/net/handshake/tlshd.c
@@ -31,6 +31,7 @@ struct tls_handshake_req {
 	int			th_type;
 	unsigned int		th_timeout_ms;
 	int			th_auth_mode;
+	const char		*th_peername;
 	key_serial_t		th_keyring;
 	key_serial_t		th_certificate;
 	key_serial_t		th_privkey;
@@ -48,6 +49,7 @@ tls_handshake_req_init(struct handshake_req *req,
 	treq->th_timeout_ms = args->ta_timeout_ms;
 	treq->th_consumer_done = args->ta_done;
 	treq->th_consumer_data = args->ta_data;
+	treq->th_peername = args->ta_peername;
 	treq->th_keyring = args->ta_keyring;
 	treq->th_num_peerids = 0;
 	treq->th_certificate = TLS_NO_CERT;
@@ -214,6 +216,12 @@ static int tls_handshake_accept(struct handshake_req *req,
 	ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_MESSAGE_TYPE, treq->th_type);
 	if (ret < 0)
 		goto out_cancel;
+	if (treq->th_peername) {
+		ret = nla_put_string(msg, HANDSHAKE_A_ACCEPT_PEERNAME,
+				     treq->th_peername);
+		if (ret < 0)
+			goto out_cancel;
+	}
 	if (treq->th_timeout_ms) {
 		ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_TIMEOUT, treq->th_timeout_ms);
 		if (ret < 0)

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Simon Horman]

On Fri, May 05, 2023 at 08:46:01PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> twice.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH v2 5/6] net/handshake: Unpin sock->file if a handshake is cancelled

[Simon Horman]

On Fri, May 05, 2023 at 08:47:13PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If user space never calls DONE, sock->file's reference count remains
> elevated. Enable sock->file to be freed eventually in this case.
> 
> Reported-by: Jakub Kacinski <kuba@kernel.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH v2 1/6] net/handshake: Remove unneeded check from handshake_dup()

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:45:34PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> handshake_req_submit() now verifies that the socket has a file.
> 
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/handshake/netlink.c |    3 ---
>  1 file changed, 3 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:46:01PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> twice.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.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().

Thanks

> -	}
>  
>  	fd_install(newfd, file);
>  	return newfd;
> 
> 
> 

Re: [PATCH v2 3/6] net/handshake: Fix uninitialized local variable

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:46:27PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> trace_handshake_cmd_done_err() simply records the pointer in @req,
> so initializing it to NULL is sufficient and safe.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/handshake/netlink.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 4/6] net/handshake: handshake_genl_notify() shouldn't ignore @flags

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:46:54PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/handshake/netlink.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 6/6] net/handshake: Enable the SNI extension to work properly

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:47:40PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Enable the upper layer protocol to specify the SNI peername. This
> avoids the need for tlshd to use a DNS lookup, which can return a
> hostname that doesn't match the incoming certificate's SubjectName.
> 
> Fixes: 2fd5532044a8 ("net/handshake: Add a kernel API for requesting a TLSv1.3 handshake")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  Documentation/netlink/specs/handshake.yaml |    4 ++++
>  Documentation/networking/tls-handshake.rst |    5 +++++
>  include/net/handshake.h                    |    1 +
>  include/uapi/linux/handshake.h             |    1 +
>  net/handshake/tlshd.c                      |    8 ++++++++
>  5 files changed, 19 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 5/6] net/handshake: Unpin sock->file if a handshake is cancelled

[Leon Romanovsky]

On Fri, May 05, 2023 at 08:47:13PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If user space never calls DONE, sock->file's reference count remains
> elevated. Enable sock->file to be freed eventually in this case.
> 
> Reported-by: Jakub Kacinski <kuba@kernel.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/handshake/handshake.h |    1 +
>  net/handshake/request.c   |    4 ++++
>  2 files changed, 5 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Paolo Abeni]

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@oracle.com>
> > 
> > If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> > twice.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.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

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Chuck Lever III]

> On May 9, 2023, at 12:04 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> 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@oracle.com>
>>> 
>>> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
>>> twice.
>>> 
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.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;

I'm happy to accommodate these changes, but it's not clear to me
whether you want this hunk applied /in addition to/ my fix or
/instead of/.


> ---
> 
> Somewhat related: handshake_nl_done_doit() releases the file refcount
> even if the req lookup fails.

That's because sockfd_lookup() increments the file ref count.


> 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.

More likely, req_cancel might take the file ref count to zero
before sockfd_lookup can increment it, resulting in a UAF?

Let me think about this.


> 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.

Since we expect other subsystems to use net/handshake besides
SunRPC, I agree to going with netdev, even for this series once
it is acceptable.


--
Chuck Lever

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Paolo Abeni]

On Tue, 2023-05-09 at 13:59 +0000, Chuck Lever III wrote:
> 
> > On May 9, 2023, at 12:04 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > 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@oracle.com>
> > > > 
> > > > If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> > > > twice.
> > > > 
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.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;
> 
> I'm happy to accommodate these changes, but it's not clear to me
> whether you want this hunk applied /in addition to/ my fix or
> /instead of/.

It's above (completely untested!) chunk is intended to be a replace for
patch 2/6

> > ---
> > 
> > Somewhat related: handshake_nl_done_doit() releases the file refcount
> > even if the req lookup fails.
> 
> That's because sockfd_lookup() increments the file ref count.

Ooops, I missed that.

Then in the successful path handshake_nl_done_doit() should call
fput() twice ?!? 1 for the reference acquired by sockfd_lookup() and 1
for the reference owned by  'req' ?!? Otherwise a ref will be leaked.

> > 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.
> 
> More likely, req_cancel might take the file ref count to zero
> before sockfd_lookup can increment it, resulting in a UAF?
> 
> Let me think about this.

I now think this race is not possible, but I now fear the refcount leak
mentioned above.

Cheers,

Paolo

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Chuck Lever III]

> On May 9, 2023, at 7:22 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Tue, 2023-05-09 at 13:59 +0000, Chuck Lever III wrote:
>> 
>>> On May 9, 2023, at 12:04 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>>> 
>>> 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@oracle.com>
>>>>> 
>>>>> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
>>>>> twice.
>>>>> 
>>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.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;
>> 
>> I'm happy to accommodate these changes, but it's not clear to me
>> whether you want this hunk applied /in addition to/ my fix or
>> /instead of/.
> 
> It's above (completely untested!) chunk is intended to be a replace for
> patch 2/6
> 
>>> ---
>>> 
>>> Somewhat related: handshake_nl_done_doit() releases the file refcount
>>> even if the req lookup fails.
>> 
>> That's because sockfd_lookup() increments the file ref count.
> 
> Ooops, I missed that.
> 
> Then in the successful path handshake_nl_done_doit() should call
> fput() twice ?!? 1 for the reference acquired by sockfd_lookup() and 1
> for the reference owned by  'req' ?!? Otherwise a ref will be leaked.
> 
>>> 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.
>> 
>> More likely, req_cancel might take the file ref count to zero
>> before sockfd_lookup can increment it, resulting in a UAF?
>> 
>> Let me think about this.
> 
> I now think this race is not possible, but I now fear the refcount leak
> mentioned above.

Not sure why I haven't seen evidence of a leak here. I'll have a closer look.


--
Chuck Lever

Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting

[Chuck Lever III]

> On May 9, 2023, at 10:32 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On May 9, 2023, at 7:22 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>> 
>> On Tue, 2023-05-09 at 13:59 +0000, Chuck Lever III wrote:
>>> 
>>>> On May 9, 2023, at 12:04 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>>>> 
>>>> 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@oracle.com>
>>>>>> 
>>>>>> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
>>>>>> twice.
>>>>>> 
>>>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>>> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.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;
>>> 
>>> I'm happy to accommodate these changes, but it's not clear to me
>>> whether you want this hunk applied /in addition to/ my fix or
>>> /instead of/.
>> 
>> It's above (completely untested!) chunk is intended to be a replace for
>> patch 2/6
>> 
>>>> ---
>>>> 
>>>> Somewhat related: handshake_nl_done_doit() releases the file refcount
>>>> even if the req lookup fails.
>>> 
>>> That's because sockfd_lookup() increments the file ref count.
>> 
>> Ooops, I missed that.
>> 
>> Then in the successful path handshake_nl_done_doit() should call
>> fput() twice ?!? 1 for the reference acquired by sockfd_lookup() and 1
>> for the reference owned by  'req' ?!? Otherwise a ref will be leaked.
>> 
>>>> 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.
>>> 
>>> More likely, req_cancel might take the file ref count to zero
>>> before sockfd_lookup can increment it, resulting in a UAF?
>>> 
>>> Let me think about this.
>> 
>> I now think this race is not possible, but I now fear the refcount leak
>> mentioned above.
> 
> Not sure why I haven't seen evidence of a leak here. I'll have a closer look.

I added provisional trace points to report the value of f_count
before the fput() in done_doit and also where the RPC client
destroys the socket. The latter reports f_count is 1, as I would
expect, just before the final __fput_sync().


--
Chuck Lever