linux-kernel - Re: Re：Re: [PATCH] powerpc: Fix a bug in __div64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 22 Aug 2020 19:25:24 +0200
From:   Gabriel Paubert <paubert@...m.es>
To:     Guohua Zhong <zhongguohua1@...wei.com>
Cc:     christophe.leroy@...roup.eu, nixiaoming@...wei.com,
        wangle6@...wei.com, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        paulus@...ba.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: Re：Re: [PATCH] powerpc: Fix a
 bug in __div64_32 if divisor is zero

On Sun, Aug 23, 2020 at 12:54:33AM +0800, Guohua Zhong wrote:
> >In generic version in lib/math/div64.c, there is no checking of 'base' 
> >either.
> >Do we really want to add this check in the powerpc version only ?
> 
> >The only user of __div64_32() is do_div() in 
> >include/asm-generic/div64.h. Wouldn't it be better to do the check there ?
> 
> >Christophe
> 
> Yet, I have noticed that there is no checking of 'base' in these functions.
> But I am not sure how to check is better.As we know that the result is 
> undefined when divisor is zero. It maybe good to print error and dump stack.
>  Let the process to know that the divisor is zero by sending SIGFPE. 
> 
> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
> index a3b98c86f077..161c656ee3ee 100644
> --- a/include/asm-generic/div64.h
> +++ b/include/asm-generic/div64.h
> @@ -43,6 +43,11 @@
>  # define do_div(n,base) ({                                     \
>         uint32_t __base = (base);                               \
>         uint32_t __rem;                                         \
> + if (unlikely(base == 0)) {                          \
> +         pr_err("do_div base=%d\n",base);            \
> +         dump_stack();                               \
> +         force_sig(SIGFPE);                          \
> + }      
> 

I suspect this will generate a strong reaction. SIGFPE is for user space
instruction attempting a division by zero. A division by zero in the
kernel is a kernel bug, period, and you don't want to kill a user
process for this reason.

If it happens in an interrupt, the context of the kernel may not even be
related to the current process.

Many other architectures (x86 for example) already trigger an exception
on a division by zero but the handler will find that the exception
happened in kernel context and generate an Oops, not raise a signal in a
(possibly innocent) userland process.

	Gabriel

> Then it also needto add this checking in functions of
> div64_s64(), div64_u64(), div64_u64_rem(), div_s64_rem and div_u64_rem () 
> in include/linux/math64.h
> 
> + if (unlikely(divisor == 0)) {
> +         pr_err("%s divisor=0\n",__func__);
> +         dump_stack();
> +         force_sig(SIGFPE);
> + }
> 
> Guohua
> 
> >>  	lwz	r5,0(r3)	# get the dividend into r5/r6
> >>  	lwz	r6,4(r3)
> >>  	cmplw	r5,r4
> >>@@ -52,6 +55,7 @@ __div64_32:
> >>  4:	stw	r7,0(r3)	# return the quotient in *r3
> >>  	stw	r8,4(r3)
> >>  	mr	r3,r6		# return the remainder in r3
> >>+5:					# return if divisor r4 is zero
> >>  	blr
> >>  
> >>  /*
> >>diff --git a/arch/powerpc/lib/div64.S b/arch/powerpc/lib/div64.S
> >>index 3d5426e7dcc4..1cc9bcabf678 100644
> >>--- a/arch/powerpc/lib/div64.S
> >>+++ b/arch/powerpc/lib/div64.S
> >>@@ -13,6 +13,9 @@
> >>  #include <asm/processor.h>
> >>  
> >>  _GLOBAL(__div64_32)
> >>+	li	r9,0
> >>+	cmplw	r4,r9	# check if divisor r4 is zero
> >>+	beq	5f			# jump to label 5 if r4(divisor) is zero
> >>  	lwz	r5,0(r3)	# get the dividend into r5/r6
> >>  	lwz	r6,4(r3)
> >>  	cmplw	r5,r4
> >>@@ -52,4 +55,5 @@ _GLOBAL(__div64_32)
> >>  4:	stw	r7,0(r3)	# return the quotient in *r3
> >>  	stw	r8,4(r3)
> >>  	mr	r3,r6		# return the remainder in r3
> >>+5:					# return if divisor r4 is zero
> >>  	blr
> >>
>