Re: [PATCH v3 10/19] psp: track generations of device key

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

Daniel Zahka wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> There is a (somewhat theoretical in absence of multi-host support)
> possibility that another entity will rotate the key and we won't
> know. This may lead to accepting packets with matching SPI but
> which used different crypto keys than we expected. 

The device would not have decrypted those? As it only has two keys,
one for each MSB of the SPI.

Except for a narrow window during rotation, where a key for generation
N is decrypted and queued to the host, then a rotation happens, so that
the host updates its valid keys to { N+1, N+2 }. These will now get
dropped. That is not strictly necessary.

> Maintain and
> compare "key generation" per PSP spec.

Where does the spec state this?

I know this generation bit is present in the Google PSP
implementation, I'm just right now drawing a blank as to its exact
purpose -- and whether the above explanation matches that.

> Since we're tracking "key generations" more explicitly now,
> maintain different lists for associations from different generations.
> This way we can catch stale associations (the user space should
> listen to rotation notifications and change the keys).
> 
> Drivers can "opt out" of generation tracking by setting
> the generation value to 0.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
> 
> Notes:
>     v1:
>     - https://lore.kernel.org/netdev/20240510030435.120935-9-kuba@kernel.org/
> 
>  include/net/psp/types.h | 10 ++++++++++
>  net/psp/psp.h           |  1 +
>  net/psp/psp_main.c      |  6 +++++-
>  net/psp/psp_nl.c        | 10 ++++++++++
>  net/psp/psp_sock.c      | 16 ++++++++++++++++
>  5 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/psp/types.h b/include/net/psp/types.h
> index 5b0c2474a042..383a1afab46d 100644
> --- a/include/net/psp/types.h
> +++ b/include/net/psp/types.h
> @@ -50,8 +50,12 @@ struct psp_dev_config {
>   * @lock:	instance lock, protects all fields
>   * @refcnt:	reference count for the instance
>   * @id:		instance id
> + * @generation:	current generation of the device key
>   * @config:	current device configuration
>   * @active_assocs:	list of registered associations
> + * @prev_assocs:	associations which use old (but still usable)
> + *			device key
> + * @stale_assocs:	associations which use a rotated out key
>   *
>   * @rcu:	RCU head for freeing the structure
>   */
> @@ -67,13 +71,19 @@ struct psp_dev {
>  
>  	u32 id;
>  
> +	u8 generation;
> +
>  	struct psp_dev_config config;
>  
>  	struct list_head active_assocs;
> +	struct list_head prev_assocs;
> +	struct list_head stale_assocs;
>  
>  	struct rcu_head rcu;
>  };
>  
> +#define PSP_GEN_VALID_MASK	0x7f
> +
>  /**
>   * struct psp_dev_caps - PSP device capabilities
>   */
> diff --git a/net/psp/psp.h b/net/psp/psp.h
> index b4092936bc64..a511ec85e1c7 100644
> --- a/net/psp/psp.h
> +++ b/net/psp/psp.h
> @@ -27,6 +27,7 @@ int psp_sock_assoc_set_rx(struct sock *sk, struct psp_assoc *pas,
>  int psp_sock_assoc_set_tx(struct sock *sk, struct psp_dev *psd,
>  			  u32 version, struct psp_key_parsed *key,
>  			  struct netlink_ext_ack *extack);
> +void psp_assocs_key_rotated(struct psp_dev *psd);
>  
>  static inline void psp_dev_get(struct psp_dev *psd)
>  {
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index bf1c704a1a40..99facb158abb 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -72,6 +72,8 @@ psp_dev_create(struct net_device *netdev,
>  
>  	mutex_init(&psd->lock);
>  	INIT_LIST_HEAD(&psd->active_assocs);
> +	INIT_LIST_HEAD(&psd->prev_assocs);
> +	INIT_LIST_HEAD(&psd->stale_assocs);
>  	refcount_set(&psd->refcnt, 1);
>  
>  	mutex_lock(&psp_devs_lock);
> @@ -120,7 +122,9 @@ void psp_dev_unregister(struct psp_dev *psd)
>  	xa_store(&psp_devs, psd->id, NULL, GFP_KERNEL);
>  	mutex_unlock(&psp_devs_lock);
>  
> -	list_for_each_entry_safe(pas, next, &psd->active_assocs, assocs_list)
> +	list_splice_init(&psd->active_assocs, &psd->prev_assocs);
> +	list_splice_init(&psd->prev_assocs, &psd->stale_assocs);
> +	list_for_each_entry_safe(pas, next, &psd->stale_assocs, assocs_list)
>  		psp_dev_tx_key_del(psd, pas);
>  
>  	rcu_assign_pointer(psd->main_netdev->psp_dev, NULL);
> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index 58508e642185..7b8a1d390cde 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -230,6 +230,7 @@ int psp_nl_key_rotate_doit(struct sk_buff *skb, struct genl_info *info)
>  	struct psp_dev *psd = info->user_ptr[0];
>  	struct genl_info ntf_info;
>  	struct sk_buff *ntf, *rsp;
> +	u8 prev_gen;
>  	int err;
>  
>  	rsp = psp_nl_reply_new(info);
> @@ -249,10 +250,19 @@ int psp_nl_key_rotate_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto err_free_ntf;
>  	}
>  
> +	/* suggest the next gen number, driver can override */
> +	prev_gen = psd->generation;
> +	psd->generation = (prev_gen + 1) & PSP_GEN_VALID_MASK;
> +
>  	err = psd->ops->key_rotate(psd, info->extack);
>  	if (err)
>  		goto err_free_ntf;
>  
> +	WARN_ON_ONCE((psd->generation && psd->generation == prev_gen) ||
> +		     psd->generation & ~PSP_GEN_VALID_MASK);
> +
> +	psp_assocs_key_rotated(psd);
> +
>  	nlmsg_end(ntf, (struct nlmsghdr *)ntf->data);
>  	genlmsg_multicast_netns(&psp_nl_family, dev_net(psd->main_netdev), ntf,
>  				0, PSP_NLGRP_USE, GFP_KERNEL);
> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index f97441935d12..8b5cb31c2836 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
> @@ -60,6 +60,7 @@ struct psp_assoc *psp_assoc_create(struct psp_dev *psd)
>  
>  	pas->psd = psd;
>  	pas->dev_id = psd->id;
> +	pas->generation = psd->generation;
>  	psp_dev_get(psd);
>  	refcount_set(&pas->refcnt, 1);
>  
> @@ -243,6 +244,21 @@ int psp_sock_assoc_set_tx(struct sock *sk, struct psp_dev *psd,
>  	return err;
>  }
>  
> +void psp_assocs_key_rotated(struct psp_dev *psd)
> +{
> +	struct psp_assoc *pas, *next;
> +
> +	/* Mark the stale associations as invalid, they will no longer
> +	 * be able to Rx any traffic.
> +	 */
> +	list_for_each_entry_safe(pas, next, &psd->prev_assocs, assocs_list)
> +		pas->generation |= ~PSP_GEN_VALID_MASK;
> +	list_splice_init(&psd->prev_assocs, &psd->stale_assocs);
> +	list_splice_init(&psd->active_assocs, &psd->prev_assocs);
> +
> +	/* TODO: we should inform the sockets that got shut down */
> +}
> +
>  void psp_twsk_init(struct inet_timewait_sock *tw, struct sock *sk)
>  {
>  	struct psp_assoc *pas = psp_sk_assoc(sk);
> -- 
> 2.47.1
>