Misc fixes for v5

Misc fixes for v5

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

Hi Chuck,

after some fiddling with nested arguments (and eventually removing them) 
I found some issues with v5.
(cf the attached patch).

And I'm still stuck with the server side; ServerHello is sent to 
userspace, but that the daemon receives the packet only after a timeout 
on the client side :-(

So if you have any idea ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
Managing Directors: I. Totev, A. Myers, A. McDonald, M. B. Moerman
(HRB 36809, AG Nürnberg)

[-- Attachment #2: tls-hs.patch --]
[-- Type: text/x-patch, Size: 2296 bytes --]

commit c3c589b15a30a6376fd69b6613ed64bd9dd6b07d
Author: Hannes Reinecke <hare@suse.de>
Date:   Fri Mar 3 15:27:10 2023 +0100

    handshake fixes

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 88775f784305..882a97972b0b 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -114,19 +114,25 @@ static int handshake_status_reply(struct sk_buff *skb, struct genl_info *gi,
  *
  * Implicit argument: "current()"
  */
-static int handshake_dup(struct socket *kernsock)
+static int handshake_dup(struct socket *sock)
 {
-	struct file *file = get_file(kernsock->file);
+	struct file *file;
 	int newfd;
 
+	if (WARN_ON(!sock))
+		return -EINVAL;
 	newfd = get_unused_fd_flags(O_CLOEXEC);
-	if (newfd < 0) {
-		fput(file);
+	if (newfd < 0)
 		return newfd;
-	}
 
-	fd_install(newfd, file);
-	return newfd;
+	file = sock_alloc_file(sock, O_CLOEXEC,
+			       sock->sk->sk_prot_creator->name);
+	if (!IS_ERR(file)) {
+		fd_install(newfd, file);
+		return newfd;
+	}
+	put_unused_fd(newfd);
+	return PTR_ERR(file);
 }
 
 static const struct nla_policy
@@ -139,7 +145,7 @@ static int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *gi)
 	struct nlattr *tb[HANDSHAKE_A_ACCEPT_MAX + 1];
 	struct net *net = sock_net(skb->sk);
 	struct handshake_req *pos, *req;
-	int fd, err;
+	int fd = -1, err;
 
 	err = -EINVAL;
 	if (genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb,
@@ -177,7 +183,6 @@ static int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *gi)
 
 out_complete:
 	handshake_complete(req, -EIO, NULL);
-	fput(req->hr_sock->file);
 out_status:
 	trace_handshake_cmd_accept_err(net, req, NULL, err);
 	return handshake_status_reply(skb, gi, err);
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 372e68488ac0..f4af9680185d 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -105,7 +105,7 @@ static void tls_handshake_done(struct handshake_req *req, int status,
 	struct tls_handshake_req *treq = handshake_req_private(req);
 	key_serial_t peerid = TLS_NO_PEERID;
 
-	if (tb[HANDSHAKE_A_DONE_REMOTE_PEERID])
+	if (tb && tb[HANDSHAKE_A_DONE_REMOTE_PEERID])
 		peerid = nla_get_u32(tb[HANDSHAKE_A_DONE_REMOTE_PEERID]);
 
 	treq->th_consumer_done(treq->th_consumer_data, status, peerid);

Re: Misc fixes for v5

[Chuck Lever III]

> On Mar 3, 2023, at 10:08 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> Hi Chuck,
> 
> after some fiddling with nested arguments (and eventually removing them) I found some issues with v5.
> (cf the attached patch).

I plan to post a v6 soon that supports:

- a timeout attribute
- keyrings
- multiple certs and peerids per handshake

I still don't like the timeout attribute.

- Having timeouts at two layers is a known recipe for tears and
  sorrow.
- The kernel can't rely on the handshake agent to timeout and
  call DONE. If the agent exits before invoking DONE, there will
  be no handshake completion.

But it's not a hill I care to die on, so I added it.

The handshake netlink protocol now supports multiple peer IDs
per upcall, but the implementation does not yet. With only
protocol support we should be able to merge the upcall and then
implement the actual support whenever we are ready to actually
make use of it.


> And I'm still stuck with the server side; ServerHello is sent to userspace, but that the daemon receives the packet only after a timeout on the client side :-(
> 
> So if you have any idea ...
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		           Kernel Storage Architect
> hare@suse.de			                  +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
> Managing Directors: I. Totev, A. Myers, A. McDonald, M. B. Moerman
> (HRB 36809, AG Nürnberg)<tls-hs.patch>


> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 88775f784305..882a97972b0b 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -114,19 +114,25 @@ static int handshake_status_reply(struct sk_buff *skb, struct genl_info *gi,
>   *
>   * Implicit argument: "current()"
>   */
> -static int handshake_dup(struct socket *kernsock)
> +static int handshake_dup(struct socket *sock)
>  {
> -	struct file *file = get_file(kernsock->file);
> +	struct file *file;
>  	int newfd;
>  
> +	if (WARN_ON(!sock))
> +		return -EINVAL;
>  	newfd = get_unused_fd_flags(O_CLOEXEC);
> -	if (newfd < 0) {
> -		fput(file);
> +	if (newfd < 0)
>  		return newfd;
> -	}
>  
> -	fd_install(newfd, file);
> -	return newfd;
> +	file = sock_alloc_file(sock, O_CLOEXEC,
> +			       sock->sk->sk_prot_creator->name);

IIUC, sock_alloc_file will overwrite sock->file. Any consumer
that has already instantiated sock->file will now leak it.

The kernel consumer has to create and associate a file with the
socket, just as xprtsock.c does. I'm not sure this is a bug but
rather a requirement for using the API.


> +	if (!IS_ERR(file)) {
> +		fd_install(newfd, file);
> +		return newfd;
> +	}
> +	put_unused_fd(newfd);
> +	return PTR_ERR(file);
>  }
>  
>  static const struct nla_policy
> @@ -139,7 +145,7 @@ static int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *gi)
>  	struct nlattr *tb[HANDSHAKE_A_ACCEPT_MAX + 1];
>  	struct net *net = sock_net(skb->sk);
>  	struct handshake_req *pos, *req;
> -	int fd, err;
> +	int fd = -1, err;
>  
>  	err = -EINVAL;
>  	if (genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb,
> @@ -177,7 +183,6 @@ static int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *gi)
>  
>  out_complete:
>  	handshake_complete(req, -EIO, NULL);
> -	fput(req->hr_sock->file);
>  out_status:
>  	trace_handshake_cmd_accept_err(net, req, NULL, err);
>  	return handshake_status_reply(skb, gi, err);
> diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
> index 372e68488ac0..f4af9680185d 100644
> --- a/net/tls/tls_handshake.c
> +++ b/net/tls/tls_handshake.c
> @@ -105,7 +105,7 @@ static void tls_handshake_done(struct handshake_req *req, int status,
>  	struct tls_handshake_req *treq = handshake_req_private(req);
>  	key_serial_t peerid = TLS_NO_PEERID;
>  
> -	if (tb[HANDSHAKE_A_DONE_REMOTE_PEERID])
> +	if (tb && tb[HANDSHAKE_A_DONE_REMOTE_PEERID])
>  		peerid = nla_get_u32(tb[HANDSHAKE_A_DONE_REMOTE_PEERID]);

This looks like an oversight. I'll fix it.

>  
>  	treq->th_consumer_done(treq->th_consumer_data, status, peerid);

--
Chuck Lever

Re: Misc fixes for v5

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

On 3/3/23 16:28, Chuck Lever III wrote:
> 
> 
>> On Mar 3, 2023, at 10:08 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> Hi Chuck,
>>
>> after some fiddling with nested arguments (and eventually removing them) I found some issues with v5.
>> (cf the attached patch).
> 
> I plan to post a v6 soon that supports:
> 
> - a timeout attribute
> - keyrings
> - multiple certs and peerids per handshake
> 
> I still don't like the timeout attribute.
> 
> - Having timeouts at two layers is a known recipe for tears and
>    sorrow.
> - The kernel can't rely on the handshake agent to timeout and
>    call DONE. If the agent exits before invoking DONE, there will
>    be no handshake completion.
> 
> But it's not a hill I care to die on, so I added it.
> 
Yeah, and that's the point where the socket is closed; once
the userspace daemon finally gives up it'll close the connection
(for reasons still be to figured out).
And that typically happens while the kernel side is trying to
send stuff on that socket, leading to interesting crashes.
So yeah, the only safe thing seems to be the close the socket
from the kernel once the kernel detects a timeout.

> The handshake netlink protocol now supports multiple peer IDs
> per upcall, but the implementation does not yet. With only
> protocol support we should be able to merge the upcall and then
> implement the actual support whenever we are ready to actually
> make use of it.
> 
Oh, good; I've already updated the PSK interface to provide several
identities (see the attached patch).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

[-- Attachment #2: 0001-net-tls-allow-to-pass-a-list-of-identities-for-tls_c.patch --]
[-- Type: text/x-patch, Size: 3866 bytes --]

From c4e9ac896ce5e0a2f499010fff11011590617048 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Fri, 3 Mar 2023 12:39:46 +0100
Subject: [PATCH] net/tls: allow to pass a list of identities for
 tls_client_hello_psk()

NVMe-over-TLS allows for several identities to be passed with the
TLS ClientHello request, so update tls_client_hello_psk() to accept
a list of identities.
The netlink interface currently only supports a single identity to
be transferred to the userspace daemon, so restrict the length of
the identity list to '1' until that is fixed.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/net/tls.h       |  2 +-
 net/tls/tls_handshake.c | 25 ++++++++++++++++++-------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index d792e3eab017..1171eb7d940a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -535,7 +535,7 @@ int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
 			  key_serial_t cert, key_serial_t privkey);
 int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 key_serial_t peerid);
+			 key_serial_t *peerids, unsigned int num_peerids);
 int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
 			  void *data, const char *priorities);
 int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 74d32a9ca857..372e68488ac0 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -42,7 +42,8 @@ struct tls_handshake_req {
 	const char		*th_priorities;
 	int			th_type;
 	int			th_auth_type;
-	key_serial_t		th_peerid;
+	unsigned int		th_num_peerids;
+	key_serial_t		*th_peerids;
 	key_serial_t		th_certificate;
 	key_serial_t		th_privkey;
 
@@ -69,7 +70,8 @@ tls_handshake_req_init(struct handshake_req *req, tls_done_func_t done,
 	treq->th_consumer_done = done;
 	treq->th_consumer_data = data;
 	treq->th_priorities = priorities;
-	treq->th_peerid = TLS_NO_PEERID;
+	treq->th_peerids = NULL;
+	treq->th_num_peerids = 0;
 	treq->th_certificate = TLS_NO_CERT;
 	treq->th_privkey = TLS_NO_PRIVKEY;
 	return treq;
@@ -136,9 +138,16 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
 		}
 		break;
 	case HANDSHAKE_AUTH_PSK:
-		if (treq->th_peerid != TLS_NO_PEERID) {
+		if (treq->th_peerids) {
+			key_serial_t peerid;
+
+			if (treq->th_num_peerids > 1) {
+				ret = -EINVAL;
+				goto out;
+			}
+			peerid = treq->th_peerids[0];
 			ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_MY_PEERID,
-					  treq->th_peerid);
+					  peerid);
 			if (ret < 0)
 				goto out;
 		}
@@ -294,7 +303,8 @@ EXPORT_SYMBOL(tls_client_hello_x509);
  * @done: function to call when the handshake has completed
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string
- * @peerid: serial number of key containing TLS identity
+ * @peerids: list of key serial numbers for possible TLS identities
+ * @num_peerids: number of entries in @peerids
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -303,7 +313,7 @@ EXPORT_SYMBOL(tls_client_hello_x509);
  */
 int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 key_serial_t peerid)
