[PATCH] seg6: Fix slab-out-of-bounds in fl6_update_dst()

[PATCH] seg6: Fix slab-out-of-bounds in fl6_update_dst()

[YueHaibing]

When update flowi6 daddr in fl6_update_dst() for srcrt, the used index
of segments should be segments_left minus one per RFC8754
(section 4.3.1.1) S15 S16. Otherwise it may results in an out-of-bounds
read.

Reported-by: syzbot+e8c028b62439eac42073@syzkaller.appspotmail.com
Fixes: 0cb7498f234e ("seg6: fix SRH processing to comply with RFC8754")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/ipv6/exthdrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5a8bbcdcaf2b..f5304bf33ab1 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1353,7 +1353,7 @@ struct in6_addr *fl6_update_dst(struct flowi6 *fl6,
 	{
 		struct ipv6_sr_hdr *srh = (struct ipv6_sr_hdr *)opt->srcrt;
 
-		fl6->daddr = srh->segments[srh->segments_left];
+		fl6->daddr = srh->segments[srh->segments_left - 1];
 		break;
 	}
 	default:
-- 
2.17.1

Re: [PATCH] seg6: Fix slab-out-of-bounds in fl6_update_dst()

[Eric Dumazet]

On 6/1/20 11:51 PM, YueHaibing wrote:
> When update flowi6 daddr in fl6_update_dst() for srcrt, the used index
> of segments should be segments_left minus one per RFC8754
> (section 4.3.1.1) S15 S16. Otherwise it may results in an out-of-bounds
> read.
> 
> Reported-by: syzbot+e8c028b62439eac42073@syzkaller.appspotmail.com
> Fixes: 0cb7498f234e ("seg6: fix SRH processing to comply with RFC8754")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  net/ipv6/exthdrs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 5a8bbcdcaf2b..f5304bf33ab1 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -1353,7 +1353,7 @@ struct in6_addr *fl6_update_dst(struct flowi6 *fl6,
>  	{
>  		struct ipv6_sr_hdr *srh = (struct ipv6_sr_hdr *)opt->srcrt;
>  
> -		fl6->daddr = srh->segments[srh->segments_left];
> +		fl6->daddr = srh->segments[srh->segments_left - 1];
>  		break;
>  	}
>  	default:
> 

1) Any reason you do not cc the author of the buggy patch ?
   I also cced David Lebrun <david.lebrun@uclouvain.be> to get more eyes.

2) What happens if segments_left == 0 ?

Re: [PATCH] seg6: Fix slab-out-of-bounds in fl6_update_dst()

[Ahmed Abdelsalam]

I’m already working on a fix for this bug.

This patch leads to a bigger semantic problem as it will send SRv6 
packets to the second segment not the first segment (as is does not 
exist in the SRH).

Please see my explanation below.

The main issue is the seg6_validate_srh() which is used to validate SRH 
for two cases:

case1: SRH of data-plane SRv6 packets to be processed /forwarded by the 
Linux kernel.
Case2: SRH of the netlink message received  from user-space (iproute2)

In case1, the SRH can be encoded in the Reduced way and the 
seg6_validate_srh() now handles this case correctly.

In case2, the SRH shouldn’t be encoded in the Reduced way otherwise we 
lose the first segment (i.e., the first hop).

The current issue is that the seg6_validate_srh() allow SRH of case2 to 
be encoded in the Reduced way. Hence the out-of-bounds problem.

I’m working on patch to verify the SRH differently depending if it is 
part of received SRv6 packet or a netlink message.


Ahmed

On 02/06/2020 15:20, Eric Dumazet wrote:
> 
> 
> On 6/1/20 11:51 PM, YueHaibing wrote:
>> When update flowi6 daddr in fl6_update_dst() for srcrt, the used index
>> of segments should be segments_left minus one per RFC8754
>> (section 4.3.1.1) S15 S16. Otherwise it may results in an out-of-bounds
>> read.
>>
>> Reported-by: syzbot+e8c028b62439eac42073@syzkaller.appspotmail.com
>> Fixes: 0cb7498f234e ("seg6: fix SRH processing to comply with RFC8754")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>   net/ipv6/exthdrs.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
>> index 5a8bbcdcaf2b..f5304bf33ab1 100644
>> --- a/net/ipv6/exthdrs.c
>> +++ b/net/ipv6/exthdrs.c
>> @@ -1353,7 +1353,7 @@ struct in6_addr *fl6_update_dst(struct flowi6 *fl6,
>>   	{
>>   		struct ipv6_sr_hdr *srh = (struct ipv6_sr_hdr *)opt->srcrt;
>>   
>> -		fl6->daddr = srh->segments[srh->segments_left];
>> +		fl6->daddr = srh->segments[srh->segments_left - 1];
>>   		break;
>>   	}
>>   	default:
>>
> 
> 1) Any reason you do not cc the author of the buggy patch ?
>     I also cced David Lebrun <david.lebrun@uclouvain.be> to get more eyes.
> 
> 2) What happens if segments_left == 0 ?
>