Re: [PATCH] psp: reject packets carrying unsupported PSP optional fields

[Daniel Zahka]

On 4/30/26 2:20 AM, David Carlier wrote:
> psp_dev_rcv() documents that it does not support optional PSP fields
> but never enforces it. The helper unconditionally strips a fixed
> PSP_ENCAP_HLEN, so a frame whose PSP header carries options is
> silently mis-decapsulated: option bytes spill into the inner packet
> head and parsing fails downstream on a corrupted skb instead of being
> rejected early.
>
> Validate hdrlen, crypt_offset and PSPHDR_VERFL_VIRT, and hoist the
> psph read above skb_ext_add() so rejected packets do not pick up an
> SKB_EXT_PSP extension only to drop it. Both in-tree callers gate on
> hardware-validated, opt-less PSP, so this is hardening rather than a
> reachable corruption path.
>
> Fixes: 0eddb8023cee ("psp: provide decapsulation and receive helper for drivers")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> ---
>   net/psp/psp_main.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 524978dfb8fd..53d7e14c054a 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -321,12 +321,20 @@ 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;
>   
> +	psph = (struct psphdr *)(skb->data + l2_hlen + l3_hlen +
> +				 sizeof(struct udphdr));
> +
> +	/* Fixed-length decap; reject optional fields rather than mis-decapsulate. */
> +
> +	if (unlikely(psph->hdrlen != PSP_HDRLEN_NOOPT ||
> +		     psph->crypt_offset ||
> +		     (psph->verfl & PSPHDR_VERFL_VIRT)))
> +		return -EINVAL;
> +
>   	pse = skb_ext_add(skb, SKB_EXT_PSP);
>   	if (!pse)
>   		return -EINVAL;
>   
> -	psph = (struct psphdr *)(skb->data + l2_hlen + l3_hlen +
> -				 sizeof(struct udphdr));
>   	pse->spi = psph->spi;
>   	pse->dev_id = dev_id;
>   	pse->generation = generation;


Thanks. There is a bit of a gray area in regards to how much we need to 
validate here, because callers ought to use this only downstream of a 
real PSP device, and only when that device has emitted some metadata 
that the skb was at least parsed as PSP and decrypted correctly. That 
being said, the handling of psp hdrlen is definitely a bug, because even 
valid values can cause corruption during decap as you noted. I think we 
should fix it by validating that it is less than the remaining bytes 
after the psp-udp header, and then stripping the correct header length 
accordingly.


For the other two, I'm not sure they are really necessary. Mandating 
that crypt off is 0 is too restrictive as this function should work just 
fine with non-zero values. We could validate that it isn't too long or 
misaligned with the payload, but it may be better to have callers know 
their NICs PSP implementation and decide if that is necessary. Similar 
story with the VC present bit.


In any case, I think this function will also need a comment update 
giving some more context for caller, and we can mention that the whole 
psp header will be stripped away according the psp hdrlen, and that any 
vc or options will be ignored and discarded.


For a fix, you'll need to target the net tree with this patch, (i.e. 
"PATCH net").