+			 key_serial_t *peerids, unsigned int num_peerids)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -323,7 +333,8 @@ int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 	treq = tls_handshake_req_init(req, done, data, tp);
 	treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTHELLO;
 	treq->th_auth_type = HANDSHAKE_AUTH_PSK;
-	treq->th_peerid = peerid;
+	treq->th_peerids = peerids;
+	treq->th_num_peerids = num_peerids;
 
 	return handshake_req_submit(req, flags);
 }
-- 
2.35.3

Re: Misc fixes for v5

[Chuck Lever III]

> On Mar 3, 2023, at 11:38 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 3/3/23 16:28, Chuck Lever III wrote:
>>> On Mar 3, 2023, at 10:08 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> Hi Chuck,
>>> 
>>> after some fiddling with nested arguments (and eventually removing them) I found some issues with v5.
>>> (cf the attached patch).
>> I plan to post a v6 soon that supports:
>> - a timeout attribute
>> - keyrings
>> - multiple certs and peerids per handshake
>> I still don't like the timeout attribute.
>> - Having timeouts at two layers is a known recipe for tears and
>>   sorrow.
>> - The kernel can't rely on the handshake agent to timeout and
>>   call DONE. If the agent exits before invoking DONE, there will
>>   be no handshake completion.
>> But it's not a hill I care to die on, so I added it.
> Yeah, and that's the point where the socket is closed; once
> the userspace daemon finally gives up it'll close the connection
> (for reasons still be to figured out).

Could be because your kernel consumer isn't holding a struct
file on that socket...? When the user fd is closed, that's
the last reference on that filp and the kernel closes the
socket right there.


--
Chuck Lever