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 psp_responder.c, conn_setup_psp() fills and exchanges key data
sized for the Rx version, then hands the same buffer to the new…
--
selftests: drv-net: psp: factor out psp connection setup
This commit moves the PSP Rx and Tx association code in psp_responder.c
into new rx_assoc() and tx_assoc() helpers. It also moves the psp.py key
exchange into _establish_psp_conn(), so other tests can reuse the key
exchange and then do more assoc calls.
> diff --git a/tools/testing/selftests/drivers/net/psp_responder.c
> b/tools/testing/selftests/drivers/net/psp_responder.c
> index a26e7628bbb13..57425ecb9561f 100644
> --- a/tools/testing/selftests/drivers/net/psp_responder.c
> +++ b/tools/testing/selftests/drivers/net/psp_responder.c
[ ... ]
> @@ -64,29 +68,34 @@ static int conn_setup_psp(struct ynl_sock *ys, struct
> opts *opts, int data_sock)
> return -1;
> }
>
> - // SPI exchange
> - key_len = rsp->rx_key._len.key;
> - memcpy(info, &rsp->rx_key.spi, sizeof(spi));
> - memcpy(&info[sizeof(spi)], rsp->rx_key.key, key_len);
> - sz = sizeof(spi) + key_len;
> + if (rsp->rx_key._len.key != key_len) {
> + fprintf(stderr, "ERROR: unexpected Rx key length %u\n",
> + rsp->rx_key._len.key);
> + psp_rx_assoc_rsp_free(rsp);
> + return -1;
> + }
[ ... ]
> - // Setup Tx assoc
> teq = psp_tx_assoc_req_alloc();
>
> psp_tx_assoc_req_set_sock_fd(teq, data_sock);
> - psp_tx_assoc_req_set_version(teq, psp_vers.tx);
> + psp_tx_assoc_req_set_version(teq, version);
> psp_tx_assoc_req_set_tx_key_spi(teq, spi);
> - psp_tx_assoc_req_set_tx_key_key(teq, &info[sizeof(spi)], key_len);
> + psp_tx_assoc_req_set_tx_key_key(teq, key, psp_key_len(version));
[ ... ]
> @@ -99,6 +108,34 @@ static int conn_setup_psp(struct ynl_sock *ys, struct
> opts *opts, int data_sock)
> return 0;
> }
>
> +static int conn_setup_psp(struct ynl_sock *ys, struct opts *opts, int
> data_sock)
> +{
> + unsigned int key_len = psp_key_len(psp_vers.rx);
> + char info[300];
> + ssize_t sz;
> + __u32 spi;
> +
> + dbg("create PSP connection\n");
> +
> + if (rx_assoc(ys, &spi, &info[sizeof(spi)], data_sock))
> + return -1;
> +
> + // SPI exchange
> + memcpy(info, &spi, sizeof(spi));
> + sz = sizeof(spi) + key_len;
> +
> + send(data_sock, info, sz, MSG_WAITALL);
> +
> + sz = recv(data_sock, info, sz, MSG_WAITALL);
> + if (sz < 0) {
> + perror("ERROR: failed to read PSP key from sock");
> + return -1;
> + }
> + memcpy(&spi, info, sizeof(spi));
> +
> + return tx_assoc(ys, psp_vers.tx, spi, &info[sizeof(spi)], data_sock);
[Severity: Low]
Can tx_assoc() read uninitialized bytes from info[] here if psp_vers.tx
and psp_vers.rx have different key sizes?
conn_setup_psp() sizes the exchange from the Rx version:
unsigned int key_len = psp_key_len(psp_vers.rx);
...
sz = sizeof(spi) + key_len;
So rx_assoc() and recv() fill only key_len bytes after the SPI. Then
tx_assoc() copies a length based on the Tx version:
psp_tx_assoc_req_set_tx_key_key(teq, key, psp_key_len(version));
The two versions are separate bytes read in run_session(), and nothing
checks that they match:
memcpy(&psp_vers, buf, 2);
With a 256-bit Tx version and a 128-bit Rx version, 16 bytes of the
uninitialized info[] stack array would go into the tx-key attribute.
Before this patch, the Tx key length was rsp->rx_key._len.key, which is
the same length used for the exchange.
The kernel should reject that request in psp_sock_assoc_set_tx() because
the versions don't match. Also, the only in-tree sender, _make_psp_conn()
in psp.py, always sends equal versions. Later patches in the series pass a
fully initialized 32-byte key to tx_assoc() from handle_tx_assoc(), but
they leave conn_setup_psp() as is. So the mismatch is still there at the
end of the series.
Would it make sense to reject Tx/Rx versions with different key sizes in
conn_setup_psp(), or to pass the valid key length to tx_assoc()?
Separately, the commit message calls this a factor-out. However,
rx_assoc() now fails when rsp->rx_key._len.key doesn't match
psp_key_len(psp_vers.rx), and the Tx key length is now computed
differently. Could the commit message mention these behaviour changes?
> +}
> +
> static void send_ack(int sock)
> {
> send(sock, "ack", 4, MSG_WAITALL);
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-psp-v2-0-5596ab50f677%40gmail.com
