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

[Charalampos Mitrodimas <charmitro@posteo.net>]

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