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] [day] [month] [year] [list]

Message-ID: <20200824181726.GR28786@gate.crashing.org>
Date:   Mon, 24 Aug 2020 13:17:26 -0500
From:   Segher Boessenkool <segher@...nel.crashing.org>
To:     Guohua Zhong <zhongguohua1@...wei.com>
Cc:     christophe.leroy@...roup.eu, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        nixiaoming@...wei.com, paulus@...ba.org, stable@...r.kernel.org,
        wangle6@...wei.com
Subject: Re: Re：Re: [PATCH] powerpc: Fix a bug in __div64_32 if divisor is zero

On Mon, Aug 24, 2020 at 07:54:07PM +0800, Guohua Zhong wrote:
> >> 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. 
> 
> > That is now what the PowerPC integer divide insns do: they just leave
> > the result undefined (and they can set the overflow flag then, but no
> > one uses that).
> 
> OK ,So just keep the patch as below. If this patch looks good for you, please
> help to review. I will send the new patch later.
> 
> Thanks for your reply.
> 
> diff --git a/arch/powerpc/boot/div64.S b/arch/powerpc/boot/div64.S
> index 4354928ed62e..1d3561cf16fa 100644
> --- a/arch/powerpc/boot/div64.S
> +++ b/arch/powerpc/boot/div64.S
> @@ -13,8 +13,10 @@
> 
>         .globl __div64_32
>         .globl __div64_32
>  __div64_32:
> + cmplwi      r4,0    # check if divisor r4 is zero
>         lwz     r5,0(r3)        # get the dividend into r5/r6
>         lwz     r6,4(r3)
> + beq 5f                      # jump to label 5 if r4(divisor) is zero

Just "beqlr".

This instruction scheduling hurts all CPUs that aren't 8xx, fwiw (but
likely only in the case where r4 *is* zero, so who cares :-) )

So...  What is the *goal* of this patch?  It looks like the routine
would not get into a loop if r4 is 0, just return the wrong result?
But, it *always* will, there *is* no right result?

No caller should call it with zero as divisor ever, so in that sense,
checking for it in the division routine is just pure wasted work.


Segher