netdev - Re: [PATCH ipsec 4/4] xfrm: Support crypto offload for outbound IPv4 UDP-encapsulated ESP packet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZovNzu58oGJv1plS@gauss3.secunet.de>
Date: Mon, 8 Jul 2024 13:30:22 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Mike Yu <yumike@...gle.com>
CC: <netdev@...r.kernel.org>, <stanleyjhu@...gle.com>, <martinwu@...gle.com>,
	<chiachangwang@...gle.com>
Subject: Re: [PATCH ipsec 4/4] xfrm: Support crypto offload for outbound IPv4
 UDP-encapsulated ESP packet

On Tue, Jul 02, 2024 at 04:44:51PM +0800, Mike Yu wrote:
> esp_xmit() is already able to handle UDP encapsulation through the call to
> esp_output_head(). The missing part in esp_xmit() is to correct the outer
> IP header.
> 
> Test: Enabled both dir=in/out IPsec crypto offload, and verified IPv4
>       UDP-encapsulated ESP packets on both wifi/cellular network
> Signed-off-by: Mike Yu <yumike@...gle.com>
> ---
>  net/ipv4/esp4.c         |  7 ++++++-
>  net/ipv4/esp4_offload.c | 14 +++++++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index 3968d3f98e08..cd4b52e131ce 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -349,6 +349,7 @@ static struct ip_esp_hdr *esp_output_udp_encap(struct sk_buff *skb,
>  {
>  	struct udphdr *uh;
>  	unsigned int len;
> +	struct xfrm_offload *xo = xfrm_offload(skb);
>  
>  	len = skb->len + esp->tailen - skb_transport_offset(skb);
>  	if (len + sizeof(struct iphdr) > IP_MAX_MTU)
> @@ -360,7 +361,11 @@ static struct ip_esp_hdr *esp_output_udp_encap(struct sk_buff *skb,
>  	uh->len = htons(len);
>  	uh->check = 0;
>  
> -	*skb_mac_header(skb) = IPPROTO_UDP;
> +	// For IPv4 ESP with UDP encapsulation, if xo is not null, the skb is in the crypto offload
> +	// data path, which means that esp_output_udp_encap is called outside of the XFRM stack.
> +	// In this case, the mac header doesn't point to the IPv4 protocol field, so don't set it.

Please use networking style comments.

> +	if (!xo || encap_type != UDP_ENCAP_ESPINUDP)
> +		*skb_mac_header(skb) = IPPROTO_UDP;
>  
>  	return (struct ip_esp_hdr *)(uh + 1);
>  }
> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
> index b3271957ad9a..ccfc466ddf6c 100644
> --- a/net/ipv4/esp4_offload.c
> +++ b/net/ipv4/esp4_offload.c
> @@ -264,6 +264,7 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
>  	struct esp_info esp;
>  	bool hw_offload = true;
>  	__u32 seq;
> +	int encap_type = 0;
>  
>  	esp.inplace = true;
>  
> @@ -296,8 +297,10 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
>  
>  	esp.esph = ip_esp_hdr(skb);
>  
> +	if (x->encap)
> +		encap_type = x->encap->encap_type;
>  
> -	if (!hw_offload || !skb_is_gso(skb)) {
> +	if (!hw_offload || !skb_is_gso(skb) || (hw_offload && encap_type == UDP_ENCAP_ESPINUDP)) {
>  		esp.nfrags = esp_output_head(x, skb, &esp);
>  		if (esp.nfrags < 0)
>  			return esp.nfrags;
> @@ -324,6 +327,15 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
>  
>  	esp.seqno = cpu_to_be64(seq + ((u64)xo->seq.hi << 32));
>  
> +	if (hw_offload && encap_type == UDP_ENCAP_ESPINUDP) {
> +		// In the XFRM stack, the encapsulation protocol is set to iphdr->protocol by
> +		// setting *skb_mac_header(skb) (see esp_output_udp_encap()) where skb->mac_header
> +		// points to iphdr->protocol (see xfrm4_tunnel_encap_add()).
> +		// However, in esp_xmit(), skb->mac_header doesn't point to iphdr->protocol.
> +		// Therefore, the protocol field needs to be corrected.

Same here.