| 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
| ||
|
Message-ID: <CAMj1kXGuHw+p5=YPrVwaHjp5hQ9uxsp7hbA0Vk-ppZ3_qHDVrA@mail.gmail.com>
Date: Mon, 30 Nov 2020 18:18:15 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Nicolas Pitre <nico@...xnic.net>
Cc: Antony Yu <swpenim@...il.com>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Russell King <linux@...linux.org.uk>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
clang-built-linux <clang-built-linux@...glegroups.com>,
Nathan Chancellor <natechancellor@...il.com>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RESEND,PATCH] ARM: fix __div64_32() error when compiling with clang
On Mon, 30 Nov 2020 at 16:51, Nicolas Pitre <nico@...xnic.net> wrote:
>
> On Mon, 30 Nov 2020, Ard Biesheuvel wrote:
>
> > (+ Nico)
> >
> > On Mon, 30 Nov 2020 at 11:11, Ard Biesheuvel <ardb@...nel.org> wrote:
> > >
> > > On Mon, 23 Nov 2020 at 08:39, Antony Yu <swpenim@...il.com> wrote:
> > > >
> > > > __do_div64 clobbers the input register r0 in little endian system.
> > > > According to the inline assembly document, if an input operand is
> > > > modified, it should be tied to a output operand. This patch can
> > > > prevent compilers from reusing r0 register after asm statements.
> > > >
> > > > Signed-off-by: Antony Yu <swpenim@...il.com>
> > > > ---
> > > > arch/arm/include/asm/div64.h | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> > > > index 898e9c78a7e7..809efc51e90f 100644
> > > > --- a/arch/arm/include/asm/div64.h
> > > > +++ b/arch/arm/include/asm/div64.h
> > > > @@ -39,9 +39,10 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> > > > asm( __asmeq("%0", __xh)
> > > > __asmeq("%1", "r2")
> > > > __asmeq("%2", "r0")
> > > > - __asmeq("%3", "r4")
> > > > + __asmeq("%3", "r0")
> > > > + __asmeq("%4", "r4")
> > > > "bl __do_div64"
> > > > - : "=r" (__rem), "=r" (__res)
> > > > + : "=r" (__rem), "=r" (__res), "=r" (__n)
> > > > : "r" (__n), "r" (__base)
> > > > : "ip", "lr", "cc");
> > > > *n = __res;
> > > > --
> > > > 2.23.0
> > > >
> > >
> > > Agree that using r0 as an input operand only is incorrect, and not
> > > only on Clang. The compiler might assume that r0 will retain its value
> > > across the asm() block, which is obviously not the case.
>
> You're right.
>
> This was done like that most likely to work around some stupid code
> generation with "__n >> 32" while using gcc from about 20 years ago. IOW
> I don't exactly remember why I did it like that, but it is certainly
> flawed.
>
> Here's my version of the fix which should be correct. Warning: this
> is completely untested, but should in theory produce the same code on
> modern gcc.
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index 898e9c78a7..595e538f5b 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -21,29 +21,20 @@
> * assembly implementation with completely non standard calling convention
> * for arguments and results (beware).
> */
> -
> -#ifdef __ARMEB__
> -#define __xh "r0"
> -#define __xl "r1"
> -#else
> -#define __xl "r0"
> -#define __xh "r1"
> -#endif
> -
> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> {
> register unsigned int __base asm("r4") = base;
> register unsigned long long __n asm("r0") = *n;
> register unsigned long long __res asm("r2");
> - register unsigned int __rem asm(__xh);
> - asm( __asmeq("%0", __xh)
> + unsigned int __rem;
> + asm( __asmeq("%0", "r0")
> __asmeq("%1", "r2")
> - __asmeq("%2", "r0")
> - __asmeq("%3", "r4")
> + __asmeq("%2", "r4")
> "bl __do_div64"
> - : "=r" (__rem), "=r" (__res)
> - : "r" (__n), "r" (__base)
> + : "+r" (__n), "=r" (__res)
> + : "r" (__base)
> : "ip", "lr", "cc");
> + __rem = __n >> 32;
This treats {r0, r1} as a {low, high} pair, regardless of endianness,
and so it puts the value of r0 into r1. Doesn't that mean the shift
should only be done on little endian?
> *n = __res;
> return __rem;
> }
>
> I'll submit it if someone confirms it works.
>
>
> Nicolas
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Powered by blists - more mailing lists