netdev - Re: [PATCH net v2] net: ipv6: fix buffer overflow in AH output

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: <20250728113656.GA1367887@horms.kernel.org>
Date: Mon, 28 Jul 2025 12:36:56 +0100
From: Simon Horman <horms@...nel.org>
To: Charalampos Mitrodimas <charmitro@...teo.net>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	syzbot+01b0667934cdceb4451c@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] net: ipv6: fix buffer overflow in AH output

On Sun, Jul 27, 2025 at 09:51:40PM +0000, Charalampos Mitrodimas wrote:
> Fix a buffer overflow where extension headers are incorrectly copied
> to the IPv6 address fields, resulting in a field-spanning write of up
> to 40 bytes into a 16-byte field (IPv6 address).
> 
>   memcpy: detected field-spanning write (size 40) of single field "&top_iph->saddr" at net/ipv6/ah6.c:439 (size 16)
>   WARNING: CPU: 0 PID: 8838 at net/ipv6/ah6.c:439 ah6_output+0xe7e/0x14e0 net/ipv6/ah6.c:439
> 
> The issue occurs in ah6_output() and ah6_output_done() where the code
> attempts to save/restore extension headers by copying them to/from the
> IPv6 source/destination address fields based on the CONFIG_IPV6_MIP6
> setting.
> 
> Reported-by: syzbot+01b0667934cdceb4451c@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=01b0667934cdceb4451c
> Signed-off-by: Charalampos Mitrodimas <charmitro@...teo.net>
> ---
> Changes in v2:
> - Link correct syzbot dashboard link in patch tags
> - Link to v1: https://lore.kernel.org/r/20250727-ah6-buffer-overflow-v1-1-1f3e11fa98db@posteo.net

You posted two versions of this patch within a few minutes.
Please don't do that. Rather, please wait 24h to allow review to occur.

https://docs.kernel.org/process/maintainer-netdev.html

> ---
>  net/ipv6/ah6.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
> index eb474f0987ae016b9d800e9f83d70d73171b21d2..0fa3ed3c64c4ed1a1907d73fb3477e11ef0bd5b8 100644
> --- a/net/ipv6/ah6.c
> +++ b/net/ipv6/ah6.c
> @@ -301,13 +301,8 @@ static void ah6_output_done(void *data, int err)
>  	memcpy(ah->auth_data, icv, ahp->icv_trunc_len);
>  	memcpy(top_iph, iph_base, IPV6HDR_BASELEN);
>  
> -	if (extlen) {
> -#if IS_ENABLED(CONFIG_IPV6_MIP6)
> -		memcpy(&top_iph->saddr, iph_ext, extlen);
> -#else
> -		memcpy(&top_iph->daddr, iph_ext, extlen);
> -#endif
> -	}
> +	if (extlen)
> +		memcpy((u8 *)(top_iph + 1), iph_ext, extlen);

nit: The cast seems unnecessary.

>  
>  	kfree(AH_SKB_CB(skb)->tmp);
>  	xfrm_output_resume(skb->sk, skb, err);

I am somewhat confused about both your description of the problem,
and the solution.

It seems to me that:

1. The existing memcpy (two variants, depending on CONFIG_IPV6_MIP6),
   are copying data to the correct location (else this fetuare would not work).
2. Due to the structure layout of struct ipv6hdr, syzcaller is warning that
   the write overruns he end of the structure.
3. Although that syzcaller is correct about the structure field being too
   small for the data, there is space to write into.

Are these three points correct?

If so, I don't think it is correct to describe this as a buffer overflow
in the patch description. But rather a warning about one, that turns
out to be a false positive. And if so, I think this patch is more of
a clean-up for ipsec-next, rather than a fix for ipsec or net.

Also, if so, I don't think your patch is correct because it changes the
destination address that data is written to from towards the end of
top_iph, to immediately after the end of top_iph (which is further into
overflow territory, if that is the problem).

I'm unsure of a concise way to resolve this problem, but it seems to me
that the following is correct (compile tested only!):

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index eb474f0987ae..5bf22b007053 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -303,10 +303,10 @@ static void ah6_output_done(void *data, int err)

 	if (extlen) {
 #if IS_ENABLED(CONFIG_IPV6_MIP6)
-		memcpy(&top_iph->saddr, iph_ext, extlen);
-#else
-		memcpy(&top_iph->daddr, iph_ext, extlen);
+		top_iph->saddr = iph_ext->saddr;
 #endif
+		top_iph->daddr = iph_ext->daddr;
+		memcpy(top_iph + 1, &iph_ext->hdrs, extlen - sizeof(*iph_ext));
 	}

 	kfree(AH_SKB_CB(skb)->tmp);

I would also suggest adding a helper (or two), to avoid (repeatedly) open
coding whatever approach is taken.

...