Re: [PATCH net-next v1 04/15] psp: add support for socket-less psp associations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message misdescribes how keys are handled by the
  new socket-less association API.

--

psp: add support for socket-less psp associations

This commit adds a registry of PSP associations that are not bound to a
socket. Each one is either an Rx or a Tx association and is referenced
by an ID. It also adds the rx-spi-alloc, assoc-add, assoc-del and
assoc-get netlink commands, which are admin-only, to manage these
associations.

> key material enters through netlink and is never reported back. no key
> is stored for rx associations, and users reference associations by id
> and never handle keys

[Severity: Medium]
Is this paragraph accurate? The code in this patch seems to do the
opposite in two places.

psp_nl_rx_spi_alloc_doit() in net/psp/psp_nl.c returns the
device-derived Rx key to user space:

net/psp/psp_nl.c:psp_nl_rx_spi_alloc_doit() {
    ...
	if (nla_put_u32(rsp, PSP_A_ASSOC_ID, pas->id) ||
	    nla_put_u32(rsp, PSP_A_ASSOC_DEV_ID, psd->id) ||
	    psp_nl_put_key(rsp, PSP_A_ASSOC_RX_KEY, version, &key)) {
    ...
}

psp_nl_put_key() emits PSP_A_KEYS_KEY with key_sz raw key bytes. The
rx-spi-alloc doc in psp.yaml says the same thing: the key "must be
passed to the peer out of band".

psp_nl_assoc_add_doit() also requires user space to supply the raw Tx
key bytes:

net/psp/psp_nl.c:psp_nl_assoc_add_doit() {
    ...
	err = psp_nl_parse_key(info, PSP_A_ASSOC_TX_KEY, &key, key_sz);
    ...
}

So user space does handle keys, and rx-spi-alloc does report key
material back.

The code only seems to guarantee two narrower things. No Rx key is
stored in the association, and assoc-get reports only the SPI.

Could the commit message describe that more precisely? For example:
"the Rx key is returned once, by rx-spi-alloc, and is never stored or
reported by assoc-get".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com