Re: [PATCH net-next v2 5/7] selftests: drv-net: psp: factor out psp connection setup
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-10 21:43:48
Also in:
linux-doc, linux-kselftest, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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; + }
[ ... ]
quoted hunk ↗ jump to hunk
- // 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));
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
+} + 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