linux-kernel - Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

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]

Message-ID: <20170428095349.GA22161@leverpostej>
Date:   Fri, 28 Apr 2017 10:53:51 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Matthias Kaehlcke <mka@...omium.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Tejun Heo <tj@...nel.org>, Christoph Lameter <cl@...ux.com>,
        Vladimir Murzin <vladimir.murzin@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
        KVM devel mailing list <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Grant Grundler <grundler@...omium.org>,
        Greg Hackmann <ghackmann@...gle.com>,
        Michael Davidson <md@...gle.com>
Subject: Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> On 27 April 2017 at 23:52, Matthias Kaehlcke <mka@...omium.org> wrote:
> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:

> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >>
> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> address must be 64-bit, so this would mask a legitimate warning.
> >>
> >> Given the prototype of this function the code if fine either way, but
> >> were we to refactor things (e.g. making this a macro), that might not be
> >> true.
> >>
> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >
> > Good point, I'll leave instances dealing with addresses untouched for now.
> >
> 
> OK, I am confused now. We started this thread under the assumption
> that all unqualified placeholders are warned about by Clang. Given
> that this appears not to be the case, could we please first find out
> what causes the warnings?

Yes please.

> Is it necessary at all to add the x modifiers for 64-bit types?

Having delved a little deeper, I think this is actively harmful, and
clang's warning indicates potential problems even when compiling with
GCC.

The below test simulates how we might write to control regs and so on,
with a mov in asm simulating something like an msr.

---->8----
#include <stdio.h>

static inline unsigned long generate_val(void)
{
        unsigned long val;

        /* hide value generation from GCC */
        asm (
                "movn %0, #0"
                : "=r" (val)
        );

        return val;
}

static inline unsigned long use_val_32(unsigned int in)
{
        unsigned long out;

	/* simulate what we might write to a sysreg */
        asm (
                "mov %x0, %x1"
                : "=r" (out)
                : "r" (in)
        );

        return out;
}

int main(int argc, char *argv)
{
        printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));

        return 0;
}
---->8----

Depending on optimization level, bits that we do not expect can flow through:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x00000000ffffffff
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0xffffffffffffffff
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0xffffffffffffffff

... that could be disastrous depending on how the result was used.

With "in" cast to an unsigned long, the compiler realises it needs to perform
any necessary truncation itself:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x00000000ffffffff
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0x00000000ffffffff
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0x00000000ffffffff
$ gcc test.c -O3 -o test
$ ./test
32-bit val is: 0x00000000ffffffff

I think that the correct fix is to use intermediate 64-bit variables, or
casts, so that the compiler *must* use an x register, and consequently
guarantees that all 64-bits of the register are as we expect.

Thanks,
Mark.