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

[Guodeqing (A)]

> -----邮件原件-----
> 发件人: Robin Murphy [mailto:robin.murphy at arm.com]
> 发送时间: Thursday, July 30, 2020 17:57
> 收件人: Will Deacon <will at kernel.org>; Guodeqing (A)
> <geffrey.guo at huawei.com>
> 抄送: catalin.marinas at arm.com; kernel-team at android.com;
> linux-arm-kernel at lists.infradead.org; luke.starrett at 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 at huawei.com>
> Signed-off-by: Robin Murphy <robin.murphy at 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