Re: [PATCH net-next 1/3] psp: validate protocol before mutating skb in psp_dev_encapsulate()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jakub Kicinski wrote:
> Code checkers / AI scans will complain that we have already modified
> the packet by the time we realize that protocol is not IP.
> 
> Move the skb->protocol check to before skb_push()/memmove() so that
> the skb is not left in a corrupted state when the function returns
> false for an unsupported protocol. psp_dev_rcv() follows similar
> pattern.
> 
> Today this path is unreachable because both in-tree callers (mlx5 and
> netdevsim) only reach psp_dev_encapsulate() from TCP socket TX paths
> where skb->protocol is always ETH_P_IP or ETH_P_IPV6, and both drop
> the skb on a false return, anyway.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  net/psp/psp_main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 9508b6c38003..652ec8a9c8a4 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -228,6 +228,10 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  	u32 ethr_len = skb_mac_header_len(skb);
>  	u32 bufflen = ethr_len + network_len;
>  
> +	if (skb->protocol != htons(ETH_P_IP) &&
> +	    skb->protocol != htons(ETH_P_IPV6))
> +		return false;
> +
>  	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
>  		return false;
>  
> @@ -243,11 +247,9 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  		ip_hdr(skb)->check = 0;
>  		ip_hdr(skb)->check =
>  			ip_fast_csum((u8 *)ip_hdr(skb), ip_hdr(skb)->ihl);
> -	} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +	} else {
>  		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
>  		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
> -	} else {
> -		return false;
>  	}
>  
>  	skb_set_inner_ipproto(skb, IPPROTO_TCP);
> -- 
> 2.54.0
>