Re: [PATCH v3 08/19] net: psp: add socket security association code

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Daniel Zahka wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Add the ability to install PSP Rx and Tx crypto keys on TCP
> connections. Netlink ops are provided for both operations.
> Rx side combines allocating a new Rx key and installing it
> on the socket. Theoretically these are separate actions,
> but in practice they will always be used one after the
> other. We can add distinct "alloc" and "install" ops later.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Co-developed-by: Daniel Zahka <daniel.zahka@gmail.com>
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
> 
> Notes:
>     v3:
>     - lift pse/pas comparison code into new function:
>       psp_pse_matches_pas().
>     - explicitly mark rcu critical section psp_reply_set_decrypted()
>     - use rcu_dereference_proteced() instead of rcu_read_lock() in
>       psp_sk_assoc_free() and psp_twsk_assoc_free()
>     - rename psp_is_nondata() to psp_is_allowed_nondata()
>     - check for sk_is_inet() before casting to inet_twsk()
>     - psp_reply_set_decrypted() should not call psp_sk_assoc()
>     - lift common code into psp_sk_get_assoc_rcu()
>     - Fix exports with EXPORT_IPV6_MOD_GPL()
>     
>     v2:
>     - add pas->dev_id == pse->dev_id to policy checks
>     - __psp_sk_rx_policy_check() now allows pure ACKs, FINs, and RSTs to
>       be non-psp authenticated before "PSP Full" state.
>     - assign tw_validate_skb funtion during psp_twsk_init()
>     - psp_skb_get_rcu() also checks if sk is a tcp timewait sock when
>       looking for psp assocs.
>     - scan ofo queue non-psp data during psp_sock_recv_queue_check()
>     - add tcp_write_collapse_fence() to psp_sock_assoc_set_tx()
>     v1:
>     - https://lore.kernel.org/netdev/20240510030435.120935-7-kuba@kernel.org/
> 
>  Documentation/netlink/specs/psp.yaml |  71 +++++++
>  include/net/psp/functions.h          | 113 +++++++++--
>  include/net/psp/types.h              |  58 ++++++
>  include/uapi/linux/psp.h             |  21 ++
>  net/psp/Kconfig                      |   1 +
>  net/psp/Makefile                     |   2 +-
>  net/psp/psp-nl-gen.c                 |  39 ++++
>  net/psp/psp-nl-gen.h                 |   7 +
>  net/psp/psp.h                        |  22 +++
>  net/psp/psp_main.c                   |  11 +-
>  net/psp/psp_nl.c                     | 244 +++++++++++++++++++++++
>  net/psp/psp_sock.c                   | 276 +++++++++++++++++++++++++++
>  12 files changed, 852 insertions(+), 13 deletions(-)
>  create mode 100644 net/psp/psp_sock.c
> 
> diff --git a/Documentation/netlink/specs/psp.yaml b/Documentation/netlink/specs/psp.yaml
> index 054cc02b65ad..57b24cd6f3f1 100644
> --- a/Documentation/netlink/specs/psp.yaml
> +++ b/Documentation/netlink/specs/psp.yaml
> @@ -38,6 +38,44 @@ attribute-sets:
>          type: u32
>          enum: version
>          enum-as-flags: true
> +  -
> +    name: assoc
> +    attributes:
> +      -
> +        name: dev-id
> +        doc: PSP device ID.
> +        type: u32
> +        checks:
> +          min: 1
> +      -
> +        name: version
> +        doc: |
> +          PSP versions (AEAD and protocol version) used by this association,
> +          dictates the size of the key.
> +        type: u32
> +        enum: version
> +      -
> +        name: rx-key
> +        type: nest
> +        nested-attributes: keys
> +      -
> +        name: tx-key
> +        type: nest
> +        nested-attributes: keys
> +      -
> +        name: sock-fd
> +        doc: Sockets which should be bound to the association immediately.
> +        type: u32
> +  -
> +    name: keys
> +    attributes:
> +      -
> +        name: key
> +        type: binary
> +      -
> +        name: spi
> +        doc: Security Parameters Index (SPI) of the association.
> +        type: u32
>  
>  operations:
>    list:
> @@ -107,6 +145,39 @@ operations:
>        notify: key-rotate
>        mcgrp: use
>  
> +    -
> +      name: rx-assoc
> +      doc: Allocate a new Rx key + SPI pair, associate it with a socket.
> +      attribute-set: assoc
> +      do:
> +        request:
> +          attributes:
> +            - dev-id
> +            - version
> +            - sock-fd
> +        reply:
> +          attributes:
> +            - dev-id
> +            - version

