Re: [PATCH net v2] psp: strip variable-length PSP header in psp_dev_rcv()

Daniel Zahka
2026-05-01T14:13:54Z

On 5/1/26 9:00 AM, David Carlier wrote:
> psp_dev_rcv() unconditionally removes a fixed PSP_ENCAP_HLEN, even
> when psph->hdrlen indicates that the PSP header carries optional
> fields. A frame whose PSP header advertises a non-zero VC or any
> extension would therefore be silently mis-decapsulated: option bytes
> would spill into the inner packet head and downstream parsing would
> fail on a corrupted skb.
>
> Compute the full PSP header length from psph->hdrlen, pull the
> optional bytes into the linear region, and strip the whole header
> when decapsulating. Optional fields (VC, ...) are still ignored,
> just discarded with the rest of the header instead of leaking.
> crypt_offset and the VIRT flag are intentionally not validated here
> - callers know their device's PSP implementation and can decide.
>
> Both in-tree callers gate on hardware-validated PSP, so this is a
> correctness fix rather than a reachable corruption path under
> current configurations.
>
> Fixes: 0eddb8023cee ("psp: provide decapsulation and receive helper for drivers")
> Suggested-by: Daniel Zahka <daniel.zahka@gmail.com>

No need for the suggested tag here.

> Cc: stable@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> v1 -> v2 (per Daniel Zahka):
> - strip the variable-length PSP header (psph->hdrlen) instead of
> rejecting opt-bearing frames; VC/options are ignored, not refused
> - drop the crypt_offset and PSPHDR_VERFL_VIRT checks
> - refresh kerneldoc above psp_dev_rcv()
> - retarget at net (was net-next)
>
> net/psp/psp_main.c | 41 +++++++++++++++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 9508b6c38003..b040345d7273 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -263,15 +263,17 @@ EXPORT_SYMBOL(psp_dev_encapsulate);
>
> /* Receive handler for PSP packets.
> *
> - * Presently it accepts only already-authenticated packets and does not
> - * support optional fields, such as virtualization cookies. The caller should
> - * ensure that skb->data is pointing to the mac header, and that skb->mac_len
> - * is set. This function does not currently adjust skb->csum (CHECKSUM_COMPLETE
> - * is not supported).
> + * Accepts only already-authenticated packets. The full PSP header is
> + * stripped according to psph->hdrlen; any optional fields it advertises
> + * (virtualization cookies, etc.) are ignored and discarded along with the
> + * rest of the header. The caller should ensure that skb->data is pointing
> + * to the mac header, and that skb->mac_len is set. This function does not
> + * currently adjust skb->csum (CHECKSUM_COMPLETE is not supported).
> */
> int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
> {
> int l2_hlen = 0, l3_hlen, encap;
> + u32 psp_hdr_len;

There is a style convention in the networking subsystem that
declarations are sorted longest to shortest from top to bottom. Let's
maintain that here.

nit: int psp_hlen might be more consistent with the types/names of the
other local vars.

> struct psp_skb_ext *pse;
> struct psphdr *psph;
> struct ethhdr *eth;
> @@ -312,18 +314,36 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
> if (unlikely(uh->dest != htons(PSP_DEFAULT_UDP_PORT)))
> return -EINVAL;
>
> - pse = skb_ext_add(skb, SKB_EXT_PSP);
> - if (!pse)
> + psph = (struct psphdr *)(skb->data + l2_hlen + l3_hlen +
> + sizeof(struct udphdr));
> +
> + /* Strip the full PSP header per psph->hdrlen; VC/options are pulled
> + * into the linear region only so they can be discarded with the
> + * rest of the header.
> + */
> + psp_hdr_len = ((u32)psph->hdrlen + 1) * 8;

I don't believe casting psph->hdrlen to u32 is necessary for correctness
here.

> +
> + if (unlikely(psp_hdr_len < sizeof(struct psphdr)))
> + return -EINVAL;
> +
> + if (psp_hdr_len > sizeof(struct psphdr) &&
> + !pskb_may_pull(skb, l2_hlen + l3_hlen +
> + sizeof(struct udphdr) + psp_hdr_len))
> return -EINVAL;
>
> psph = (struct psphdr *)(skb->data + l2_hlen + l3_hlen +
> sizeof(struct udphdr));
> +
> + pse = skb_ext_add(skb, SKB_EXT_PSP);
> + if (!pse)
> + return -EINVAL;
> +
> pse->spi = psph->spi;
> pse->dev_id = dev_id;
> pse->generation = generation;
> pse->version = FIELD_GET(PSPHDR_VERFL_VERSION, psph->verfl);
>
> - encap = PSP_ENCAP_HLEN;
> + encap = sizeof(struct udphdr) + psp_hdr_len;
> encap += strip_icv ? PSP_TRL_SIZE : 0;
>
> if (proto == htons(ETH_P_IP)) {
> @@ -340,8 +360,9 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
> ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) - encap);
> }
>
> - memmove(skb->data + PSP_ENCAP_HLEN, skb->data, l2_hlen + l3_hlen);
> - skb_pull(skb, PSP_ENCAP_HLEN);
> + memmove(skb->data + sizeof(struct udphdr) + psp_hdr_len,
> + skb->data, l2_hlen + l3_hlen);
> + skb_pull(skb, sizeof(struct udphdr) + psp_hdr_len);
>
> if (strip_icv)
> pskb_trim(skb, skb->len - PSP_TRL_SIZE);

Minor comments, but otherwise lgtm.

Reviewed-by: Daniel Zahka <daniel.zahka@gmail.com>