[PATCH] arm64/alternatives: use subsections for replacement sequences

[Ard Biesheuvel]

On Thu, 9 Jul 2020 at 15:31, Ard Biesheuvel <ardb at kernel.org> wrote:
>
> On Thu, 9 Jul 2020 at 14:11, Alexandru Elisei <alexandru.elisei at arm.com> wrote:
> >
> > Hi,
> >
>
> Hi Alex,
>
> Apologies for the breakage.
>
>
> > I should post the entire boot log:
> ...
> > [    0.002204] pc : work_pending+0x32c/0x348
>
> This suggests that you ended executing directly from the alternatives
> replacement section that gets appended to the end of work_pending (and
> therefore gets mistaken for being part of it)
>
> It appears that the following code in alternatives.c
>
> static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
> {
>     unsigned long replptr;
>
>     if (kernel_text_address(pc))
>        return true;
>
> returns true inadvertently for the branch in this piece of code in entry.S
>
> alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>     ldr x20, [sp, #S_PMR_SAVE]
>     msr_s SYS_ICC_PMR_EL1, x20
>     mrs_s x21, SYS_ICC_CTLR_EL1
>     tbz x21, #6, .L__skip_pmr_sync\@ // Check for ICC_CTLR_EL1.PMHE
>     dsb sy // Ensure priority change is seen by redistributor
> .L__skip_pmr_sync\@:
>
>
> due to the fact that kernel_text_address() has no way of
> distinguishing branches inside the subsection from branches that
> require updating. So the alternatives patching code dutifully updates
> the tbz opcode and points it to its original target in the subsection.
>
> This is going to be rather tricky to fix, unless we special case
> tbz/cbz branches and other branches with limited range that would
> never have worked before anyway.
>
> For now, better to just revert it and revisit it later.
>

... unless we decide to fix up all branches pointing outside the
replacement sequence, which is not an entirely unreasonable thing to
do:

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index d1757ef1b1e7..7c205f9202a3 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -45,18 +45,11 @@
 {
        unsigned long replptr;

-       if (kernel_text_address(pc))
-               return true;
-
        replptr = (unsigned long)ALT_REPL_PTR(alt);
        if (pc >= replptr && pc <= (replptr + alt->alt_len))
                return false;

-       /*
-        * Branching into *another* alternate sequence is doomed, and
-        * we're not even trying to fix it up.
-        */
-       BUG();
+       return true;
 }