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

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

[Charalampos Mitrodimas]

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=01b0667934cdceb4451c
Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.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
---
 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);
 
 	kfree(AH_SKB_CB(skb)->tmp);
 	xfrm_output_resume(skb->sk, skb, err);
@@ -379,11 +374,7 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	memcpy(iph_base, top_iph, IPV6HDR_BASELEN);
 
 	if (extlen) {
-#if IS_ENABLED(CONFIG_IPV6_MIP6)
-		memcpy(iph_ext, &top_iph->saddr, extlen);
-#else
-		memcpy(iph_ext, &top_iph->daddr, extlen);
-#endif
+		memcpy(iph_ext, (u8 *)(top_iph + 1), extlen);
 		err = ipv6_clear_mutable_options(top_iph,
 						 extlen - sizeof(*iph_ext) +
 						 sizeof(*top_iph),
@@ -434,13 +425,8 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	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);
 
 out_free:
 	kfree(iph_base);

---
base-commit: b711733e89a3f84c8e1e56e2328f9a0fa5facc7c
change-id: 20250727-ah6-buffer-overflow-ff795b87398d

Best regards,
-- 
Charalampos Mitrodimas <charmitro@posteo.net>

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

[Simon Horman]

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=01b0667934cdceb4451c
> Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.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.

...

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

[Charalampos Mitrodimas]

Simon Horman <horms@kernel.org> writes:

> 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@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=01b0667934cdceb4451c
>> Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.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.

I'm aware. The reason for posting the second version so soon was because
I did not want people to get confused about which syzbot report this
solves, as the one in v1 was the wrong.

>
> 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.

You're right.

>
>>  
>>  	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?

Yes, you're right. The code works because extension headers come right
after the IPv6 header in memory, and the warning is a false positive. I
now see my patch could break things by only copying extension headers
instead of both addresses and extension headers like the original code
does.

>
> 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.

Yes this can be changed to mention something with "field-spanning memcpy
warning".

>
> 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);

This is much better actually, thanks a lot. I tested it with the syzbot
reproducer and no issues were found.

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

I'll do that and go on with a patch targetting ipsec-next. Is it okay to
keep the the versioning or it should a completely new patch?

>
> ...

Thanks,
C. Mitrodimas

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

[Simon Horman]

On Mon, Jul 28, 2025 at 12:36:18PM +0000, Charalampos Mitrodimas wrote:
> Simon Horman <horms@kernel.org> writes:
> 
> > On Sun, Jul 27, 2025 at 09:51:40PM +0000, Charalampos Mitrodimas wrote:

...

> >> 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.
> 
> I'm aware. The reason for posting the second version so soon was because
> I did not want people to get confused about which syzbot report this
> solves, as the one in v1 was the wrong.

Understood. FWIIW, I think it would have been better
to respond to v1 with corrected syzbot information.

...

> This is much better actually, thanks a lot. I tested it with the syzbot
> reproducer and no issues were found.

Excellent.

> > I would also suggest adding a helper (or two), to avoid (repeatedly) open
> > coding whatever approach is taken.
>
> I'll do that and go on with a patch targetting ipsec-next. Is it okay to
> keep the the versioning or it should a completely new patch?

I think that keeping the versioning is fine, although it is up to you.
If you do so, please do include a link to earlier versions
(as you did in this patch) as I assume the subject will change.

-- 
pw-bot: cr