linux-kernel - Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line

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: <CAKv+Gu9LJzMm_xGQOZ=wS6Wr=W1hN7cS+GsVAS0mPUFJ3NT0ng@mail.gmail.com>
Date:   Fri, 7 Dec 2018 19:01:52 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Will Deacon <will.deacon@....com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Robin Murphy <robin.murphy@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <marc.zyngier@....com>,
        "Suzuki K. Poulose" <suzuki.poulose@....com>,
        Dave Martin <Dave.Martin@....com>
Subject: Re: [PATCH 0/5] arm64: assorted fixes for dcache_by_line_op

On Fri, 7 Dec 2018 at 18:53, Will Deacon <will.deacon@....com> wrote:
>
> Hi Ard,
>
> Cheers for looking at this.
>
> On Thu, Dec 06, 2018 at 04:57:34PM +0100, Ard Biesheuvel wrote:
> > This fixes two issues in dcache_by_line_op: patch #4 fixes the logic
> > that is applied when patching DC CVAP instructions, and patch #5 gets
> > rid of some unintended undefined symbols resulting from incorrect use
> > of conditional GAS directives.
> >
> > Patches #1 to #3 are groundwork for #4.
> >
> > Cc: Robin Murphy <robin.murphy@....com>
> > Cc: Will Deacon <will.deacon@....com>
> > Cc: Catalin Marinas <catalin.marinas@....com>
> > Cc: Marc Zyngier <marc.zyngier@....com>
> > Cc: Suzuki Poulose <suzuki.poulose@....com>
> > Cc: Dave Martin <Dave.Martin@....com>
> >
> > Ard Biesheuvel (5):
> >   arm64/alternative_cb: move callback reference into replacements
> >     section
> >   arm64/alternative_cb: add nr_alts parameter to callback handlers
> >   arm64/alternative_cb: add support for alternative sequences
> >   arm64/assembler: use callback to 3-way alt-patch DC CVAP instructions
> >   arm64/mm: use string comparisons in dcache_by_line_op
> >
> >  arch/arm64/include/asm/alternative.h | 54 +++++++++++---------
> >  arch/arm64/include/asm/assembler.h   | 17 +++---
> >  arch/arm64/include/asm/kvm_mmu.h     |  4 +-
> >  arch/arm64/kernel/alternative.c      | 22 +++++---
> >  arch/arm64/kernel/cpu_errata.c       | 24 ++++++---
> >  arch/arm64/kvm/va_layout.c           |  8 +--
> >  6 files changed, 79 insertions(+), 50 deletions(-)
>
> Whilst I can see that this solves the problem, I do wonder whether the
> additional infrastructure is really worth it. Couldn't we just do something
> really simple like the diff below instead?
>

That looks fine to me, although not as elegant :-) You are in a better
position to assess if the additional infrastructure may see other uses
in the near future.

In any case, I don't feel strongly about it, so pick whatever you feel
is most suitable. (I just spotted the undefined symbols when I was
playing with lld.)


> --->8
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 953208267252..8dea015b6599 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -390,27 +390,38 @@ alternative_endif
>   *     size:           size of the region
>   *     Corrupts:       kaddr, size, tmp1, tmp2
>   */
> +       .macro __dcache_op_workaround_clean_cache, op, kaddr
> +alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> +       dc      \op, \kaddr
> +alternative_else
> +       dc      civac, \kaddr
> +alternative_endif
> +       .endm
> +
>         .macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
>         dcache_line_size \tmp1, \tmp2
>         add     \size, \kaddr, \size
>         sub     \tmp2, \tmp1, #1
>         bic     \kaddr, \kaddr, \tmp2
>  9998:
> -       .if     (\op == cvau || \op == cvac)
> -alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> -       dc      \op, \kaddr
> -alternative_else
> -       dc      civac, \kaddr
> -alternative_endif
> -       .elseif (\op == cvap)
> +       .ifc    \op, cvau
> +       __dcache_op_workaround_clean_cache \op, \kaddr
> +       .else
> +       .ifc    \op, cvac
> +       __dcache_op_workaround_clean_cache \op, \kaddr
> +       .else
> +       .ifc    \op, cvap
>  alternative_if ARM64_HAS_DCPOP
>         sys 3, c7, c12, 1, \kaddr       // dc cvap
> -alternative_else
> -       dc      cvac, \kaddr
> -alternative_endif
> +       b       9996f
> +alternative_else_nop_endif
> +       __dcache_op_workaround_clean_cache cvac, \kaddr
> +9996:
>         .else
>         dc      \op, \kaddr
>         .endif
> +       .endif
> +       .endif
>         add     \kaddr, \kaddr, \tmp1
>         cmp     \kaddr, \size
>         b.lo    9998b