* [PATCH,v2] arm64: fix the illegal address access in some cases @ 2020-07-25 2:08 guodeqing 2020-07-27 11:47 ` Catalin Marinas 2020-07-28 13:03 ` Will Deacon 0 siblings, 2 replies; 13+ messages in thread From: guodeqing @ 2020-07-25 2:08 UTC (permalink / raw) To: catalin.marinas Cc: robin.murphy, luke.starrett, will, linux-arm-kernel, geffrey.guo The ihl value of ip header is smaller than 5 in some cases, if the ihl value is smaller than 5, then the next code will access the illegal address, and the system will panic. ip_fast_csum() must be able to handle any value that could fit in the ihl field of the ip protocol header. Here I add the check of the ihl value to solve this 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..5a7d9ac 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 (unlikely(ihl < 5)) + return 1; + 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-25 2:08 [PATCH,v2] arm64: fix the illegal address access in some cases guodeqing @ 2020-07-27 11:47 ` Catalin Marinas 2020-07-27 13:29 ` 答复: " Guodeqing (A) 2020-07-28 13:03 ` Will Deacon 1 sibling, 1 reply; 13+ messages in thread From: Catalin Marinas @ 2020-07-27 11:47 UTC (permalink / raw) To: guodeqing; +Cc: will, luke.starrett, robin.murphy, linux-arm-kernel On Sat, Jul 25, 2020 at 10:08:06AM +0800, guodeqing wrote: > The ihl value of ip header is smaller than 5 in some cases, if the > ihl value is smaller than 5, then the next code will access the illegal > address, and the system will panic. ip_fast_csum() must be able to handle > any value that could fit in the ihl field of the ip protocol header. > > Here I add the check of the ihl value to solve this 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..5a7d9ac 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 (unlikely(ihl < 5)) > + return 1; > + > tmp = *(const __uint128_t *)iph; > iph += 16; > ihl -= 4; IHL in IPv4 should be at least 5. Do you have a stack trace to show how it got here? Maybe the caller should ensure that the correct size is passed. Thanks. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-27 11:47 ` Catalin Marinas @ 2020-07-27 13:29 ` Guodeqing (A) 2020-07-28 9:53 ` Catalin Marinas 0 siblings, 1 reply; 13+ messages in thread From: Guodeqing (A) @ 2020-07-27 13:29 UTC (permalink / raw) To: Catalin Marinas Cc: will, luke.starrett, robin.murphy, Eric Dumazet, linux-arm-kernel > -----邮件原件----- > 发件人: Catalin Marinas [mailto:catalin.marinas@arm.com] > 发送时间: Monday, July 27, 2020 19:47 > 收件人: Guodeqing (A) <geffrey.guo@huawei.com> > 抄送: robin.murphy@arm.com; luke.starrett@broadcom.com; will@kernel.org; > linux-arm-kernel@lists.infradead.org > 主题: Re: [PATCH,v2] arm64: fix the illegal address access in some cases > > On Sat, Jul 25, 2020 at 10:08:06AM +0800, guodeqing wrote: > > The ihl value of ip header is smaller than 5 in some cases, if the ihl > > value is smaller than 5, then the next code will access the illegal > > address, and the system will panic. ip_fast_csum() must be able to > > handle any value that could fit in the ihl field of the ip protocol header. > > > > Here I add the check of the ihl value to solve this 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..5a7d9ac 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 (unlikely(ihl < 5)) > > + return 1; > > + > > tmp = *(const __uint128_t *)iph; > > iph += 16; > > ihl -= 4; > > IHL in IPv4 should be at least 5. Do you have a stack trace to show how it got > here? Maybe the caller should ensure that the correct size is passed. > If do the following test,this will cause a panic in a arm64 VM. this can be reproduced easily. ~# 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 PING 168.16.0.1 [ 582.368938] Unable to handle kernel paging request at virtual address ffff0000f85f0000 [ 582.369732] Mem abort info: [ 582.369987] ESR = 0x96000007 [ 582.370266] EC = 0x25: DABT (current EL), IL = 32 bits [ 582.370833] SET = 0, FnV = 0 [ 582.371113] EA = 0, S1PTW = 0 [ 582.371391] Data abort info: [ 582.371671] ISV = 0, ISS = 0x00000007 [ 582.372017] CM = 0, WnR = 0 [ 582.372299] swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000012dab7000 [ 582.372896] [ffff0000f85f0000] pgd=000000013fff8003, p4d=000000013fff8003, pud=000000013f9f4003, pmd=000000013f838003, pte=0000000000000000 [ 582.374033] Internal error: Oops: 96000007 [#1] SMP [ 582.374468] Modules linked in: [ 582.374795] CPU: 1 PID: 525 Comm: ping Not tainted 5.8.0-rc6+ #3 [ 582.375468] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015 [ 582.376215] pstate: 20400005 (nzCv daif +PAN -UAO BTYPE=--) [ 582.376805] pc : __ip_local_out+0x84/0x188 [ 582.377234] lr : ip_local_out+0x34/0x68 [ 582.377635] sp : ffff800013263440 [ 582.377986] x29: ffff800013263440 x28: 0000000000000001 [ 582.378536] x27: ffff8000111d2018 x26: ffff8000114cba80 [ 582.379093] x25: ffff0000ec4e7400 x24: 0000000000000000 [ 582.379653] x23: 0000000000000062 x22: ffff8000114c9000 [ 582.380221] x21: ffff0000d97ac600 x20: ffff0000ec519000 [ 582.380778] x19: ffff8000115b5bc0 x18: 0000000000000000 [ 582.381324] x17: 0000000000000000 x16: 0000000000000000 [ 582.381876] x15: 0000000000000000 x14: 0000000000000000 [ 582.382431] x13: 0000000000000000 x12: 0000000000000001 [ 582.382986] x11: ffff800010d21838 x10: 0000000000000001 [ 582.383567] x9 : 0000000000000001 x8 : 0000000000000000 [ 582.384136] x7 : 0000000000000000 x6 : ffff0000ec4e5e00 [ 582.384693] x5 : 024079ca54000184 x4 : ffff0000ec4e5e10 [ 582.385246] x3 : 0000000000000000 x2 : ffff0004ec4e5e20 [ 582.385808] x1 : ffff0000f85f0000 x0 : 031d079626a9c7ae [ 582.386365] Call trace: [ 582.386629] __ip_local_out+0x84/0x188 [ 582.387030] ip_local_out+0x34/0x68 [ 582.387400] ipvlan_queue_xmit+0x548/0x5c0 [ 582.387845] ipvlan_start_xmit+0x2c/0x90 [ 582.388283] dev_hard_start_xmit+0xb4/0x260 [ 582.388732] sch_direct_xmit+0x1b4/0x550 [ 582.389145] __qdisc_run+0x140/0x648 [ 582.389524] __dev_queue_xmit+0x6a4/0x8b8 [ 582.389948] dev_queue_xmit+0x24/0x30 [ 582.390339] ip_finish_output2+0x324/0x580 [ 582.390770] __ip_finish_output+0x130/0x218 [ 582.391218] ip_finish_output+0x38/0xd0 [ 582.391633] ip_output+0xb4/0x130 [ 582.391984] ip_local_out+0x58/0x68 [ 582.392369] ip_send_skb+0x2c/0x88 [ 582.392729] ip_push_pending_frames+0x44/0x50 [ 582.393189] raw_sendmsg+0x7a4/0x988 [ 582.393569] inet_sendmsg+0x4c/0x78 [ 582.393942] sock_sendmsg+0x58/0x68 [ 582.394311] ____sys_sendmsg+0x284/0x2c0 [ 582.394721] ___sys_sendmsg+0x90/0xd0 [ 582.395113] __sys_sendmsg+0x78/0xd0 [ 582.395504] __arm64_sys_sendmsg+0x2c/0x38 [ 582.395942] el0_svc_common.constprop.2+0x70/0x128 [ 582.396472] do_el0_svc+0x34/0xa0 [ 582.396834] el0_sync_handler+0xec/0x128 [ 582.397249] el0_sync+0x140/0x180 [ 582.397611] Code: ab030005 91001442 9a030000 8b020882 (b8404423) [ 582.398264] ---[ end trace 92adb54c8611f8c5 ]--- [ 582.398754] Kernel panic - not syncing: Fatal exception in interrupt [ 582.399481] SMP: stopping secondary CPUs [ 582.399923] Kernel Offset: 0xc0000 from 0xffff800010000000 [ 582.400561] PHYS_OFFSET: 0x40000000 [ 582.400939] CPU features: 0x040002,22a08238 [ 582.401380] Memory Limit: none [ 582.401710] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]--- Do the same test in the x86_64 VM will not cause a panic. this is because ip_fast_csum in the x86_64 architecture has the check of the ihl value. static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) { unsigned int sum; asm(" movl (%1), %0\n" " subl $4, %2\n" " jbe 2f\n" --- here " addl 4(%1), %0\n" " adcl 8(%1), %0\n" " adcl 12(%1), %0\n" "1: adcl 16(%1), %0\n" " lea 4(%1), %1\n" " decl %2\n" " jne 1b\n" " adcl $0, %0\n" " movl %0, %2\n" " shrl $16, %0\n" " addw %w2, %w0\n" " adcl $0, %0\n" " notl %0\n" "2:" /* Since the input registers which are loaded with iph and ihl are modified, we must also specify them as outputs, or gcc will assume they contain their original values. */ : "=r" (sum), "=r" (iph), "=r" (ihl) : "1" (iph), "2" (ihl) : "memory"); return (__force __sum16)sum; } Thanks. > Thanks. > > -- > Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-27 13:29 ` 答复: " Guodeqing (A) @ 2020-07-28 9:53 ` Catalin Marinas 0 siblings, 0 replies; 13+ messages in thread From: Catalin Marinas @ 2020-07-28 9:53 UTC (permalink / raw) To: Guodeqing (A) Cc: will, luke.starrett, robin.murphy, Eric Dumazet, linux-arm-kernel On Mon, Jul 27, 2020 at 01:29:36PM +0000, Guodeqing (A) wrote: > > On Sat, Jul 25, 2020 at 10:08:06AM +0800, guodeqing wrote: > > > The ihl value of ip header is smaller than 5 in some cases, if the ihl > > > value is smaller than 5, then the next code will access the illegal > > > address, and the system will panic. ip_fast_csum() must be able to > > > handle any value that could fit in the ihl field of the ip protocol header. > > > > > > Here I add the check of the ihl value to solve this 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..5a7d9ac 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 (unlikely(ihl < 5)) > > > + return 1; > > > + > > > tmp = *(const __uint128_t *)iph; > > > iph += 16; > > > ihl -= 4; > > > > IHL in IPv4 should be at least 5. Do you have a stack trace to show how it got > > here? Maybe the caller should ensure that the correct size is passed. > > If do the following test,this will cause a panic in a arm64 VM. this > can be reproduced easily. > > ~# 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 PING 168.16.0.1 Thanks for the explanation. The fix looks fine to me. Acked-by: Catalin Marinas <catalin.marinas@arm.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-25 2:08 [PATCH,v2] arm64: fix the illegal address access in some cases guodeqing 2020-07-27 11:47 ` Catalin Marinas @ 2020-07-28 13:03 ` Will Deacon 2020-07-28 14:30 ` Robin Murphy 1 sibling, 1 reply; 13+ messages in thread From: Will Deacon @ 2020-07-28 13:03 UTC (permalink / raw) To: catalin.marinas, guodeqing Cc: Will Deacon, luke.starrett, kernel-team, robin.murphy, linux-arm-kernel On Sat, 25 Jul 2020 10:08:06 +0800, guodeqing wrote: > The ihl value of ip header is smaller than 5 in some cases, if the > ihl value is smaller than 5, then the next code will access the illegal > address, and the system will panic. ip_fast_csum() must be able to handle > any value that could fit in the ihl field of the ip protocol header. > > Here I add the check of the ihl value to solve this problem. Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five https://git.kernel.org/arm64/c/09aaef1c5f50 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-28 13:03 ` Will Deacon @ 2020-07-28 14:30 ` Robin Murphy 2020-07-28 15:35 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Robin Murphy @ 2020-07-28 14:30 UTC (permalink / raw) To: Will Deacon, catalin.marinas, guodeqing Cc: luke.starrett, kernel-team, linux-arm-kernel Hi Will, On 2020-07-28 14:03, Will Deacon wrote: > On Sat, 25 Jul 2020 10:08:06 +0800, guodeqing wrote: >> The ihl value of ip header is smaller than 5 in some cases, if the >> ihl value is smaller than 5, then the next code will access the illegal >> address, and the system will panic. ip_fast_csum() must be able to handle >> any value that could fit in the ihl field of the ip protocol header. >> >> Here I add the check of the ihl value to solve this problem. > > Applied to arm64 (for-next/fixes), thanks! > > [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > https://git.kernel.org/arm64/c/09aaef1c5f50 I'm not sure your commit message is entirely right there. AFAICS it's not "the same way as x86" at all - x86 dereferences the first word of iph and returns that as the sum if ihl <= 4 (and thus is still capable of crashing given sufficiently bogus data). I'm not sure where "return 1" came from - if we're going to return nonsense then the mildly more efficient choice of 0 seems just as good. Otherwise it would seem reasonable to jump straight into the word-at-a-time loop if ip_fast_csum() is really expected to cope with more than just genuine IP headers (which should be backed by at least 20 bytes of valid memory regardless of what ihl says). I still think this smells of papering over some other bug that led to a bogus skb getting that far into the transmit stack in the first place - presumably it's all wasted effort anyway since a "header" with no space for a destination address and a deliberately wrong checksum seems unlikely to go very far... On a quick look there appear to be quite a few implementations dereferencing up to 5 words unconditionally, so it's not like this is arm64's own bug. Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-28 14:30 ` Robin Murphy @ 2020-07-28 15:35 ` Will Deacon 2020-07-29 7:05 ` 答复: " Guodeqing (A) 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2020-07-28 15:35 UTC (permalink / raw) To: Robin Murphy Cc: catalin.marinas, luke.starrett, kernel-team, linux-arm-kernel, guodeqing Hi Robin, On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: > On 2020-07-28 14:03, Will Deacon wrote: > > On Sat, 25 Jul 2020 10:08:06 +0800, guodeqing wrote: > > > The ihl value of ip header is smaller than 5 in some cases, if the > > > ihl value is smaller than 5, then the next code will access the illegal > > > address, and the system will panic. ip_fast_csum() must be able to handle > > > any value that could fit in the ihl field of the ip protocol header. > > > > > > Here I add the check of the ihl value to solve this problem. > > > > Applied to arm64 (for-next/fixes), thanks! > > > > [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > > https://git.kernel.org/arm64/c/09aaef1c5f50 > > I'm not sure your commit message is entirely right there. AFAICS it's not > "the same way as x86" at all - x86 dereferences the first word of iph and > returns that as the sum if ihl <= 4 (and thus is still capable of crashing > given sufficiently bogus data). I'm not sure where "return 1" came from - if > we're going to return nonsense then the mildly more efficient choice of 0 > seems just as good. Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. Geffrey? > Otherwise it would seem reasonable to jump straight into > the word-at-a-time loop if ip_fast_csum() is really expected to cope with > more than just genuine IP headers (which should be backed by at least 20 > bytes of valid memory regardless of what ihl says). Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and ihl of 5 would be my preference, because I agree with you that this feels like it shouldn't be happening to start with. > I still think this smells of papering over some other bug that led to a > bogus skb getting that far into the transmit stack in the first place - > presumably it's all wasted effort anyway since a "header" with no space for > a destination address and a deliberately wrong checksum seems unlikely to go > very far... Looking at the ipvlan_start_xmit() path from the backtrace, it looks to me like ipvlan_get_L3_hdr() returns NULL if the header length is invalid, but then ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. Hmm. I really don't know enough about VLANs to know what the right behaviour is here and I guess just returning NET_XMIT_DROP will break something. > On a quick look there appear to be quite a few implementations dereferencing > up to 5 words unconditionally, so it's not like this is arm64's own bug. I'll drop the patch, but we are apparently open to a crash here, so if you have time to figure out what's going on, that would be great. The reproducer didn't work for me (I guess I'm missing some utils) and sending bogus header lengths with a raw socket worked fine (i.e. didn't crash either). I guess the vlan is an important piece of the picture. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-28 15:35 ` Will Deacon @ 2020-07-29 7:05 ` Guodeqing (A) 2020-07-30 8:44 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Guodeqing (A) @ 2020-07-29 7:05 UTC (permalink / raw) To: Will Deacon, Robin Murphy Cc: catalin.marinas, luke.starrett, kernel-team, linux-arm-kernel > -----邮件原件----- > 发件人: Will Deacon [mailto:will@kernel.org] > 发送时间: Tuesday, July 28, 2020 23:35 > 收件人: Robin Murphy <robin.murphy@arm.com> > 抄送: catalin.marinas@arm.com; Guodeqing (A) <geffrey.guo@huawei.com>; > kernel-team@android.com; linux-arm-kernel@lists.infradead.org; > luke.starrett@broadcom.com > 主题: Re: [PATCH,v2] arm64: fix the illegal address access in some cases > > Hi Robin, > > On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: > > On 2020-07-28 14:03, Will Deacon wrote: > > > On Sat, 25 Jul 2020 10:08:06 +0800, guodeqing wrote: > > > > The ihl value of ip header is smaller than 5 in some cases, if the > > > > ihl value is smaller than 5, then the next code will access the > > > > illegal address, and the system will panic. ip_fast_csum() must be > > > > able to handle any value that could fit in the ihl field of the ip protocol > header. > > > > > > > > Here I add the check of the ihl value to solve this problem. > > > > > > Applied to arm64 (for-next/fixes), thanks! > > > > > > [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > > > https://git.kernel.org/arm64/c/09aaef1c5f50 > > > > I'm not sure your commit message is entirely right there. AFAICS it's > > not "the same way as x86" at all - x86 dereferences the first word of > > iph and returns that as the sum if ihl <= 4 (and thus is still capable > > of crashing given sufficiently bogus data). I'm not sure where "return > > 1" came from - if we're going to return nonsense then the mildly more > > efficient choice of 0 seems just as good. > > Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. > Geffrey? > The return 1 is just the report of ip checksum error, the return value 0 means the ip checksum correct. x86 dereferences the first word of iph and returns that as the sum, this may be just the report of ip checksum error too. > > Otherwise it would seem reasonable to jump straight into the > > word-at-a-time loop if ip_fast_csum() is really expected to cope with > > more than just genuine IP headers (which should be backed by at least > > 20 bytes of valid memory regardless of what ihl says). > > Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and ihl of > 5 would be my preference, because I agree with you that this feels like it > shouldn't be happening to start with. How about modify the patch like this? static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) { __uint128_t tmp; u64 sum; if (unlikely(ihl < 5)) ihl = 5; tmp = *(const __uint128_t *)iph; iph += 16; ihl -= 4; tmp += ((tmp >> 64) | (tmp << 64)); sum = tmp >> 64; do { sum += *(const u32 *)iph; iph += 4; } while (--ihl); sum += ((sum >> 32) | (sum << 32)); return csum_fold((__force u32)(sum >> 32)); } > > > I still think this smells of papering over some other bug that led to > > a bogus skb getting that far into the transmit stack in the first > > place - presumably it's all wasted effort anyway since a "header" with > > no space for a destination address and a deliberately wrong checksum > > seems unlikely to go very far... > > Looking at the ipvlan_start_xmit() path from the backtrace, it looks to me like > ipvlan_get_L3_hdr() returns NULL if the header length is invalid, but then > ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. > Hmm. I really don't know enough about VLANs to know what the right > behaviour is here and I guess just returning NET_XMIT_DROP will break > something. The network maintainer has replied to me, " ip_fast_csum() must be able to handle any value that could fit in the ihl field of the ip protocol header. That's not only the most correct logic, but also the most robust." > > > On a quick look there appear to be quite a few implementations > > dereferencing up to 5 words unconditionally, so it's not like this is arm64's > own bug. > > I'll drop the patch, but we are apparently open to a crash here, so if you have > time to figure out what's going on, that would be great. The reproducer didn't > work for me (I guess I'm missing some utils) and sending bogus header lengths > with a raw socket worked fine (i.e. didn't crash either). I guess the vlan is an > important piece of the picture. This test needs the netem module and " ip netns exec ns1 tc qdisc add dev ip1 root netem corrupt 100%" must execute successfully. then the panic can be reproduced. This is a fault injection test, the corrupt function of netem is the emulation of random noise introducing an error in a random position for a chosen percent of packets to test the network module.the netem will modify the packet randomly,so the ihl value of ip header may be modified to 1. Thanks. > > Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-29 7:05 ` 答复: " Guodeqing (A) @ 2020-07-30 8:44 ` Will Deacon 2020-07-30 9:56 ` Robin Murphy 2020-07-30 10:49 ` Guodeqing (A) 0 siblings, 2 replies; 13+ messages in thread From: Will Deacon @ 2020-07-30 8:44 UTC (permalink / raw) To: Guodeqing (A) Cc: catalin.marinas, luke.starrett, kernel-team, Robin Murphy, linux-arm-kernel On Wed, Jul 29, 2020 at 07:05:09AM +0000, Guodeqing (A) wrote: > > On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: > > > On 2020-07-28 14:03, Will Deacon wrote: > > > > Applied to arm64 (for-next/fixes), thanks! > > > > > > > > [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > > > > https://git.kernel.org/arm64/c/09aaef1c5f50 > > > > > > I'm not sure your commit message is entirely right there. AFAICS it's > > > not "the same way as x86" at all - x86 dereferences the first word of > > > iph and returns that as the sum if ihl <= 4 (and thus is still capable > > > of crashing given sufficiently bogus data). I'm not sure where "return > > > 1" came from - if we're going to return nonsense then the mildly more > > > efficient choice of 0 seems just as good. > > > > Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. > > Geffrey? > > > > The return 1 is just the report of ip checksum error, the return value 0 means the ip > checksum correct. x86 dereferences the first word of iph and returns that as the sum, > this may be just the report of ip checksum error too. On the receive path, sure, but the crash was on the transmit path where we're computing the checksum to insert into the header, no? > > > Otherwise it would seem reasonable to jump straight into the > > > word-at-a-time loop if ip_fast_csum() is really expected to cope with > > > more than just genuine IP headers (which should be backed by at least > > > 20 bytes of valid memory regardless of what ihl says). > > > > Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and ihl of > > 5 would be my preference, because I agree with you that this feels like it > > shouldn't be happening to start with. > > How about modify the patch like this? > > static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > { > __uint128_t tmp; > u64 sum; > > if (unlikely(ihl < 5)) > ihl = 5; I'd probably do: /* Callers should really be checking this first */ if (WARN_ON_ONCE(ihl < 5)) ihl = 5; because I'd still like to understand what the vlan code is up to. > > > I still think this smells of papering over some other bug that led to > > > a bogus skb getting that far into the transmit stack in the first > > > place - presumably it's all wasted effort anyway since a "header" with > > > no space for a destination address and a deliberately wrong checksum > > > seems unlikely to go very far... > > > > Looking at the ipvlan_start_xmit() path from the backtrace, it looks to me like > > ipvlan_get_L3_hdr() returns NULL if the header length is invalid, but then > > ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. > > Hmm. I really don't know enough about VLANs to know what the right > > behaviour is here and I guess just returning NET_XMIT_DROP will break > > something. > > The network maintainer has replied to me, > " ip_fast_csum() must be able to handle any value that could fit in the > ihl field of the ip protocol header. That's not only the most correct > logic, but also the most robust." Is that on a public list somewhere? Would be a good link for the commit message. > This is a fault injection test, the corrupt function of netem is the > emulation of random noise introducing an error in a random position for a > chosen percent of packets to test the network module.the netem will modify > the packet randomly,so the ihl value of ip header may be modified to 1. Ok, but netem is running in userspace (right?) and so I still think the network layer can reject the invalid ihl before calling into the checksum code. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-30 8:44 ` Will Deacon @ 2020-07-30 9:56 ` Robin Murphy 2020-07-30 16:03 ` Will Deacon 2020-07-31 3:04 ` 答复: " Guodeqing (A) 2020-07-30 10:49 ` Guodeqing (A) 1 sibling, 2 replies; 13+ messages in thread From: Robin Murphy @ 2020-07-30 9:56 UTC (permalink / raw) To: Will Deacon, Guodeqing (A) Cc: catalin.marinas, luke.starrett, kernel-team, linux-arm-kernel On 2020-07-30 09:44, Will Deacon wrote: > On Wed, Jul 29, 2020 at 07:05:09AM +0000, Guodeqing (A) wrote: >>> On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: >>>> On 2020-07-28 14:03, Will Deacon wrote: >>>>> Applied to arm64 (for-next/fixes), thanks! >>>>> >>>>> [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five >>>>> https://git.kernel.org/arm64/c/09aaef1c5f50 >>>> >>>> I'm not sure your commit message is entirely right there. AFAICS it's >>>> not "the same way as x86" at all - x86 dereferences the first word of >>>> iph and returns that as the sum if ihl <= 4 (and thus is still capable >>>> of crashing given sufficiently bogus data). I'm not sure where "return >>>> 1" came from - if we're going to return nonsense then the mildly more >>>> efficient choice of 0 seems just as good. >>> >>> Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. >>> Geffrey? >>> >> >> The return 1 is just the report of ip checksum error, the return value 0 means the ip >> checksum correct. x86 dereferences the first word of iph and returns that as the sum, >> this may be just the report of ip checksum error too. > > On the receive path, sure, but the crash was on the transmit path where > we're computing the checksum to insert into the header, no? > >>>> Otherwise it would seem reasonable to jump straight into the >>>> word-at-a-time loop if ip_fast_csum() is really expected to cope with >>>> more than just genuine IP headers (which should be backed by at least >>>> 20 bytes of valid memory regardless of what ihl says). >>> >>> Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and ihl of >>> 5 would be my preference, because I agree with you that this feels like it >>> shouldn't be happening to start with. >> >> How about modify the patch like this? >> >> static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) >> { >> __uint128_t tmp; >> u64 sum; >> >> if (unlikely(ihl < 5)) >> ihl = 5; > > I'd probably do: > > /* Callers should really be checking this first */ > if (WARN_ON_ONCE(ihl < 5)) > ihl = 5; > > because I'd still like to understand what the vlan code is up to. > >>>> I still think this smells of papering over some other bug that led to >>>> a bogus skb getting that far into the transmit stack in the first >>>> place - presumably it's all wasted effort anyway since a "header" with >>>> no space for a destination address and a deliberately wrong checksum >>>> seems unlikely to go very far... >>> >>> Looking at the ipvlan_start_xmit() path from the backtrace, it looks to me like >>> ipvlan_get_L3_hdr() returns NULL if the header length is invalid, but then >>> ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. >>> Hmm. I really don't know enough about VLANs to know what the right >>> behaviour is here and I guess just returning NET_XMIT_DROP will break >>> something. >> >> The network maintainer has replied to me, >> " ip_fast_csum() must be able to handle any value that could fit in the >> ihl field of the ip protocol header. That's not only the most correct >> logic, but also the most robust." > > Is that on a public list somewhere? Would be a good link for the commit > message. > >> This is a fault injection test, the corrupt function of netem is the >> emulation of random noise introducing an error in a random position for a >> chosen percent of packets to test the network module.the netem will modify >> the packet randomly,so the ihl value of ip header may be modified to 1. > > Ok, but netem is running in userspace (right?) and so I still think the > network layer can reject the invalid ihl before calling into the checksum > code. Oh, on second look I realise it's probably not that the fault emanates from dereferencing the actual header itself, it'll be from the code just going completely bonkers *after* that. I still agree that this case should be avoidable entirely on the transmit path, but I accept that robustness for the sake of receive does make good sense. How about this? Robin. ----->8----- Subject: [PATCH] arm64: csum: Fix handling of bad packets Although iph is expected to point to at least 20 bytes of valid memory, ihl may be bogus, for example on reception of a corrupt packet. If it happens to be less than 5, we really don't want to run away and dereference 16GB worth of memory until it wraps back to exactly zero... Fixes: 0e455d8e80aa ("arm64: Implement optimised IP checksum helpers") Reported-by: guodeqing <geffrey.guo@huawei.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- arch/arm64/include/asm/checksum.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/checksum.h b/arch/arm64/include/asm/checksum.h index b6f7bc6da5fb..93a161b3bf3f 100644 --- a/arch/arm64/include/asm/checksum.h +++ b/arch/arm64/include/asm/checksum.h @@ -24,16 +24,17 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) { __uint128_t tmp; u64 sum; + int n = ihl; /* we want it signed */ tmp = *(const __uint128_t *)iph; iph += 16; - ihl -= 4; + n -= 4; tmp += ((tmp >> 64) | (tmp << 64)); sum = tmp >> 64; do { sum += *(const u32 *)iph; iph += 4; - } while (--ihl); + } while (--n > 0); sum += ((sum >> 32) | (sum << 32)); return csum_fold((__force u32)(sum >> 32)); -- 2.28.0.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-30 9:56 ` Robin Murphy @ 2020-07-30 16:03 ` Will Deacon 2020-07-31 3:04 ` 答复: " Guodeqing (A) 1 sibling, 0 replies; 13+ messages in thread From: Will Deacon @ 2020-07-30 16:03 UTC (permalink / raw) To: Robin Murphy Cc: catalin.marinas, luke.starrett, kernel-team, linux-arm-kernel, Guodeqing (A) On Thu, Jul 30, 2020 at 10:56:49AM +0100, Robin Murphy wrote: > Subject: [PATCH] arm64: csum: Fix handling of bad packets > > Although iph is expected to point to at least 20 bytes of valid memory, > ihl may be bogus, for example on reception of a corrupt packet. If it > happens to be less than 5, we really don't want to run away and > dereference 16GB worth of memory until it wraps back to exactly zero... > > Fixes: 0e455d8e80aa ("arm64: Implement optimised IP checksum helpers") > Reported-by: guodeqing <geffrey.guo@huawei.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > arch/arm64/include/asm/checksum.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Cheers, Robin, I'll queue this one shortly (b4 can't cope with this thread at all, so treat this as the automated email :) Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* 答复: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-30 9:56 ` Robin Murphy 2020-07-30 16:03 ` Will Deacon @ 2020-07-31 3:04 ` Guodeqing (A) 1 sibling, 0 replies; 13+ messages in thread From: Guodeqing (A) @ 2020-07-31 3:04 UTC (permalink / raw) To: Robin Murphy, Will Deacon Cc: catalin.marinas, luke.starrett, kernel-team, linux-arm-kernel > -----邮件原件----- > 发件人: Robin Murphy [mailto:robin.murphy@arm.com] > 发送时间: Thursday, July 30, 2020 17:57 > 收件人: Will Deacon <will@kernel.org>; Guodeqing (A) > <geffrey.guo@huawei.com> > 抄送: catalin.marinas@arm.com; kernel-team@android.com; > linux-arm-kernel@lists.infradead.org; luke.starrett@broadcom.com > 主题: Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases > > On 2020-07-30 09:44, Will Deacon wrote: > > On Wed, Jul 29, 2020 at 07:05:09AM +0000, Guodeqing (A) wrote: > >>> On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: > >>>> On 2020-07-28 14:03, Will Deacon wrote: > >>>>> Applied to arm64 (for-next/fixes), thanks! > >>>>> > >>>>> [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > >>>>> https://git.kernel.org/arm64/c/09aaef1c5f50 > >>>> > >>>> I'm not sure your commit message is entirely right there. AFAICS > >>>> it's not "the same way as x86" at all - x86 dereferences the first > >>>> word of iph and returns that as the sum if ihl <= 4 (and thus is > >>>> still capable of crashing given sufficiently bogus data). I'm not > >>>> sure where "return 1" came from - if we're going to return nonsense > >>>> then the mildly more efficient choice of 0 seems just as good. > >>> > >>> Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. > >>> Geffrey? > >>> > >> > >> The return 1 is just the report of ip checksum error, the return > >> value 0 means the ip checksum correct. x86 dereferences the first > >> word of iph and returns that as the sum, this may be just the report of ip > checksum error too. > > > > On the receive path, sure, but the crash was on the transmit path > > where we're computing the checksum to insert into the header, no? > > > >>>> Otherwise it would seem reasonable to jump straight into the > >>>> word-at-a-time loop if ip_fast_csum() is really expected to cope > >>>> with more than just genuine IP headers (which should be backed by > >>>> at least > >>>> 20 bytes of valid memory regardless of what ihl says). > >>> > >>> Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and > >>> ihl of > >>> 5 would be my preference, because I agree with you that this feels > >>> like it shouldn't be happening to start with. > >> > >> How about modify the patch like this? > >> > >> static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > >> { > >> __uint128_t tmp; > >> u64 sum; > >> > >> if (unlikely(ihl < 5)) > >> ihl = 5; > > > > I'd probably do: > > > > /* Callers should really be checking this first */ > > if (WARN_ON_ONCE(ihl < 5)) > > ihl = 5; > > > > because I'd still like to understand what the vlan code is up to. > > > >>>> I still think this smells of papering over some other bug that led > >>>> to a bogus skb getting that far into the transmit stack in the > >>>> first place - presumably it's all wasted effort anyway since a > >>>> "header" with no space for a destination address and a deliberately > >>>> wrong checksum seems unlikely to go very far... > >>> > >>> Looking at the ipvlan_start_xmit() path from the backtrace, it looks > >>> to me like > >>> ipvlan_get_L3_hdr() returns NULL if the header length is invalid, > >>> but then > >>> ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. > >>> Hmm. I really don't know enough about VLANs to know what the right > >>> behaviour is here and I guess just returning NET_XMIT_DROP will > >>> break something. > >> > >> The network maintainer has replied to me, " ip_fast_csum() must be > >> able to handle any value that could fit in the ihl field of the ip > >> protocol header. That's not only the most correct logic, but also the > >> most robust." > > > > Is that on a public list somewhere? Would be a good link for the > > commit message. > > > >> This is a fault injection test, the corrupt function of netem is the > >> emulation of random noise introducing an error in a random position > >> for a chosen percent of packets to test the network module.the netem > >> will modify the packet randomly,so the ihl value of ip header may be modified > to 1. > > > > Ok, but netem is running in userspace (right?) and so I still think > > the network layer can reject the invalid ihl before calling into the > > checksum code. > > Oh, on second look I realise it's probably not that the fault emanates from > dereferencing the actual header itself, it'll be from the code just going > completely bonkers *after* that. I still agree that this case should be avoidable > entirely on the transmit path, but I accept that robustness for the sake of > receive does make good sense. How about this? > > Robin. > > ----->8----- > Subject: [PATCH] arm64: csum: Fix handling of bad packets > > Although iph is expected to point to at least 20 bytes of valid memory, ihl may > be bogus, for example on reception of a corrupt packet. If it happens to be less > than 5, we really don't want to run away and dereference 16GB worth of > memory until it wraps back to exactly zero... > > Fixes: 0e455d8e80aa ("arm64: Implement optimised IP checksum helpers") > Reported-by: guodeqing <geffrey.guo@huawei.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > arch/arm64/include/asm/checksum.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/checksum.h > b/arch/arm64/include/asm/checksum.h > index b6f7bc6da5fb..93a161b3bf3f 100644 > --- a/arch/arm64/include/asm/checksum.h > +++ b/arch/arm64/include/asm/checksum.h > @@ -24,16 +24,17 @@ static inline __sum16 ip_fast_csum(const void *iph, > unsigned int ihl) { > __uint128_t tmp; > u64 sum; > + int n = ihl; /* we want it signed */ > > tmp = *(const __uint128_t *)iph; > iph += 16; > - ihl -= 4; > + n -= 4; > tmp += ((tmp >> 64) | (tmp << 64)); > sum = tmp >> 64; > do { > sum += *(const u32 *)iph; > iph += 4; > - } while (--ihl); > + } while (--n > 0); > Maybe the local temporary variable n is not necessary. static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) { __uint128_t tmp; u64 sum; tmp = *(const __uint128_t *)iph; iph += 16; ihl -= 4; tmp += ((tmp >> 64) | (tmp << 64)); sum = tmp >> 64; do { sum += *(const u32 *)iph; iph += 4; } while ((int)(--ihl) > 0); sum += ((sum >> 32) | (sum << 32)); return csum_fold((__force u32)(sum >> 32)); } Thanks. > sum += ((sum >> 32) | (sum << 32)); > return csum_fold((__force u32)(sum >> 32)); > -- > 2.28.0.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* 答复: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases 2020-07-30 8:44 ` Will Deacon 2020-07-30 9:56 ` Robin Murphy @ 2020-07-30 10:49 ` Guodeqing (A) 1 sibling, 0 replies; 13+ messages in thread From: Guodeqing (A) @ 2020-07-30 10:49 UTC (permalink / raw) To: Will Deacon Cc: catalin.marinas, luke.starrett, kernel-team, Robin Murphy, linux-arm-kernel > -----邮件原件----- > 发件人: Will Deacon [mailto:will@kernel.org] > 发送时间: Thursday, July 30, 2020 16:44 > 收件人: Guodeqing (A) <geffrey.guo@huawei.com> > 抄送: Robin Murphy <robin.murphy@arm.com>; catalin.marinas@arm.com; > kernel-team@android.com; linux-arm-kernel@lists.infradead.org; > luke.starrett@broadcom.com > 主题: Re: 答复: [PATCH,v2] arm64: fix the illegal address access in some cases > > On Wed, Jul 29, 2020 at 07:05:09AM +0000, Guodeqing (A) wrote: > > > On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote: > > > > On 2020-07-28 14:03, Will Deacon wrote: > > > > > Applied to arm64 (for-next/fixes), thanks! > > > > > > > > > > [1/1] arm64: csum: Reject IP headers with 'ihl' field smaller than five > > > > > https://git.kernel.org/arm64/c/09aaef1c5f50 > > > > > > > > I'm not sure your commit message is entirely right there. AFAICS > > > > it's not "the same way as x86" at all - x86 dereferences the first > > > > word of iph and returns that as the sum if ihl <= 4 (and thus is > > > > still capable of crashing given sufficiently bogus data). I'm not > > > > sure where "return 1" came from - if we're going to return > > > > nonsense then the mildly more efficient choice of 0 seems just as good. > > > > > > Argh, yes, that's %1 not $1, so I don't know where the 1 comes from either. > > > Geffrey? > > > > > > > The return 1 is just the report of ip checksum error, the return value > > 0 means the ip checksum correct. x86 dereferences the first word of > > iph and returns that as the sum, this may be just the report of ip checksum > error too. > > On the receive path, sure, but the crash was on the transmit path where we're > computing the checksum to insert into the header, no? [ 388.873996] Call trace: [ 388.874280] ip_send_check+0x40/0x78 [ 388.874657] __ip_local_out+0x78/0x160 [ 388.875060] ip_local_out+0x34/0x68 [ 388.875432] ipvlan_queue_xmit+0x608/0x7f0 [ 388.875870] ipvlan_start_xmit+0x2c/0x90 [ 388.876293] dev_hard_start_xmit+0xac/0x258 [ 388.876734] sch_direct_xmit+0x1a8/0x4c0 [ 388.877149] __qdisc_run+0x88/0xe0 [ 388.877508] __dev_queue_xmit+0x630/0x950 [ 388.877942] dev_queue_xmit+0x24/0x30 [ 388.878356] ip_finish_output2+0x26c/0x420 [ 388.878791] ip_finish_output+0x1c8/0x2a0 The midify action is in the qdisc module, because netem is a kernel module, the kernel config is CONFIG_NET_SCH_NETEM. > > > > > Otherwise it would seem reasonable to jump straight into the > > > > word-at-a-time loop if ip_fast_csum() is really expected to cope > > > > with more than just genuine IP headers (which should be backed by > > > > at least > > > > 20 bytes of valid memory regardless of what ihl says). > > > > > > Either copying the x86 behaviour or WARN_ON_ONCE() and assuming and > > > ihl of > > > 5 would be my preference, because I agree with you that this feels > > > like it shouldn't be happening to start with. > > > > How about modify the patch like this? > > > > static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > > { > > __uint128_t tmp; > > u64 sum; > > > > if (unlikely(ihl < 5)) > > ihl = 5; > > I'd probably do: > > /* Callers should really be checking this first */ > if (WARN_ON_ONCE(ihl < 5)) > ihl = 5; > > because I'd still like to understand what the vlan code is up to. > Maybe ipvlan has to drop the corrupted packet, but ipvlan also has the choice to let the corrupted packet to be dropped by the network stack, the network stack will call the ip_fast_csum. so ip_fast_csum must be robust. > > > > I still think this smells of papering over some other bug that led > > > > to a bogus skb getting that far into the transmit stack in the > > > > first place - presumably it's all wasted effort anyway since a > > > > "header" with no space for a destination address and a > > > > deliberately wrong checksum seems unlikely to go very far... > > > > > > Looking at the ipvlan_start_xmit() path from the backtrace, it looks > > > to me like > > > ipvlan_get_L3_hdr() returns NULL if the header length is invalid, > > > but then > > > ipvlan_xmit_mode_l3() ends up calling ipvlan_process_outbound() anyway. > > > Hmm. I really don't know enough about VLANs to know what the right > > > behaviour is here and I guess just returning NET_XMIT_DROP will > > > break something. > > > > The network maintainer has replied to me, " ip_fast_csum() must be > > able to handle any value that could fit in the ihl field of the ip > > protocol header. That's not only the most correct logic, but also the > > most robust." > > Is that on a public list somewhere? Would be a good link for the commit > message. https://www.spinics.net/lists/netdev/msg671867.html > > > This is a fault injection test, the corrupt function of netem is the > > emulation of random noise introducing an error in a random position > > for a chosen percent of packets to test the network module.the netem > > will modify the packet randomly,so the ihl value of ip header may be modified > to 1. > > Ok, but netem is running in userspace (right?) and so I still think the network > layer can reject the invalid ihl before calling into the checksum code. > netem is a kernel module, the kernel config is CONFIG_NET_SCH_NETEM. https://www.mail-archive.com/netdev@vger.kernel.org/msg342331.html > Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-31 3:06 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-25 2:08 [PATCH,v2] arm64: fix the illegal address access in some cases guodeqing 2020-07-27 11:47 ` Catalin Marinas 2020-07-27 13:29 ` 答复: " Guodeqing (A) 2020-07-28 9:53 ` Catalin Marinas 2020-07-28 13:03 ` Will Deacon 2020-07-28 14:30 ` Robin Murphy 2020-07-28 15:35 ` Will Deacon 2020-07-29 7:05 ` 答复: " Guodeqing (A) 2020-07-30 8:44 ` Will Deacon 2020-07-30 9:56 ` Robin Murphy 2020-07-30 16:03 ` Will Deacon 2020-07-31 3:04 ` 答复: " Guodeqing (A) 2020-07-30 10:49 ` Guodeqing (A)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.