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: <20170427110256.GC31337@leverpostej>
Date:   Thu, 27 Apr 2017 12:02:56 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     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>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        kvm@...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

Hi,

On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> Many inline assembly statements don't include the 'x' modifier when
> using xN registers as operands. This is perfectly valid, however it
> causes clang to raise warnings like this:
> 
> warning: value size does not match register size specified by the
>   constraint and modifier [-Wasm-operand-widths]
> ...
> arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>   '__smp_store_release'
>     asm volatile ("stlr %1, %0"
> 
> Add the modifiers to keep clang happy.

If we're going to make this consistent, it would make sense to similarly
annotate 'w' regs. That will make it easier going forward to enforce a
policy that registers are suitably annotated.

Also, there's a risk that we silently mask a bug here, for which clang's
warning is legitimate, so we need to review this very carefully...

> 
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
> Changes in v2:
> - also add modifiers to multiline ASM statements in include/asm/
>   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
>   that were missed on v1
> 
>  arch/arm64/include/asm/arch_gicv3.h     |  2 +-
>  arch/arm64/include/asm/atomic_ll_sc.h   | 36 ++++++++++++++++-----------------
>  arch/arm64/include/asm/barrier.h        |  4 ++--
>  arch/arm64/include/asm/io.h             | 24 +++++++++++-----------
>  arch/arm64/include/asm/irqflags.h       | 10 ++++-----
>  arch/arm64/include/asm/kvm_hyp.h        | 10 ++++-----
>  arch/arm64/include/asm/kvm_mmu.h        | 12 +++++------
>  arch/arm64/include/asm/percpu.h         |  4 ++--
>  arch/arm64/include/asm/pgtable.h        | 20 +++++++++---------
>  arch/arm64/include/asm/sysreg.h         |  4 ++--
>  arch/arm64/include/asm/uaccess.h        | 14 ++++++-------
>  arch/arm64/include/asm/word-at-a-time.h | 14 ++++++-------
>  arch/arm64/kernel/armv8_deprecated.c    |  4 ++--
>  arch/arm64/kernel/probes/kprobes.c      |  2 +-
>  arch/arm64/kvm/hyp/switch.c             |  4 ++--
>  15 files changed, 82 insertions(+), 82 deletions(-)

... to that end, could you split these into a few patches?

That way, knowledgeable people can focus their review on the code they
understand.

That doesn't need to be a patch per file; all the KVM bits can be
collated in one patch, for example. However, the atomics, kvm, and
uaccess+word-at-a-time bits should certainly be separate patches given
their (existing) complexity.

Otherwise, I have a couple of comments below.

> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index f37e3a21f6e7..ba54e5bee885 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
>  
>  static inline void gic_write_bpr1(u32 val)
>  {
> -	asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
> +	asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
>  }

Please make this use write_sysreg_s() instead, i.e.

static inline void gic_write_bpr1(u32 val)
{
	write_sysreg_s(var, ICC_BPR1_EL1);
}

... that uses the 'x' modifier internally, and it's what we do for the
other GIC sysreg accesors.

This accessor was missed by commit:

  d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to {read,write}_sysreg_s")

... because it was added concurrently by commitL

  91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")

... i.e. it was not deliberately omitted.

[...]

> -	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.

Thanks,
Mark.