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

[Daniel Zahka]

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>