[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d170edfd-f2a2-c7a2-c473-c80f20c6557a@suse.de>
Date: Thu, 16 Feb 2023 11:47:52 +0100
From: Hannes Reinecke <hare@...e.de>
To: Chuck Lever <chuck.lever@...cle.com>, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com
Cc: netdev@...r.kernel.org, hare@...e.com, dhowells@...hat.com,
bcodding@...hat.com, kolga@...app.com, jmeneghi@...hat.com
Subject: Re: [PATCH v4 1/2] net/handshake: Create a NETLINK service for
handling handshake requests
On 2/15/23 20:23, Chuck Lever wrote:
> When a kernel consumer needs a transport layer security session, it
> first needs a handshake to negotiate and establish a session. This
> negotiation can be done in user space via one of the several
> existing library implementations, or it can be done in the kernel.
>
> No in-kernel handshake implementations yet exist. In their absence,
> we add a netlink service that can:
>
> a. Notify a user space daemon that a handshake is needed.
>
> b. Once notified, the daemon calls the kernel back via this
> netlink service to get the handshake parameters, including an
> open socket on which to establish the session.
>
> c. Once the handshake is complete, the daemon reports the
> session status and other information via a second netlink
> operation. This operation marks that it is safe for the
> kernel to use the open socket and the security session
> established there.
>
> The notification service uses a multicast group. Each handshake
> protocol (eg, TLSv1.3, PSP, etc) adopts its own group number so that
> the user space daemons for performing handshakes are completely
> independent of one another. The kernel can then tell via
> netlink_has_listeners() whether a user space daemon is active and
> can handle a handshake request for the desired security layer
> protocol.
>
> A new netlink operation, ACCEPT, acts like accept(2) in that it
> instantiates a file descriptor in the user space daemon's fd table.
> If this operation is successful, the reply carries the fd number,
> which can be treated as an open and ready file descriptor.
>
> While user space is performing the handshake, the kernel keeps its
> muddy paws off the open socket. A second new netlink operation,
> DONE, indicates that the user space daemon is finished with the
> socket and it is safe for the kernel to use again. The operation
> also indicates whether a session was established successfully.
>
> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
> ---
> include/net/handshake.h | 46 +++++
> include/net/net_namespace.h | 5 +
> include/net/sock.h | 1
> include/uapi/linux/handshake.h | 56 ++++++
> net/Makefile | 1
> net/handshake/Makefile | 11 +
> net/handshake/handshake.h | 43 +++++
> net/handshake/netlink.c | 370 ++++++++++++++++++++++++++++++++++++++++
> net/handshake/request.c | 160 +++++++++++++++++
> 9 files changed, 693 insertions(+)
> create mode 100644 include/net/handshake.h
> create mode 100644 include/uapi/linux/handshake.h
> create mode 100644 net/handshake/Makefile
> create mode 100644 net/handshake/handshake.h
> create mode 100644 net/handshake/netlink.c
> create mode 100644 net/handshake/request.c
>
> diff --git a/include/net/handshake.h b/include/net/handshake.h
> new file mode 100644
> index 000000000000..ca401c08c541
> --- /dev/null
> +++ b/include/net/handshake.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Generic HANDSHAKE service.
> + *
> + * Author: Chuck Lever <chuck.lever@...cle.com>
> + *
> + * Copyright (c) 2023, Oracle and/or its affiliates.
> + */
> +
> +/*
> + * Data structures and functions that are visible only within the
> + * kernel are declared here.
> + */
> +
> +#ifndef _NET_HANDSHAKE_H
> +#define _NET_HANDSHAKE_H
> +
> +struct handshake_req;
> +
> +/*
> + * Invariants for all handshake requests for one transport layer
> + * security protocol
> + */
> +struct handshake_proto {
> + int hp_protocol;
> + int hp_mcgrp;
> + size_t hp_privsize;
> +
> + int (*hp_accept)(struct handshake_req *req,
> + struct genl_info *gi, int fd);
> + void (*hp_done)(struct handshake_req *req,
> + int status, struct nlattr *args);
> + void (*hp_destroy)(struct handshake_req *req);
> +};
> +
> +extern struct handshake_req *
> +handshake_req_alloc(struct socket *sock, const struct handshake_proto *proto,
> + gfp_t flags);
> +extern void *handshake_req_private(struct handshake_req *req);
> +extern int handshake_req_submit(struct handshake_req *req, gfp_t flags);
> +extern void handshake_req_cancel(struct socket *sock);
> +
> +extern struct nlmsghdr *handshake_genl_put(struct sk_buff *msg,
> + struct genl_info *gi);
> +
> +#endif /* _NET_HANDSHAKE_H */
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 8c3587d5c308..a66309789560 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -186,6 +186,11 @@ struct net {
> #if IS_ENABLED(CONFIG_SMC)
> struct netns_smc smc;
> #endif
> +
> + /* transport layer security handshake requests */
> + spinlock_t hs_lock;
> + struct list_head hs_requests;
> + int hs_pending;
> } __randomize_layout;
>
> #include <linux/seq_file_net.h>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index e0517ecc6531..e16e63ff61f2 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -515,6 +515,7 @@ struct sock {
>
> struct socket *sk_socket;
> void *sk_user_data;
> + void *sk_handshake_req;
> #ifdef CONFIG_SECURITY
> void *sk_security;
> #endif
> diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
> new file mode 100644
> index 000000000000..9544edeb181f
> --- /dev/null
> +++ b/include/uapi/linux/handshake.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * GENL HANDSHAKE service.
> + *
> + * Author: Chuck Lever <chuck.lever@...cle.com>
> + *
> + * Copyright (c) 2023, Oracle and/or its affiliates.
> + */
> +
> +/*
> + * Data structures and functions that are visible to user space are
> + * declared here. This file constitutes an API contract between the
> + * Linux kernel and user space.
> + */
> +
> +#ifndef _UAPI_LINUX_HANDSHAKE_H
> +#define _UAPI_LINUX_HANDSHAKE_H
> +
> +#define HANDSHAKE_GENL_NAME "handshake"
> +#define HANDSHAKE_GENL_VERSION 0x01
> +
> +enum handshake_genl_mcgrps {
> + HANDSHAKE_GENL_MCGRP_NONE = 0,
> +};
> +
> +#define HANDSHAKE_GENL_MCGRP_NONE_NAME "none"
> +
> +enum handshake_genl_cmds {
> + HANDSHAKE_GENL_CMD_UNSPEC = 0,
> + HANDSHAKE_GENL_CMD_READY,
> + HANDSHAKE_GENL_CMD_ACCEPT,
> + HANDSHAKE_GENL_CMD_DONE,
> +
> + __HANDSHAKE_GENL_CMD_MAX
> +};
> +#define HANDSHAKE_GENL_CMD_MAX (__HANDSHAKE_GENL_CMD_MAX - 1)
> +
> +enum handshake_genl_attrs {
> + HANDSHAKE_GENL_ATTR_UNSPEC = 0,
> + HANDSHAKE_GENL_ATTR_MSG_STATUS,
> + HANDSHAKE_GENL_ATTR_SESS_STATUS,
> + HANDSHAKE_GENL_ATTR_SOCKFD,
> + HANDSHAKE_GENL_ATTR_PROTOCOL,
> +
> + HANDSHAKE_GENL_ATTR_ACCEPT,
> + HANDSHAKE_GENL_ATTR_DONE,
> +
> + __HANDSHAKE_GENL_ATTR_MAX
> +};
> +#define HANDSHAKE_GENL_ATTR_MAX (__HANDSHAKE_GENL_ATTR_MAX - 1)
> +
> +enum handshake_genl_protocol {
> + HANDSHAKE_GENL_PROTO_UNSPEC = 0,
> +};
> +
> +#endif /* _UAPI_LINUX_HANDSHAKE_H */
> diff --git a/net/Makefile b/net/Makefile
> index 6a62e5b27378..c1bb53f00486 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_NET_NCSI) += ncsi/
> obj-$(CONFIG_XDP_SOCKETS) += xdp/
> obj-$(CONFIG_MPTCP) += mptcp/
> obj-$(CONFIG_MCTP) += mctp/
> +obj-y += handshake/
> diff --git a/net/handshake/Makefile b/net/handshake/Makefile
> new file mode 100644
> index 000000000000..824e08c626af
> --- /dev/null
> +++ b/net/handshake/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for the Generic HANDSHAKE service
> +#
> +# Author: Chuck Lever <chuck.lever@...cle.com>
> +#
> +# Copyright (c) 2023, Oracle and/or its affiliates.
> +#
> +
> +obj-y += handshake.o
> +handshake-y := netlink.o request.o
> diff --git a/net/handshake/handshake.h b/net/handshake/handshake.h
> new file mode 100644
> index 000000000000..1cbcfc632a24
> --- /dev/null
> +++ b/net/handshake/handshake.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Generic netlink handshake service
> + *
> + * Author: Chuck Lever <chuck.lever@...cle.com>
> + *
> + * Copyright (c) 2023, Oracle and/or its affiliates.
> + */
> +
> +/*
> + * Data structures and functions that are visible only within the
> + * handshake module are declared here.
> + */
> +
> +#ifndef _INTERNAL_HANDSHAKE_H
> +#define _INTERNAL_HANDSHAKE_H
> +
> +/*
> + * One handshake request
> + */
> +struct handshake_req {
> + refcount_t hr_ref;
> + struct list_head hr_list;
> + unsigned long hr_flags;
> + const struct handshake_proto *hr_proto;
> + struct socket *hr_sock;
> + int hr_fd;
> +};
> +
> +#define HANDSHAKE_F_COMPLETED BIT(0)
> +
> +int handshake_genl_notify(struct net *net, struct handshake_req *req,
> + gfp_t flags);
> +void handshake_complete(struct handshake_req *req, int status,
> + struct nlattr *args);
> +
> +struct handshake_req *handshake_req_get(struct handshake_req *req);
> +void handshake_req_put(struct handshake_req *req);
> +
> +void add_pending_locked(struct net *net, struct handshake_req *req);
> +bool handshake_remove_pending(struct net *net, struct handshake_req *req);
> +
> +#endif /* _INTERNAL_HANDSHAKE_H */
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> new file mode 100644
> index 000000000000..8d0bf11396a7
> --- /dev/null
> +++ b/net/handshake/netlink.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic netlink handshake service
> + *
> + * Author: Chuck Lever <chuck.lever@...cle.com>
> + *
> + * Copyright (c) 2023, Oracle and/or its affiliates.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/socket.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/skbuff.h>
> +#include <linux/inet.h>
> +
> +#include <net/sock.h>
> +#include <net/genetlink.h>
> +#include <net/handshake.h>
> +
> +#include <uapi/linux/handshake.h>
> +#include "handshake.h"
> +
> +static struct genl_family __ro_after_init handshake_genl_family;
> +
> +void add_pending_locked(struct net *net, struct handshake_req *req)
> +{
> + net->hs_pending++;
> + list_add_tail(&req->hr_list, &net->hs_requests);
> +}
> +
> +static void remove_pending_locked(struct net *net, struct handshake_req *req)
> +{
> + net->hs_pending--;
> + list_del_init(&req->hr_list);
> +}
> +
> +/*
> + * Returns true if this req was on the pending list.
> + */
> +bool handshake_remove_pending(struct net *net, struct handshake_req *req)
> +{
> + struct sock *sk = req->hr_sock->sk;
> + bool ret;
> +
> + ret = false;
> +
> + spin_lock(&net->hs_lock);
> + if (!list_empty(&req->hr_list)) {
> + remove_pending_locked(net, req);
> + ret = true;
> + }
> + sk->sk_handshake_req = NULL;
> + spin_unlock(&net->hs_lock);
> +
> + return ret;
> +}
> +
> +void handshake_complete(struct handshake_req *req, int status,
> + struct nlattr *args)
> +{
> + if (!test_and_set_bit(HANDSHAKE_F_COMPLETED, &req->hr_flags)) {
> + req->hr_proto->hp_done(req, status, args);
> + req->hr_sock->sk->sk_handshake_req = NULL;
> + }
> + handshake_req_put(req);
> +}
> +
> +int handshake_genl_notify(struct net *net, struct handshake_req *req,
> + gfp_t flags)
> +{
> + struct sk_buff *skb;
> + void *hdr;
> +
> + if (!genl_has_listeners(&handshake_genl_family, net,
> + req->hr_proto->hp_mcgrp))
> + return -ESRCH;
> +
> + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, 0, 0, &handshake_genl_family, 0,
> + HANDSHAKE_GENL_CMD_READY);
> + if (!hdr) {
> + nlmsg_free(skb);
> + return -EMSGSIZE;
> + }
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_multicast(&handshake_genl_family, skb, 0,
> + req->hr_proto->hp_mcgrp, flags);
> +}
> +
> +static int handshake_accept(struct handshake_req *req)
> +{
> + struct socket *sock = req->hr_sock;
> + int flags = O_CLOEXEC;
> + struct file *file;
> + int fd;
> +
> + fd = get_unused_fd_flags(flags);
> + if (fd < 0)
> + return fd;
> + file = sock_alloc_file(sock, flags, sock->sk->sk_prot_creator->name);
> + if (IS_ERR(file)) {
> + put_unused_fd(fd);
> + return PTR_ERR(file);
> + }
> +
> + req->hr_fd = fd;
> + fd_install(fd, file);
> + return 0;
> +}
> +
> +static const struct nla_policy
> +handshake_genl_policy[HANDSHAKE_GENL_ATTR_MAX + 1] = {
> + [HANDSHAKE_GENL_ATTR_MSG_STATUS] = {
> + .type = NLA_U32
> + },
> + [HANDSHAKE_GENL_ATTR_SESS_STATUS] = {
> + .type = NLA_U32
> + },
> + [HANDSHAKE_GENL_ATTR_SOCKFD] = {
> + .type = NLA_U32
> + },
> + [HANDSHAKE_GENL_ATTR_PROTOCOL] = {
> + .type = NLA_U32
> + },
> +
> + [HANDSHAKE_GENL_ATTR_ACCEPT] = {
> + .type = NLA_NESTED,
> + },
> + [HANDSHAKE_GENL_ATTR_DONE] = {
> + .type = NLA_NESTED,
> + },
> +};
> +
> +/**
> + * handshake_genl_put - Create a generic netlink message header
> + * @msg: buffer in which to create the header
> + * @gi: generic netlink message context
> + *
> + * Returns a ready-to-use header, or NULL.
> + */
> +struct nlmsghdr *handshake_genl_put(struct sk_buff *msg, struct genl_info *gi)
> +{
> + return genlmsg_put(msg, gi->snd_portid, gi->snd_seq,
> + &handshake_genl_family, 0, gi->genlhdr->cmd);
> +}
> +EXPORT_SYMBOL(handshake_genl_put);
> +
> +static int handshake_genl_status_reply(struct sk_buff *skb,
> + struct genl_info *gi, int status)
> +{
> + struct nlmsghdr *hdr;
> + struct sk_buff *msg;
> + int ret;
> +
> + ret = -ENOMEM;
> + msg = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!msg)
> + goto out;
> + hdr = handshake_genl_put(msg, gi);
> + if (!hdr)
> + goto out_free;
> +
> + ret = -EMSGSIZE;
> + ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_MSG_STATUS, status);
> + if (ret < 0)
> + goto out_free;
> +
> + genlmsg_end(msg, hdr);
> + return genlmsg_reply(msg, gi);
> +
> +out_free:
> + genlmsg_cancel(msg, hdr);
> +out:
> + return ret;
> +}
> +
> +static int handshake_genl_cmd_accept(struct sk_buff *skb, struct genl_info *gi)
> +{
> + struct nlattr *tb[HANDSHAKE_GENL_ATTR_MAX + 1];
> + struct net *net = sock_net(skb->sk);
> + struct handshake_req *pos, *req;
> + int err;
> +
> + err = genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb,
> + HANDSHAKE_GENL_ATTR_MAX, handshake_genl_policy,
> + NULL);
> + if (err) {
> + pr_err_ratelimited("%s: genlmsg_parse() returned %d\n",
> + __func__, err);
> + return err;
> + }
> +
> + if (!tb[HANDSHAKE_GENL_ATTR_PROTOCOL])
> + return handshake_genl_status_reply(skb, gi, -EINVAL);
> +
> + req = NULL;
> + spin_lock(&net->hs_lock);
> + list_for_each_entry(pos, &net->hs_requests, hr_list) {
> + if (pos->hr_proto->hp_protocol !=
> + nla_get_u32(tb[HANDSHAKE_GENL_ATTR_PROTOCOL]))
> + continue;
> + remove_pending_locked(net, pos);
> + req = handshake_req_get(pos);
> + break;
> + }
> + spin_unlock(&net->hs_lock);
> + if (!req)
> + return handshake_genl_status_reply(skb, gi, -EAGAIN);
> +
> + err = handshake_accept(req);
> + if (err < 0) {
> + handshake_complete(req, -EIO, NULL);
> + handshake_req_put(req);
> + return handshake_genl_status_reply(skb, gi, err);
> + }
> + err = req->hr_proto->hp_accept(req, gi, req->hr_fd);
> + if (err) {
> + put_unused_fd(req->hr_fd);
> + handshake_complete(req, -EIO, NULL);
> + handshake_req_put(req);
> + return err;
> + }
> + return 0;
> +}
> +
> +/*
> + * This function is careful to not close the socket. It merely removes
> + * it from the file descriptor table so that it is no longer visible
> + * to the calling process.
> + */
> +static int handshake_genl_cmd_done(struct sk_buff *skb, struct genl_info *gi)
> +{
> + struct nlattr *tb[HANDSHAKE_GENL_ATTR_MAX + 1];
> + struct handshake_req *req;
> + struct socket *sock;
> + int fd, status, err;
> +
> + err = genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb,
> + HANDSHAKE_GENL_ATTR_MAX, handshake_genl_policy,
> + NULL);
> + if (err) {
> + pr_err_ratelimited("%s: genlmsg_parse() returned %d\n",
> + __func__, err);
> + return err;
> + }
> +
> + if (!tb[HANDSHAKE_GENL_ATTR_SOCKFD])
> + return handshake_genl_status_reply(skb, gi, -EINVAL);
> + err = 0;
> + fd = nla_get_u32(tb[HANDSHAKE_GENL_ATTR_SOCKFD]);
> + sock = sockfd_lookup(fd, &err);
> + if (err)
> + return handshake_genl_status_reply(skb, gi, -EBADF);
> +
> + req = sock->sk->sk_handshake_req;
And this will crash horribly if userspace released the socket in the
meantime (as then sock->sk is NULL).
(Note: I probably show my complete ignorance of the network stack here,
but ...)
Is there a good way of figuring out if 'sock->sk' is valid?
sock_hold() only makes sure that 'sock' is valid; it does nothing about
sock->sk.
Especially this bit in net/socket.c:__sock_release()
if (!sock->file) {
iput(SOCK_INODE(sock));
return;
}
sock->file = NULL;
will always get you, as the _first_ caller to sock_release() does the
right thing (by setting sock->file to NULL), but the second caller will
crash in iput().
There _must_ be a better way of checking...
Cheers,
Hannes
Powered by blists - more mailing lists