[PATCH net-next] ipv6: fix access to non-linear packet in ndisc_fill_redirect_hdr_option()

[PATCH net-next] ipv6: fix access to non-linear packet in ndisc_fill_redirect_hdr_option()

[Lorenzo Bianconi]

Fix the following slab-out-of-bounds kasan report in
ndisc_fill_redirect_hdr_option when the incoming ipv6 packet is not
linear and the accessed data are not in the linear data region of orig_skb

[ 1503.122508] ==================================================================
[ 1503.122832] BUG: KASAN: slab-out-of-bounds in ndisc_send_redirect+0x94e/0x990
[ 1503.123036] Read of size 1184 at addr ffff8800298ab6b0 by task netperf/1932

[ 1503.123220] CPU: 0 PID: 1932 Comm: netperf Not tainted 4.16.0-rc2+ #124
[ 1503.123347] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-2.fc27 04/01/2014
[ 1503.123527] Call Trace:
[ 1503.123579]  <IRQ>
[ 1503.123638]  print_address_description+0x6e/0x280
[ 1503.123849]  kasan_report+0x233/0x350
[ 1503.123946]  memcpy+0x1f/0x50
[ 1503.124037]  ndisc_send_redirect+0x94e/0x990
[ 1503.125150]  ip6_forward+0x1242/0x13b0
[...]
[ 1503.153890] Allocated by task 1932:
[ 1503.153982]  kasan_kmalloc+0x9f/0xd0
[ 1503.154074]  __kmalloc_track_caller+0xb5/0x160
[ 1503.154198]  __kmalloc_reserve.isra.41+0x24/0x70
[ 1503.154324]  __alloc_skb+0x130/0x3e0
[ 1503.154415]  sctp_packet_transmit+0x21a/0x1810
[ 1503.154533]  sctp_outq_flush+0xc14/0x1db0
[ 1503.154624]  sctp_do_sm+0x34e/0x2740
[ 1503.154715]  sctp_primitive_SEND+0x57/0x70
[ 1503.154807]  sctp_sendmsg+0xaa6/0x1b10
[ 1503.154897]  sock_sendmsg+0x68/0x80
[ 1503.154987]  ___sys_sendmsg+0x431/0x4b0
[ 1503.155078]  __sys_sendmsg+0xa4/0x130
[ 1503.155168]  do_syscall_64+0x171/0x3f0
[ 1503.155259]  entry_SYSCALL_64_after_hwframe+0x42/0xb7

[ 1503.155436] Freed by task 1932:
[ 1503.155527]  __kasan_slab_free+0x134/0x180
[ 1503.155618]  kfree+0xbc/0x180
[ 1503.155709]  skb_release_data+0x27f/0x2c0
[ 1503.155800]  consume_skb+0x94/0xe0
[ 1503.155889]  sctp_chunk_put+0x1aa/0x1f0
[ 1503.155979]  sctp_inq_pop+0x2f8/0x6e0
[ 1503.156070]  sctp_assoc_bh_rcv+0x6a/0x230
[ 1503.156164]  sctp_inq_push+0x117/0x150
[ 1503.156255]  sctp_backlog_rcv+0xdf/0x4a0
[ 1503.156346]  __release_sock+0x142/0x250
[ 1503.156436]  release_sock+0x80/0x180
[ 1503.156526]  sctp_sendmsg+0xbb0/0x1b10
[ 1503.156617]  sock_sendmsg+0x68/0x80
[ 1503.156708]  ___sys_sendmsg+0x431/0x4b0
[ 1503.156799]  __sys_sendmsg+0xa4/0x130
[ 1503.156889]  do_syscall_64+0x171/0x3f0
[ 1503.156980]  entry_SYSCALL_64_after_hwframe+0x42/0xb7

[ 1503.157158] The buggy address belongs to the object at ffff8800298ab600
                which belongs to the cache kmalloc-1024 of size 1024
[ 1503.157444] The buggy address is located 176 bytes inside of
                1024-byte region [ffff8800298ab600, ffff8800298aba00)
[ 1503.157702] The buggy address belongs to the page:
[ 1503.157820] page:ffffea0000a62a00 count:1 mapcount:0 mapping:0000000000000000 index:0x0 compound_mapcount: 0
[ 1503.158053] flags: 0x4000000000008100(slab|head)
[ 1503.158171] raw: 4000000000008100 0000000000000000 0000000000000000 00000001800e000e
[ 1503.158350] raw: dead000000000100 dead000000000200 ffff880036002600 0000000000000000
[ 1503.158523] page dumped because: kasan: bad access detected

[ 1503.158698] Memory state around the buggy address:
[ 1503.158816]  ffff8800298ab900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1503.158988]  ffff8800298ab980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1503.159165] >ffff8800298aba00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 1503.159338]                    ^
[ 1503.159436]  ffff8800298aba80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1503.159610]  ffff8800298abb00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1503.159785] ==================================================================
[ 1503.159964] Disabling lock debugging due to kernel taint

Reported-by: Jianlin Shi <jishi@redhat.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/ipv6/ndisc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 0a19ce3a6f7f..afd8c15827cd 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1554,7 +1554,8 @@ static void ndisc_fill_redirect_hdr_option(struct sk_buff *skb,
 	*(opt++) = (rd_len >> 3);
 	opt += 6;
 
-	memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8);
+	skb_copy_bits(orig_skb, skb_network_offset(orig_skb), opt,
+		      rd_len - 8);
 }
 
 void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target)
-- 
2.14.3

Re: [PATCH net-next] ipv6: fix access to non-linear packet in ndisc_fill_redirect_hdr_option()

[David Miller]

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Fri,  2 Mar 2018 11:53:06 +0100

> Fix the following slab-out-of-bounds kasan report in
> ndisc_fill_redirect_hdr_option when the incoming ipv6 packet is not
> linear and the accessed data are not in the linear data region of orig_skb
 ...
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

As a bug fix this should be targetting 'net' not 'net-next'.

Furthermore, we need an appropriate Fixes: tag so we know when this
problem existed.

If you go far back and it seems like the problem has always been
there, say so and mention how far back you checked.

It also helps to explain exactly how the condition is created
("X creates packet with Y bytes of header space, Z fragments
it at byte N, and that's how we end up here with such a packet")
because such a description aids understanding and might help
suggest alternative (less expensive, cleaner) ways to fix the
problem.

Thanks.

Re: [PATCH net-next] ipv6: fix access to non-linear packet in ndisc_fill_redirect_hdr_option()

[Eric Dumazet]

On Fri, 2018-03-02 at 11:53 +0100, Lorenzo Bianconi wrote:
> Fix the following slab-out-of-bounds kasan report in
> ndisc_fill_redirect_hdr_option when the incoming ipv6 packet is not
> linear and the accessed data are not in the linear data region of orig_skb
> 

> Reported-by: Jianlin Shi <jishi@redhat.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  net/ipv6/ndisc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 0a19ce3a6f7f..afd8c15827cd 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1554,7 +1554,8 @@ static void ndisc_fill_redirect_hdr_option(struct sk_buff *skb,
>  	*(opt++) = (rd_len >> 3);
>  	opt += 6;
>  
> -	memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8);
> +	skb_copy_bits(orig_skb, skb_network_offset(orig_skb), opt,
> +		      rd_len - 8);
>  }

Wow, nice catch !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] ipv6: fix access to non-linear packet in ndisc_fill_redirect_hdr_option()

[Lorenzo Bianconi]

> From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Date: Fri,  2 Mar 2018 11:53:06 +0100
>
>> Fix the following slab-out-of-bounds kasan report in
>> ndisc_fill_redirect_hdr_option when the incoming ipv6 packet is not
>> linear and the accessed data are not in the linear data region of orig_skb
>  ...
>> Reported-by: Jianlin Shi <jishi@redhat.com>
>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>
> As a bug fix this should be targetting 'net' not 'net-next'.
>
> Furthermore, we need an appropriate Fixes: tag so we know when this
> problem existed.
>
> If you go far back and it seems like the problem has always been
> there, say so and mention how far back you checked.

Hi Dave,

Yes sorry, that is exactly the case. Bisecting the issue, I am able to
trigger the kasan report on 4.14 and I have not gone back further.
Moreover the issue is much more hard to trigger on net-next (or net)
respect to 4.14.

>
> It also helps to explain exactly how the condition is created
> ("X creates packet with Y bytes of header space, Z fragments
> it at byte N, and that's how we end up here with such a packet")
> because such a description aids understanding and might help
> suggest alternative (less expensive, cleaner) ways to fix the
> problem.
>
> Thanks.

The test scenario consists of 4 devices (simulated using veth and namespaces):
- H0: data sender, connected to LAN0
- H1: data receiver, connected to LAN1
- GW0 and GW1: routers between LAN0 and LAN1. Both of them have an
ethernet connection on LAN{0,1}
- The default route on H{0,1} is GW0
- using ip command on GW0, set GW1 as next hop for LAN1 from LAN0
- create a ip6ip6 tunnel between H0 and H1
- send 3 concurrent data streams (TCP/UDP/SCTP) from H0 to H1 through
ip6ip6 tunnel (buffer size is set to 16K)
- while data streams are active flush the route cache on HA multiple times.

Regards,
Lorenzo