-----邮件原件-----
发件人: Robin Murphy [mailto:robin.murphy@arm.com]
发送时间: Thursday, July 30, 2020 17:57
收件人: Will Deacon [off-list ref]; Guodeqing (A)
[off-list ref]
抄送: 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:

quoted

On Wed, Jul 29, 2020 at 07:05:09AM +0000, Guodeqing (A) wrote:

quoted

quoted

On Tue, Jul 28, 2020 at 03:30:50PM +0100, Robin Murphy wrote:

quoted

On 2020-07-28 14:03, Will Deacon wrote:

quoted

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.

quoted

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?

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

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.

quoted

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 <redacted>
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);