[PATCH,v3] arm64: fix the illegal address access in some cases

[PATCH,v3] arm64: fix the illegal address access in some cases

[guodeqing]

If do the following test in the arm64 VM, a panic will be produced.
$ ifconfig eth0 up
$ ip netns add ns1
$ ip link add gw link eth0 type ipvlan
$ ip addr add 168.16.0.1/24 dev gw
$ ip link set dev gw up
$ ip link add ip1 link eth0 type ipvlan
$ ip link set ip1 netns ns1
$ ip netns exec ns1 ip link set ip1 up
$ ip netns exec ns1 ip addr add 168.16.0.2/24 dev ip1
$ ip netns exec ns1 ip link set lo up
$ ip netns exec ns1 ip addr add 127.0.0.1/8 dev lo
$ ip netns exec ns1 tc qdisc add dev ip1 root netem corrupt 100%
$ ip netns exec ns1 ping 168.16.0.1

 | Unable to handle kernel paging request at virtual address 
 | Internal error: Oops: 96000007 [#1] SMP
 | CPU: 1 PID: 525 Comm: ping Not tainted 5.8.0-rc6+ #3
 | pstate: 20400005 (nzCv daif +PAN -UAO BTYPE=--)
 | pc : __ip_local_out+0x84/0x188
 | lr : ip_local_out+0x34/0x68 sp : ffff800013263440
 | x29: ffff800013263440 x28: 0000000000000001
 | x27: ffff8000111d2018 x26: ffff8000114cba80
 | x25: ffff0000ec4e7400 x24: 0000000000000000
 | x23: 0000000000000062 x22: ffff8000114c9000
 | x21: ffff0000d97ac600 x20: ffff0000ec519000
 | x19: ffff8000115b5bc0 x18: 0000000000000000
 | x17: 0000000000000000 x16: 0000000000000000
 | x15: 0000000000000000 x14: 0000000000000000
 | x13: 0000000000000000 x12: 0000000000000001
 | x11: ffff800010d21838 x10: 0000000000000001
 | x9 : 0000000000000001 x8 : 0000000000000000
 | x7 : 0000000000000000 x6 : ffff0000ec4e5e00
 | x5 : 024079ca54000184 x4 : ffff0000ec4e5e10
 | x3 : 0000000000000000 x2 : ffff0004ec4e5e20
 | x1 : ffff0000f85f0000 x0 : 031d079626a9c7ae
 | Call trace:
 |  __ip_local_out+0x84/0x188
 |  ip_local_out+0x34/0x68
 |  ipvlan_queue_xmit+0x548/0x5c0
 |  ipvlan_start_xmit+0x2c/0x90
 |  dev_hard_start_xmit+0xb4/0x260
 |  sch_direct_xmit+0x1b4/0x550
 |  __qdisc_run+0x140/0x648
 |  __dev_queue_xmit+0x6a4/0x8b8
 |  dev_queue_xmit+0x24/0x30
 |  ip_finish_output2+0x324/0x580
 |  __ip_finish_output+0x130/0x218
 |  ip_finish_output+0x38/0xd0
 |  ip_output+0xb4/0x130

Here I add the check of the ihl value to fix the problem.

Fixes: 0e455d8e80aa (arm64: Implement optimised IP checksum helpers)
Signed-off-by: guodeqing <geffrey.guo@huawei.com>
---
 arch/arm64/include/asm/checksum.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/include/asm/checksum.h b/arch/arm64/include/asm/checksum.h
index b6f7bc6..702ac89 100644
--- a/arch/arm64/include/asm/checksum.h
+++ b/arch/arm64/include/asm/checksum.h
@@ -25,6 +25,9 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
 	__uint128_t tmp;
 	u64 sum;
 
+	if (WARN_ON_ONCE(ihl < 5))
+		ihl = 5;
+
 	tmp = *(const __uint128_t *)iph;
 	iph += 16;
 	ihl -= 4;
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH,v3] arm64: fix the illegal address access in some cases

[Robin Murphy]

