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

David CARLIER
2026-05-01T14:39:56Z

On Fri, 1 May 2026 at 15:13, Daniel Zahka <daniel.zahka@gmail.com> wrote:
>
>
> 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>
>

ACK, will resend tomorrow, Cheers !