Re: [net-next,1/9] psp: support rx rekey operation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
psp: support rx rekey operation

This patch adds support for rx-assoc netlink operation on an already keyed
socket, enabling rx rekey by splicing a new psp_assoc object with newly
generated rx state and reused tx state.

> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index f785672b7df6..3a8abd023f99 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c

[ ... ]

> @@ -129,30 +130,42 @@ void psp_sk_assoc_free(struct sock *sk)
>  	psp_assoc_put(pas);
>  }
>
> -int psp_sock_assoc_set_rx(struct sock *sk, struct psp_assoc *pas,
> -			  struct psp_key_parsed *key,
> -			  struct netlink_ext_ack *extack)
> +static void psp_sock_rx_rekey(struct psp_assoc *pas, struct psp_assoc *prev)
>  {
> -	int err;
> +	lockdep_assert_held(&pas->psd->lock);
> +
> +	pas->peer_tx = prev->peer_tx;
> +	pas->upgrade_seq = prev->upgrade_seq;
> +
> +	/* steal refcount from sk->psp_assoc */
> +	pas->prev = prev;
> +
> +	memcpy(&pas->tx, &prev->tx, sizeof(pas->tx));
> +	memcpy(pas->drv_data, prev->drv_data, pas->psd->caps->assoc_drv_spc);

Is there a check that prev->psd == pas->psd before this memcpy?

The psp_assoc_device_get_locked() pre_doit handler obtains the PSP device from
the socket's current destination route via psp_dev_get_for_sock(), but this
may differ from the device used when the existing association (prev) was
created if the route changed.

The memcpy copies pas->psd->caps->assoc_drv_spc bytes from prev->drv_data, but
if prev->psd has a smaller assoc_drv_spc, this could read beyond the bounds of
prev->drv_data.

For comparison, psp_sock_assoc_set_tx() has explicit device validation:

    if (pas->psd != psd)
        NL_SET_ERR_MSG(extack, "Rx key from different device");

Should psp_sock_rx_rekey() have a similar check?

> +	prev->tx_moved = true;