On 2020-07-30 12:24, guodeqing wrote:
> If do the following test in the arm64 VM, a panic will be produced.
> $ ifconfig eth0 up
> $ ip netns add ns1
> $ ip link add gw link eth0 type ipvlan
> $ ip addr add 168.16.0.1/24 dev gw
> $ ip link set dev gw up
> $ ip link add ip1 link eth0 type ipvlan
> $ ip link set ip1 netns ns1
> $ ip netns exec ns1 ip link set ip1 up
> $ ip netns exec ns1 ip addr add 168.16.0.2/24 dev ip1
> $ ip netns exec ns1 ip link set lo up
> $ ip netns exec ns1 ip addr add 127.0.0.1/8 dev lo
> $ ip netns exec ns1 tc qdisc add dev ip1 root netem corrupt 100%
> $ ip netns exec ns1 ping 168.16.0.1
> 
>   | Unable to handle kernel paging request at virtual address
>   | Internal error: Oops: 96000007 [#1] SMP
>   | CPU: 1 PID: 525 Comm: ping Not tainted 5.8.0-rc6+ #3
>   | pstate: 20400005 (nzCv daif +PAN -UAO BTYPE=--)
>   | pc : __ip_local_out+0x84/0x188
>   | lr : ip_local_out+0x34/0x68 sp : ffff800013263440
>   | x29: ffff800013263440 x28: 0000000000000001
>   | x27: ffff8000111d2018 x26: ffff8000114cba80
>   | x25: ffff0000ec4e7400 x24: 0000000000000000
>   | x23: 0000000000000062 x22: ffff8000114c9000
>   | x21: ffff0000d97ac600 x20: ffff0000ec519000
>   | x19: ffff8000115b5bc0 x18: 0000000000000000
>   | x17: 0000000000000000 x16: 0000000000000000
>   | x15: 0000000000000000 x14: 0000000000000000
>   | x13: 0000000000000000 x12: 0000000000000001
>   | x11: ffff800010d21838 x10: 0000000000000001
>   | x9 : 0000000000000001 x8 : 0000000000000000
>   | x7 : 0000000000000000 x6 : ffff0000ec4e5e00
>   | x5 : 024079ca54000184 x4 : ffff0000ec4e5e10
>   | x3 : 0000000000000000 x2 : ffff0004ec4e5e20
>   | x1 : ffff0000f85f0000 x0 : 031d079626a9c7ae
>   | Call trace:
>   |  __ip_local_out+0x84/0x188
>   |  ip_local_out+0x34/0x68
>   |  ipvlan_queue_xmit+0x548/0x5c0
>   |  ipvlan_start_xmit+0x2c/0x90
>   |  dev_hard_start_xmit+0xb4/0x260
>   |  sch_direct_xmit+0x1b4/0x550
>   |  __qdisc_run+0x140/0x648
>   |  __dev_queue_xmit+0x6a4/0x8b8
>   |  dev_queue_xmit+0x24/0x30
>   |  ip_finish_output2+0x324/0x580
>   |  __ip_finish_output+0x130/0x218
>   |  ip_finish_output+0x38/0xd0
>   |  ip_output+0xb4/0x130
> 
> Here I add the check of the ihl value to fix the problem.

Can you confirm that the inline patch I sent also fixes it? I'd prefer 
that as it has no impact for correct cases.

It would also have been useful to explain *why* this case causes a 
crash, since it's not all that obvious - having eventually figured it 
out by inspection, and knowing that compilers like to transform index 
comparisons into pointer comparisons, x4 and x2 in the register dump 
then start to stand out as the smoking gun, but it took me a while to 
get there, since the nature of your fix implies that accessing within 
the assumed bounds of iph itself was a problem, when in fact it isn't.

> Fixes: 0e455d8e80aa (arm64: Implement optimised IP checksum helpers)
> Signed-off-by: guodeqing <geffrey.guo@huawei.com>
> ---
>   arch/arm64/include/asm/checksum.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/checksum.h b/arch/arm64/include/asm/checksum.h
> index b6f7bc6..702ac89 100644
> --- a/arch/arm64/include/asm/checksum.h
> +++ b/arch/arm64/include/asm/checksum.h
> @@ -25,6 +25,9 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
>   	__uint128_t tmp;
>   	u64 sum;
>   
> +	if (WARN_ON_ONCE(ihl < 5))

My reading of the netdev discussion is that if we're expected to be 
robust to this due to possible Rx packet corruption, then WARN probably 
isn't appropriate.

Robin.

> +		ihl = 5;
> +
>   	tmp = *(const __uint128_t *)iph;
>   	iph += 16;
>   	ihl -= 4;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

答复: [PATCH,v3] arm64: fix the illegal address access in some cases

[Guodeqing (A)]

> -----邮件原件-----
> 发件人: Robin Murphy [mailto:robin.murphy@arm.com]
> 发送时间: Thursday, July 30, 2020 20:02
> 收件人: Guodeqing (A) <geffrey.guo@huawei.com>;
> catalin.marinas@arm.com
> 抄送: will@kernel.org; luke.starrett@broadcom.com;
> linux-arm-kernel@lists.infradead.org
> 主题: Re: [PATCH,v3] arm64: fix the illegal address access in some cases
> 
> On 2020-07-30 12:24, guodeqing wrote:
> > If do the following test in the arm64 VM, a panic will be produced.
> > $ ifconfig eth0 up
> > $ ip netns add ns1
> > $ ip link add gw link eth0 type ipvlan $ ip addr add 168.16.0.1/24 dev
> > gw $ ip link set dev gw up $ ip link add ip1 link eth0 type ipvlan $
> > ip link set ip1 netns ns1 $ ip netns exec ns1 ip link set ip1 up $ ip
> > netns exec ns1 ip addr add 168.16.0.2/24 dev ip1 $ ip netns exec ns1
> > ip link set lo up $ ip netns exec ns1 ip addr add 127.0.0.1/8 dev lo $
> > ip netns exec ns1 tc qdisc add dev ip1 root netem corrupt 100% $ ip
> > netns exec ns1 ping 168.16.0.1
> >
> >   | Unable to handle kernel paging request at virtual address
> >   | Internal error: Oops: 96000007 [#1] SMP
> >   | CPU: 1 PID: 525 Comm: ping Not tainted 5.8.0-rc6+ #3
> >   | pstate: 20400005 (nzCv daif +PAN -UAO BTYPE=--)
> >   | pc : __ip_local_out+0x84/0x188
> >   | lr : ip_local_out+0x34/0x68 sp : ffff800013263440
> >   | x29: ffff800013263440 x28: 0000000000000001
> >   | x27: ffff8000111d2018 x26: ffff8000114cba80
> >   | x25: ffff0000ec4e7400 x24: 0000000000000000
> >   | x23: 0000000000000062 x22: ffff8000114c9000
> >   | x21: ffff0000d97ac600 x20: ffff0000ec519000
> >   | x19: ffff8000115b5bc0 x18: 0000000000000000
> >   | x17: 0000000000000000 x16: 0000000000000000
> >   | x15: 0000000000000000 x14: 0000000000000000
> >   | x13: 0000000000000000 x12: 0000000000000001
> >   | x11: ffff800010d21838 x10: 0000000000000001
> >   | x9 : 0000000000000001 x8 : 0000000000000000
> >   | x7 : 0000000000000000 x6 : ffff0000ec4e5e00
> >   | x5 : 024079ca54000184 x4 : ffff0000ec4e5e10
> >   | x3 : 0000000000000000 x2 : ffff0004ec4e5e20
> >   | x1 : ffff0000f85f0000 x0 : 031d079626a9c7ae
> >   | Call trace:
> >   |  __ip_local_out+0x84/0x188
> >   |  ip_local_out+0x34/0x68
> >   |  ipvlan_queue_xmit+0x548/0x5c0
> >   |  ipvlan_start_xmit+0x2c/0x90
> >   |  dev_hard_start_xmit+0xb4/0x260
> >   |  sch_direct_xmit+0x1b4/0x550
> >   |  __qdisc_run+0x140/0x648
> >   |  __dev_queue_xmit+0x6a4/0x8b8
> >   |  dev_queue_xmit+0x24/0x30
> >   |  ip_finish_output2+0x324/0x580
> >   |  __ip_finish_output+0x130/0x218
> >   |  ip_finish_output+0x38/0xd0
> >   |  ip_output+0xb4/0x130
> >
> > Here I add the check of the ihl value to fix the problem.
> 
> Can you confirm that the inline patch I sent also fixes it? I'd prefer that as it has
> no impact for correct cases.

I do a test, your patch aslo solve the problem. your patch is better which has no condition jugement.

> 
> It would also have been useful to explain *why* this case causes a crash, since
> it's not all that obvious - having eventually figured it out by inspection, and
> knowing that compilers like to transform index comparisons into pointer
> comparisons, x4 and x2 in the register dump then start to stand out as the
> smoking gun, but it took me a while to get there, since the nature of your fix
> implies that accessing within the assumed bounds of iph itself was a problem,
> when in fact it isn't.
> 
> > Fixes: 0e455d8e80aa (arm64: Implement optimised IP checksum helpers)
> > Signed-off-by: guodeqing <geffrey.guo@huawei.com>
> > ---
> >   arch/arm64/include/asm/checksum.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/checksum.h
> > b/arch/arm64/include/asm/checksum.h
> > index b6f7bc6..702ac89 100644
> > --- a/arch/arm64/include/asm/checksum.h
> > +++ b/arch/arm64/include/asm/checksum.h
> > @@ -25,6 +25,9 @@ static inline __sum16 ip_fast_csum(const void *iph,
> unsigned int ihl)
> >   	__uint128_t tmp;
> >   	u64 sum;
> >
> > +	if (WARN_ON_ONCE(ihl < 5))
> 
> My reading of the netdev discussion is that if we're expected to be robust to
> this due to possible Rx packet corruption, then WARN probably isn't
> appropriate.
> 
> Robin.
> 
> > +		ihl = 5;
> > +
> >   	tmp = *(const __uint128_t *)iph;
> >   	iph += 16;
> >   	ihl -= 4;
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel