netdev - Re: [PATCH 2/2] xfrm: Remove inner/outer modes from output path

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] [day] [month] [year] [list]

Date: Tue, 28 Nov 2023 12:33:36 +0100
From: Tobias Brunner <tobias@...ongswan.org>
To: Steffen Klassert <steffen.klassert@...unet.com>,
 Herbert Xu <herbert@...dor.apana.org.au>
Cc: netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH 2/2] xfrm: Remove inner/outer modes from output path

Hi Steffen, Herbert,

> @@ -875,21 +875,10 @@ static int xfrm6_extract_output(struct xfrm_state *x, struct sk_buff *skb)
>  
>  static int xfrm_inner_extract_output(struct xfrm_state *x, struct sk_buff *skb)
>  {
> -	const struct xfrm_mode *inner_mode;
> -
> -	if (x->sel.family == AF_UNSPEC)
> -		inner_mode = xfrm_ip2inner_mode(x,
> -				xfrm_af2proto(skb_dst(skb)->ops->family));
> -	else
> -		inner_mode = &x->inner_mode;
> -
> -	if (inner_mode == NULL)
> -		return -EAFNOSUPPORT;
> -
> -	switch (inner_mode->family) {
> -	case AF_INET:
> +	switch (skb->protocol) {
> +	case htons(ETH_P_IP):
>  		return xfrm4_extract_output(x, skb);
> -	case AF_INET6:
> +	case htons(ETH_P_IPV6):
>  		return xfrm6_extract_output(x, skb);
>  	}

The changes in this function indirectly break tunneling IPv4 packets
sent from RAW sockets.  Such packets currently don't have
skb->protocol set, so this results in EAFNOSUPPORT.  For IPv6, this
isn't a problem because the protocol is set since v3.11, or more
specifically 9c9c9ad5fae7 ("ipv6: set skb->protocol on tcp, raw and
ip6_append_data genereated skbs").  For IPv4, that's not the case.  I
wonder why this hasn't been an issue so far (e.g. with MTU calculation
as mentioned in that commit for IPv6).

To fix this, we basically need the patch below.  The question is how
it should be marked with regards to a Fixes: tag.  Should it actually
reference this xfrm-specific commit?  Or should it even get backported
further (e.g. reference the ipv6 commit above)?  (The question then
would be if this change could have unintended side-effects.)

diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 27da9d7294c0..696e1e734729 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -350,6 +350,7 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
 		goto error;
 	skb_reserve(skb, hlen);

+	skb->protocol = htons(ETH_P_IP);
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = sockc->mark;
 	skb->tstamp = sockc->transmit_time;

Regards,
Tobias