Re: [PATCH net-next v3 5/5] selftests: drv-net: psp: add rekey tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/drivers/net/psp.py, the second half of
  rekey_both_sides() (the "# now, tx then rx" block, lines 734-736) does…

--

selftests: drv-net: psp: add rekey tests

This commit adds psp.py selftests for rekeying PSP connections. They cover
basic Rx and Tx rekeys, plus cases where rx-assoc and tx-assoc should fail
because of a psp version mismatch or the psp state. It also adds rx-assoc,
tx-assoc and key-rotate RPCs to psp_responder.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index 71eeade62ad24..004faf4bb57ae 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> @@ -489,6 +558,186 @@ def _data_basic_send(cfg, version, ipver):

[ ... ]

> +@ksft_variants(_get_psp_ver_variants())
> +def rekey_both_sides(cfg, version):
> +    """Test rekeying both directions"""
> +    _init_psp_dev(cfg)
> +
> +    s = _establish_psp_conn(cfg, version)
> +    try:
> +        data_len = _psp_txrx(cfg, s, 10)
> +
> +        cfg.pspnl.key_rotate({"id": cfg.psp_dev_id})
> +        _remote_key_rotate(cfg)
> +
> +        # first, rx then tx
> +        data_len = _rekey_rx(cfg, s, version, data_len)
> +        data_len = _rekey_tx(cfg, s, data_len)
> +
> +        cfg.pspnl.key_rotate({"id": cfg.psp_dev_id})
> +        _remote_key_rotate(cfg)
> +
> +        # now, tx then rx
> +        data_len = _rekey_tx(cfg, s, data_len)
> +        _rekey_rx(cfg, s, version, data_len)

[Severity: Low]
Does the "tx then rx" half of rekey_both_sides() check that either of its
Tx rekeys actually took effect?

After an Rx rekey, the old SPI stays acceptable:

net/psp/psp_sock.c:psp_sock_rx_rekey() {
    ...
	pas->prev_spi = prev->rx.spi;
	pas->prev_generation = prev->generation;

	memcpy(&pas->tx, &prev->tx, sizeof(pas->tx));
    ...
}

psp_pse_matches_pas() accepts packets that match it:

include/net/psp/functions.h:psp_pse_matches_pas() {
    ...
		(pas->prev_spi && pas->prev_spi == pse->spi &&
		 pas->prev_generation == pse->generation));
}

The old SPI only becomes invalid when the device key is rotated:

net/psp/psp_sock.c:psp_assocs_key_rotated() {
    ...
	list_for_each_entry(pas, &psd->active_assocs, assocs_list) {
		if ((pas->rx.spi ^ pas->prev_spi) &
		    cpu_to_be32(PSP_SPI_KEY_PHASE))
			pas->prev_generation |= ~PSP_GEN_VALID_MASK;
	}
    ...
}

In the first half, the key_rotate calls that come next cause a stale Tx
SPI to be dropped. The second half ends right after _rekey_rx(), and
neither side rotates again.

Say the local tx_assoc in _rekey_tx() were silently ignored. The local
side would keep sending with the old SPI, and the responder would still
accept it through prev_spi. The same applies if the responder ignored
the tx assoc sent by _remote_tx_assoc() in _rekey_rx(): its echo would
still be accepted locally.

This half is also the only place where an Rx rekey follows a Tx rekey on
the same socket, which is the memcpy of prev->tx shown above. If that
path lost the Tx key that was just installed, would this test notice?

The v3 changelog says the basic tests now "rotate again after it to
verify the new key is in use". Could this test end the same way, with a
final cfg.pspnl.key_rotate() and _remote_key_rotate() and then another
_psp_txrx()?

> +    finally:
> +        _close_psp_conn(cfg, s)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-psp-v3-0-a8dd38f5a727%40gmail.com