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

[David CARLIER]

On Fri, 1 May 2026 at 15:13, Daniel Zahka <daniel.zahka@xxxxxxxxx> 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@xxxxxxxxx>
>
>
> No need for the suggested tag here.
>
>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>

ACK, will resend tomorrow, Cheers !