Why return the same values as passed in the request?

> +            - rx-key
> +        pre: psp-assoc-device-get-locked
> +        post: psp-device-unlock
> +    -
> +      name: tx-assoc
> +      doc: Add a PSP Tx association.
> +      attribute-set: assoc
> +      do:
> +        request:
> +          attributes:
> +            - dev-id
> +            - version

Version must be the same for rx and tx alloc. It is already set for
rx, so no need to pass explicitly. Just adds the need to for a sanity
check in the handler.

> +            - tx-key
> +            - sock-fd
> +        reply:
> +          attributes: []
> +        pre: psp-assoc-device-get-locked
> +        post: psp-device-unlock
> +
>  mcast-groups:
>    list:
>      -

> +static inline bool
> +psp_pse_matches_pas(struct psp_skb_ext *pse, struct psp_assoc *pas)
> +{
> +	return pse && pas->rx.spi == pse->spi &&
> +	       pas->generation == pse->generation &&
> +	       pas->version == pse->version &&
> +	       pas->dev_id == pse->dev_id;

Since struct psp_skb_ext is 64 bits, could almost implement this as
a single 64-bit match. The only outlier is spi, which is rx.spi.

Which also indicates that this is psp_pse_matches_pas_rx.

Still, the other three fields could be a single 32b comparison.
Not sure if worth the effort (using a union or cast, say).

> +struct psp_assoc {
> +	struct psp_dev *psd;
> +
> +	u16 dev_id;
> +	u8 generation;
> +	u8 version;
> +	u8 key_sz;

implied by version?

> +	u8 peer_tx;
> +
> +	u32 upgrade_seq;
> +
> +	struct psp_key_parsed tx;
> +	struct psp_key_parsed rx;
> +
> +	refcount_t refcnt;
> +	struct rcu_head rcu;
> +	struct work_struct work;
> +	struct list_head assocs_list;
> +
> +	u8 drv_data[] __aligned(8);
> +};
> +

> +int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
> +				struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct socket *socket;
> +	struct psp_dev *psd;
> +	struct nlattr *id;
> +	struct sock *sk;
> +	int fd, err;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, PSP_A_ASSOC_SOCK_FD))
> +		return -EINVAL;
> +
> +	fd = nla_get_u32(info->attrs[PSP_A_ASSOC_SOCK_FD]);
> +	socket = sockfd_lookup(fd, &err);
> +	if (!socket)
> +		return err;
> +
> +	sk = socket->sk;
> +	if (sk->sk_family != AF_INET && sk->sk_family != AF_INET6) {
> +		NL_SET_ERR_MSG_ATTR(info->extack,
> +				    info->attrs[PSP_A_ASSOC_SOCK_FD],
> +				    "Unsupported socket family");

Should this also check sk_type?

> +
> +struct psp_dev *psd_get_for_sock(struct sock *sk)

Same as other support functions, consider spelling it out fully as
psp_dev_get_for_sock.

> +{
> +	struct dst_entry *dst;
> +	struct psp_dev *psd;
> +
> +	dst = sk_dst_get(sk);
> +	if (!dst)
> +		return NULL;
> +
> +	rcu_read_lock();
> +	psd = rcu_dereference(dst->dev->psp_dev);
> +	if (psd && !psp_dev_tryget(psd))
> +		psd = NULL;
> +	rcu_read_unlock();
> +
> +	dst_release(dst);
> +
> +	return psd;
> +}

> +void psp_reply_set_decrypted(struct sk_buff *skb)
> +{
> +	struct psp_assoc *pas;
> +
> +	rcu_read_lock();
> +	pas = psp_sk_get_assoc_rcu(skb->sk);
> +	if (pas && pas->tx.spi)
> +		skb->decrypted = 1;
> +	rcu_read_unlock();
> +}
> +EXPORT_IPV6_MOD_GPL(psp_reply_set_decrypted)

semicolon

> -- 
> 2.47.1
>