arm64: Fix compile error with KVM and !HARDEN_BRANCH_PREDICTOR

Commit Message

Corey Minyard Nov. 27, 2019, 6:10 p.m. UTC

From: Corey Minyard <[e-mail]>

When compiling with KVM enabled and without HARDEN_BRANCH_PREDICTOR,
the following compile error happens:

arch/arm64/kernel/cpu_errata.c:92:23:
error: '__bp_harden_hyp_vecs_start' undeclared (first use in this function);
did you mean 'hyp_vecs_start'?
  void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);

Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
spectre-v2 vulnerability detection" for CONFIG_HARDEN_BRANCH_PREDICTOR,
but __bp_harden_hyp_vecs_start is only defined if that config is
enabled.

Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has CONFIG_KVM,
It looks like you need both of those for that code to be valid.

Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2 vulnerability detection"
Cc: Andre Przywara <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Stefan Wahren <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Ard Biesheuvel <[e-mail]>
Signed-off-by: Corey Minyard <[e-mail]>
---
This is for 4.14, I'm not sure if it is needed for other kernels.

It is not needed in master because a new config item was added,
CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling the
patches that add the required changes, and they make a lot of
changes.  This change is the simple fix, but I'm not sure if we want to
pull all those other changes into 4.14 and whatever other kernels
are required.

 arch/arm64/kernel/cpu_errata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ard Biesheuvel Nov. 28, 2019, 5:20 p.m. UTC | #1

(+ Marc)



On Wed, 27 Nov 2019 at 19:10, <[e-mail]> wrote:
>
> From: Corey Minyard <[e-mail]>
>
> When compiling with KVM enabled and without HARDEN_BRANCH_PREDICTOR,
> the following compile error happens:
>
> arch/arm64/kernel/cpu_errata.c:92:23:
> error: '__bp_harden_hyp_vecs_start' undeclared (first use in this function);
> did you mean 'hyp_vecs_start'?
>   void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);
>
> Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
> spectre-v2 vulnerability detection" for CONFIG_HARDEN_BRANCH_PREDICTOR,
> but __bp_harden_hyp_vecs_start is only defined if that config is
> enabled.
>
> Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has CONFIG_KVM,
> It looks like you need both of those for that code to be valid.
>
> Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2 vulnerability detection"
> Cc: Andre Przywara <[e-mail]>
> Cc: Catalin Marinas <[e-mail]>
> Cc: Stefan Wahren <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Ard Biesheuvel <[e-mail]>
> Signed-off-by: Corey Minyard <[e-mail]>
> ---
> This is for 4.14, I'm not sure if it is needed for other kernels.
>
> It is not needed in master because a new config item was added,
> CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
> HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling the
> patches that add the required changes, and they make a lot of
> changes.  This change is the simple fix, but I'm not sure if we want to
> pull all those other changes into 4.14 and whatever other kernels
> are required.
>

I agree that backporting this cleanly is going to be problematic,
since it pulls in the entire EL2 VA randomization feature with all its
prerequisites.

Backporting the following set could be done fairly cleanly, and fixes
the problem at build time, but unfortunately, it causes a boot time
crash (see below)

97eca4d2bfec (HEAD -> linux-4.14.y) arm64: Move the content of bpi.S
to hyp-entry.S
514dd33114c6 arm64: Make BP hardening slot counter available
d7ddf3ae9470 arm64; insn: Add encoder for the EXTR instruction
c0b2c4e56e10 arm64: KVM: Introduce EL2 VA randomisation
56ab0a87c737 arm64: KVM: Dynamically compute the HYP VA mask
d92c02628dfb arm64: KVM: Allow far branches from vector slots to the
main vectors
7adec01c9674 arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
1095e4fc3134 arm64: KVM: Move stashing of x0/x1 into the vector code itself
bb2e1aceb423 arm64: KVM: Dynamically patch the kernel/hyp VA mask
6f0ccfc451be arm64: KVM: Reserve 4 additional instructions in the BPI template
bf425ffee07a arm64: insn: Add encoder for bitwise operations using literals
41dda94d1a9f arm64: insn: Add N immediate encoding
3225668ebe00 arm64: KVM: Move BP hardening vectors into .hyp.text section

Marc?

[    0.062126] CPU: All CPU(s) started at EL1
[    0.063109] alternatives: patching kernel code
[    0.064228] random: get_random_u64 called from
compute_layout+0x94/0xe8 with crng_init=0
[    0.066313] aarch64_insn_gen_add_sub_imm: invalid immediate encoding 1904640
[    0.068136] ------------[ cut here ]------------
[    0.069327] kernel BUG at arch/arm64/kvm/va_layout.c:148!
[    0.070727] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[    0.072143] Modules linked in:
[    0.072932] Process migration/0 (pid: 11, stack limit = 0xffff000009110000)
[    0.074735] CPU: 0 PID: 11 Comm: migration/0 Not tainted
4.14.156-00013-g97eca4d2bfec #135
[    0.076869] Hardware name: linux,dummy-virt (DT)
[    0.078061] task: ffff8000058f8d80 task.stack: ffff000009110000
[    0.079597] PC is at kvm_update_va_mask+0x188/0x1b4
[    0.080858] LR is at kvm_update_va_mask+0x160/0x1b4
[    0.082116] pc : [<ffff000008e170f8>] lr : [<ffff000008e170d0>]
pstate: 604000c5
[    0.084030] sp : ffff000009113ca0
[    0.084885] x29: ffff000009113ca0 x28: 0000000000000000
[    0.086256] x27: ffff00000803bcf8 x26: 0000000091400021
[    0.087633] x25: 00000000d4202000 x24: ffff0000080ac92c
[    0.089005] x23: ffff8000000ac92c x22: 0000000000000003
[    0.090382] x21: 0000000000000001 x20: ffff000009058848
[    0.091756] x19: 0000000000000003 x18: 0000000000000001
[    0.093130] x17: 0000000076158468 x16: 000000009e488a0e
[    0.094507] x15: 0000000000000010 x14: 3034363430393120
[    0.095881] x13: 676e69646f636e65 x12: 206574616964656d
[    0.097252] x11: 6d692064696c6176 x10: 6e69203a6d6d695f
[    0.098629] x9 : 6275735f6464615f x8 : ffff00000856d0c8
[    0.100000] x7 : 5f34366863726161 x6 : 000000000000005a
[    0.101372] x5 : 0000000000000000 x4 : 0000000000000000
[    0.102748] x3 : ffffffffffffffff x2 : ffff000008f53500
[    0.104116] x1 : ffff8000058f8d80 x0 : 00000000d4202000
[    0.105485] Call trace:
[    0.106115] Exception stack(0xffff000009113b60 to 0xffff000009113ca0)
[    0.107780] 3b60: 00000000d4202000 ffff8000058f8d80
ffff000008f53500 ffffffffffffffff
[    0.109798] 3b80: 0000000000000000 0000000000000000
000000000000005a 5f34366863726161
[    0.111821] 3ba0: ffff00000856d0c8 6275735f6464615f
6e69203a6d6d695f 6d692064696c6176
[    0.113838] 3bc0: 206574616964656d 676e69646f636e65
3034363430393120 0000000000000010
[    0.115862] 3be0: 000000009e488a0e 0000000076158468
0000000000000001 0000000000000003
[    0.117879] 3c00: ffff000009058848 0000000000000001
0000000000000003 ffff8000000ac92c
[    0.119901] 3c20: ffff0000080ac92c 00000000d4202000
0000000091400021 ffff00000803bcf8
[    0.121918] 3c40: 0000000000000000 ffff000009113ca0
ffff000008e170d0 ffff000009113ca0
[    0.123941] 3c60: ffff000008e170f8 00000000604000c5
000000001430322e 0000000043b87f45
[    0.125959] 3c80: ffffffffffffffff ffff000008e170d0
ffff000009113ca0 ffff000008e170f8
[    0.127982] [<ffff000008e170f8>] kvm_update_va_mask+0x188/0x1b4
[    0.129508] [<ffff00000808f220>] __apply_alternatives+0xe8/0x138
[    0.131060] [<ffff00000808f2e8>] __apply_alternatives_multi_stop+0x78/0x98
[    0.132832] [<ffff000008161c9c>] multi_cpu_stop+0x94/0x118
[    0.134248] [<ffff000008161f50>] cpu_stopper_thread+0x98/0x120
[    0.135759] [<ffff0000080ee5dc>] smpboot_thread_fn+0x124/0x248
[    0.137265] [<ffff0000080ea6d0>] kthread+0x128/0x130
[    0.138553] [<ffff000008084e10>] ret_from_fork+0x10/0x18

Marc Zyngier Nov. 29, 2019, 7:21 a.m. UTC | #2

On Thu, 28 Nov 2019 17:20:20 +0000,

[fixing Will's email address]

Ard Biesheuvel <[e-mail]> wrote:
> 
> (+ Marc)
> 
> 
> 
> On Wed, 27 Nov 2019 at 19:10, <[e-mail]> wrote:
> >
> > From: Corey Minyard <[e-mail]>
> >
> > When compiling with KVM enabled and without HARDEN_BRANCH_PREDICTOR,
> > the following compile error happens:
> >
> > arch/arm64/kernel/cpu_errata.c:92:23:
> > error: '__bp_harden_hyp_vecs_start' undeclared (first use in this function);
> > did you mean 'hyp_vecs_start'?
> >   void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);
> >
> > Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
> > spectre-v2 vulnerability detection" for CONFIG_HARDEN_BRANCH_PREDICTOR,
> > but __bp_harden_hyp_vecs_start is only defined if that config is
> > enabled.
> >
> > Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has CONFIG_KVM,
> > It looks like you need both of those for that code to be valid.
> >
> > Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2 vulnerability detection"
> > Cc: Andre Przywara <[e-mail]>
> > Cc: Catalin Marinas <[e-mail]>
> > Cc: Stefan Wahren <[e-mail]>
> > Cc: Will Deacon <[e-mail]>
> > Cc: Ard Biesheuvel <[e-mail]>
> > Signed-off-by: Corey Minyard <[e-mail]>
> > ---
> > This is for 4.14, I'm not sure if it is needed for other kernels.
> >
> > It is not needed in master because a new config item was added,
> > CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
> > HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling the
> > patches that add the required changes, and they make a lot of
> > changes.  This change is the simple fix, but I'm not sure if we want to
> > pull all those other changes into 4.14 and whatever other kernels
> > are required.
> >
> 
> I agree that backporting this cleanly is going to be problematic,
> since it pulls in the entire EL2 VA randomization feature with all its
> prerequisites.
> 
> Backporting the following set could be done fairly cleanly, and fixes
> the problem at build time, but unfortunately, it causes a boot time
> crash (see below)
> 
> 97eca4d2bfec (HEAD -> linux-4.14.y) arm64: Move the content of bpi.S
> to hyp-entry.S
> 514dd33114c6 arm64: Make BP hardening slot counter available
> d7ddf3ae9470 arm64; insn: Add encoder for the EXTR instruction
> c0b2c4e56e10 arm64: KVM: Introduce EL2 VA randomisation
> 56ab0a87c737 arm64: KVM: Dynamically compute the HYP VA mask
> d92c02628dfb arm64: KVM: Allow far branches from vector slots to the
> main vectors
> 7adec01c9674 arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
> 1095e4fc3134 arm64: KVM: Move stashing of x0/x1 into the vector code itself
> bb2e1aceb423 arm64: KVM: Dynamically patch the kernel/hyp VA mask
> 6f0ccfc451be arm64: KVM: Reserve 4 additional instructions in the BPI template
> bf425ffee07a arm64: insn: Add encoder for bitwise operations using literals
> 41dda94d1a9f arm64: insn: Add N immediate encoding
> 3225668ebe00 arm64: KVM: Move BP hardening vectors into .hyp.text section
> 
> Marc?

You need at least these:

1bb32a44aea1 KVM: arm/arm64: Keep GICv2 HYP VAs in kvm_vgic_global_state
44a497abd621 KVM: arm/arm64: Do not use kern_hyp_va() with kvm_vgic_global_state

But that's definitely not enough to fix the crash.

> 
> [    0.062126] CPU: All CPU(s) started at EL1
> [    0.063109] alternatives: patching kernel code
> [    0.064228] random: get_random_u64 called from
> compute_layout+0x94/0xe8 with crng_init=0
> [    0.066313] aarch64_insn_gen_add_sub_imm: invalid immediate encoding 1904640

OK, that one is really bizarre. This value (tag_val) is supposed to be
a small value (only 12 significant bits out of 24 at any given time),
and it is not (0x1D1000). So somehow compute_instruction() is not
doing the right thing.

Do you have a tree somewhere with this patches?

Thanks,

	M.

Ard Biesheuvel Nov. 29, 2019, 7:25 a.m. UTC | #3

On Fri, 29 Nov 2019 at 08:21, Marc Zyngier <[e-mail]> wrote:
>
> On Thu, 28 Nov 2019 17:20:20 +0000,
>
> [fixing Will's email address]
>
> Ard Biesheuvel <[e-mail]> wrote:
> >
> > (+ Marc)
> >
> >
> >
> > On Wed, 27 Nov 2019 at 19:10, <[e-mail]> wrote:
> > >
> > > From: Corey Minyard <[e-mail]>
> > >
> > > When compiling with KVM enabled and without HARDEN_BRANCH_PREDICTOR,
> > > the following compile error happens:
> > >
> > > arch/arm64/kernel/cpu_errata.c:92:23:
> > > error: '__bp_harden_hyp_vecs_start' undeclared (first use in this function);
> > > did you mean 'hyp_vecs_start'?
> > >   void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * SZ_2K);
> > >
> > > Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
> > > spectre-v2 vulnerability detection" for CONFIG_HARDEN_BRANCH_PREDICTOR,
> > > but __bp_harden_hyp_vecs_start is only defined if that config is
> > > enabled.
> > >
> > > Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has CONFIG_KVM,
> > > It looks like you need both of those for that code to be valid.
> > >
> > > Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2 vulnerability detection"
> > > Cc: Andre Przywara <[e-mail]>
> > > Cc: Catalin Marinas <[e-mail]>
> > > Cc: Stefan Wahren <[e-mail]>
> > > Cc: Will Deacon <[e-mail]>
> > > Cc: Ard Biesheuvel <[e-mail]>
> > > Signed-off-by: Corey Minyard <[e-mail]>
> > > ---
> > > This is for 4.14, I'm not sure if it is needed for other kernels.
> > >
> > > It is not needed in master because a new config item was added,
> > > CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
> > > HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling the
> > > patches that add the required changes, and they make a lot of
> > > changes.  This change is the simple fix, but I'm not sure if we want to
> > > pull all those other changes into 4.14 and whatever other kernels
> > > are required.
> > >
> >
> > I agree that backporting this cleanly is going to be problematic,
> > since it pulls in the entire EL2 VA randomization feature with all its
> > prerequisites.
> >
> > Backporting the following set could be done fairly cleanly, and fixes
> > the problem at build time, but unfortunately, it causes a boot time
> > crash (see below)
> >
> > 97eca4d2bfec (HEAD -> linux-4.14.y) arm64: Move the content of bpi.S
> > to hyp-entry.S
> > 514dd33114c6 arm64: Make BP hardening slot counter available
> > d7ddf3ae9470 arm64; insn: Add encoder for the EXTR instruction
> > c0b2c4e56e10 arm64: KVM: Introduce EL2 VA randomisation
> > 56ab0a87c737 arm64: KVM: Dynamically compute the HYP VA mask
> > d92c02628dfb arm64: KVM: Allow far branches from vector slots to the
> > main vectors
> > 7adec01c9674 arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
> > 1095e4fc3134 arm64: KVM: Move stashing of x0/x1 into the vector code itself
> > bb2e1aceb423 arm64: KVM: Dynamically patch the kernel/hyp VA mask
> > 6f0ccfc451be arm64: KVM: Reserve 4 additional instructions in the BPI template
> > bf425ffee07a arm64: insn: Add encoder for bitwise operations using literals
> > 41dda94d1a9f arm64: insn: Add N immediate encoding
> > 3225668ebe00 arm64: KVM: Move BP hardening vectors into .hyp.text section
> >
> > Marc?
>
> You need at least these:
>
> 1bb32a44aea1 KVM: arm/arm64: Keep GICv2 HYP VAs in kvm_vgic_global_state
> 44a497abd621 KVM: arm/arm64: Do not use kern_hyp_va() with kvm_vgic_global_state
>

Yeah, I only did a fairly mechanical backport based on the actual
diffs depend on each other, but I did spot those two as possibly
related.

> But that's definitely not enough to fix the crash.
>
> >
> > [    0.062126] CPU: All CPU(s) started at EL1
> > [    0.063109] alternatives: patching kernel code
> > [    0.064228] random: get_random_u64 called from
> > compute_layout+0x94/0xe8 with crng_init=0
> > [    0.066313] aarch64_insn_gen_add_sub_imm: invalid immediate encoding 1904640
>
> OK, that one is really bizarre. This value (tag_val) is supposed to be
> a small value (only 12 significant bits out of 24 at any given time),
> and it is not (0x1D1000). So somehow compute_instruction() is not
> doing the right thing.
>
> Do you have a tree somewhere with this patches?
>

Sure, thanks for having a look.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-v4.14-backport%2b%2b

Another thing I found bizarre is that we actually run this code when
all CPUs boot at EL1. Or is that intended?

Marc Zyngier Nov. 29, 2019, 9:04 a.m. UTC | #4

On 2019-11-29 07:25, Ard Biesheuvel wrote:
> On Fri, 29 Nov 2019 at 08:21, Marc Zyngier <[e-mail]> wrote:
>>
>> On Thu, 28 Nov 2019 17:20:20 +0000,
>>
>> [fixing Will's email address]
>>
>> Ard Biesheuvel <[e-mail]> wrote:
>> >
>> > (+ Marc)
>> >
>> >
>> >
>> > On Wed, 27 Nov 2019 at 19:10, <[e-mail]> wrote:
>> > >
>> > > From: Corey Minyard <[e-mail]>
>> > >
>> > > When compiling with KVM enabled and without 
>> HARDEN_BRANCH_PREDICTOR,
>> > > the following compile error happens:
>> > >
>> > > arch/arm64/kernel/cpu_errata.c:92:23:
>> > > error: '__bp_harden_hyp_vecs_start' undeclared (first use in 
>> this function);
>> > > did you mean 'hyp_vecs_start'?
>> > >   void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot * 
>> SZ_2K);
>> > >
>> > > Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
>> > > spectre-v2 vulnerability detection" for 
>> CONFIG_HARDEN_BRANCH_PREDICTOR,
>> > > but __bp_harden_hyp_vecs_start is only defined if that config is
>> > > enabled.
>> > >
>> > > Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has 
>> CONFIG_KVM,
>> > > It looks like you need both of those for that code to be valid.
>> > >
>> > > Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2 
>> vulnerability detection"
>> > > Cc: Andre Przywara <[e-mail]>
>> > > Cc: Catalin Marinas <[e-mail]>
>> > > Cc: Stefan Wahren <[e-mail]>
>> > > Cc: Will Deacon <[e-mail]>
>> > > Cc: Ard Biesheuvel <[e-mail]>
>> > > Signed-off-by: Corey Minyard <[e-mail]>
>> > > ---
>> > > This is for 4.14, I'm not sure if it is needed for other 
>> kernels.
>> > >
>> > > It is not needed in master because a new config item was added,
>> > > CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
>> > > HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling 
>> the
>> > > patches that add the required changes, and they make a lot of
>> > > changes.  This change is the simple fix, but I'm not sure if we 
>> want to
>> > > pull all those other changes into 4.14 and whatever other 
>> kernels
>> > > are required.
>> > >
>> >
>> > I agree that backporting this cleanly is going to be problematic,
>> > since it pulls in the entire EL2 VA randomization feature with all 
>> its
>> > prerequisites.
>> >
>> > Backporting the following set could be done fairly cleanly, and 
>> fixes
>> > the problem at build time, but unfortunately, it causes a boot 
>> time
>> > crash (see below)
>> >
>> > 97eca4d2bfec (HEAD -> linux-4.14.y) arm64: Move the content of 
>> bpi.S
>> > to hyp-entry.S
>> > 514dd33114c6 arm64: Make BP hardening slot counter available
>> > d7ddf3ae9470 arm64; insn: Add encoder for the EXTR instruction
>> > c0b2c4e56e10 arm64: KVM: Introduce EL2 VA randomisation
>> > 56ab0a87c737 arm64: KVM: Dynamically compute the HYP VA mask
>> > d92c02628dfb arm64: KVM: Allow far branches from vector slots to 
>> the
>> > main vectors
>> > 7adec01c9674 arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW 
>> feature flag
>> > 1095e4fc3134 arm64: KVM: Move stashing of x0/x1 into the vector 
>> code itself
>> > bb2e1aceb423 arm64: KVM: Dynamically patch the kernel/hyp VA mask
>> > 6f0ccfc451be arm64: KVM: Reserve 4 additional instructions in the 
>> BPI template
>> > bf425ffee07a arm64: insn: Add encoder for bitwise operations using 
>> literals
>> > 41dda94d1a9f arm64: insn: Add N immediate encoding
>> > 3225668ebe00 arm64: KVM: Move BP hardening vectors into .hyp.text 
>> section
>> >
>> > Marc?
>>
>> You need at least these:
>>
>> 1bb32a44aea1 KVM: arm/arm64: Keep GICv2 HYP VAs in 
>> kvm_vgic_global_state
>> 44a497abd621 KVM: arm/arm64: Do not use kern_hyp_va() with 
>> kvm_vgic_global_state
>>
>
> Yeah, I only did a fairly mechanical backport based on the actual
> diffs depend on each other, but I did spot those two as possibly
> related.
>
>> But that's definitely not enough to fix the crash.
>>
>> >
>> > [    0.062126] CPU: All CPU(s) started at EL1
>> > [    0.063109] alternatives: patching kernel code
>> > [    0.064228] random: get_random_u64 called from
>> > compute_layout+0x94/0xe8 with crng_init=0
>> > [    0.066313] aarch64_insn_gen_add_sub_imm: invalid immediate 
>> encoding 1904640
>>
>> OK, that one is really bizarre. This value (tag_val) is supposed to 
>> be
>> a small value (only 12 significant bits out of 24 at any given 
>> time),
>> and it is not (0x1D1000). So somehow compute_instruction() is not
>> doing the right thing.
>>
>> Do you have a tree somewhere with this patches?
>>
>
> Sure, thanks for having a look.
>
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-v4.14-backport%2b%2b

OK, I tracked it down to this missing patch:

11d764079c9f arm64: insn: Allow ADD/SUB (immediate) with LSL #12

I haven't tryied to boot the whole thing as a host though, only
tested it as a guest.

> Another thing I found bizarre is that we actually run this code when
> all CPUs boot at EL1. Or is that intended?

It is so that I can debug the whole thing in a guest! ;-)

Thanks,

         M.

Ard Biesheuvel Dec. 3, 2019, 6:35 p.m. UTC | #5

On Fri, 29 Nov 2019 at 09:04, Marc Zyngier <[e-mail]> wrote:
>
> On 2019-11-29 07:25, Ard Biesheuvel wrote:
> > On Fri, 29 Nov 2019 at 08:21, Marc Zyngier <[e-mail]> wrote:
> >>
> >> On Thu, 28 Nov 2019 17:20:20 +0000,
> >>
> >> [fixing Will's email address]
> >>
> >> Ard Biesheuvel <[e-mail]> wrote:
> >> >
> >> > (+ Marc)
> >> >
> >> >
> >> >
> >> > On Wed, 27 Nov 2019 at 19:10, <[e-mail]> wrote:
> >> > >
> >> > > From: Corey Minyard <[e-mail]>
> >> > >
> >> > > When compiling with KVM enabled and without
> >> HARDEN_BRANCH_PREDICTOR,
> >> > > the following compile error happens:
> >> > >
> >> > > arch/arm64/kernel/cpu_errata.c:92:23:
> >> > > error: '__bp_harden_hyp_vecs_start' undeclared (first use in
> >> this function);
> >> > > did you mean 'hyp_vecs_start'?
> >> > >   void *dst = lm_alias(__bp_harden_hyp_vecs_start + slot *
> >> SZ_2K);
> >> > >
> >> > > Some ifdefs were removed by 3e91f3eacc91d9 "arm64: Always enable
> >> > > spectre-v2 vulnerability detection" for
> >> CONFIG_HARDEN_BRANCH_PREDICTOR,
> >> > > but __bp_harden_hyp_vecs_start is only defined if that config is
> >> > > enabled.
> >> > >
> >> > > Add CONFIG_HARDEN_BRANCH_PREDICTOR to the #if that has
> >> CONFIG_KVM,
> >> > > It looks like you need both of those for that code to be valid.
> >> > >
> >> > > Fixes: 3e91f3eacc91d9 "arm64: Always enable spectre-v2
> >> vulnerability detection"
> >> > > Cc: Andre Przywara <[e-mail]>
> >> > > Cc: Catalin Marinas <[e-mail]>
> >> > > Cc: Stefan Wahren <[e-mail]>
> >> > > Cc: Will Deacon <[e-mail]>
> >> > > Cc: Ard Biesheuvel <[e-mail]>
> >> > > Signed-off-by: Corey Minyard <[e-mail]>
> >> > > ---
> >> > > This is for 4.14, I'm not sure if it is needed for other
> >> kernels.
> >> > >
> >> > > It is not needed in master because a new config item was added,
> >> > > CONFIG_KVM_INDIRECT_VECTORS, that depends on KVM and
> >> > > HARDEN_BRANCH_PREDICTOR being configured.  I looked at pulling
> >> the
> >> > > patches that add the required changes, and they make a lot of
> >> > > changes.  This change is the simple fix, but I'm not sure if we
> >> want to
> >> > > pull all those other changes into 4.14 and whatever other
> >> kernels
> >> > > are required.
> >> > >
> >> >
> >> > I agree that backporting this cleanly is going to be problematic,
> >> > since it pulls in the entire EL2 VA randomization feature with all
> >> its
> >> > prerequisites.
> >> >
> >> > Backporting the following set could be done fairly cleanly, and
> >> fixes
> >> > the problem at build time, but unfortunately, it causes a boot
> >> time
> >> > crash (see below)
> >> >
> >> > 97eca4d2bfec (HEAD -> linux-4.14.y) arm64: Move the content of
> >> bpi.S
> >> > to hyp-entry.S
> >> > 514dd33114c6 arm64: Make BP hardening slot counter available
> >> > d7ddf3ae9470 arm64; insn: Add encoder for the EXTR instruction
> >> > c0b2c4e56e10 arm64: KVM: Introduce EL2 VA randomisation
> >> > 56ab0a87c737 arm64: KVM: Dynamically compute the HYP VA mask
> >> > d92c02628dfb arm64: KVM: Allow far branches from vector slots to
> >> the
> >> > main vectors
> >> > 7adec01c9674 arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW
> >> feature flag
> >> > 1095e4fc3134 arm64: KVM: Move stashing of x0/x1 into the vector
> >> code itself
> >> > bb2e1aceb423 arm64: KVM: Dynamically patch the kernel/hyp VA mask
> >> > 6f0ccfc451be arm64: KVM: Reserve 4 additional instructions in the
> >> BPI template
> >> > bf425ffee07a arm64: insn: Add encoder for bitwise operations using
> >> literals
> >> > 41dda94d1a9f arm64: insn: Add N immediate encoding
> >> > 3225668ebe00 arm64: KVM: Move BP hardening vectors into .hyp.text
> >> section
> >> >
> >> > Marc?
> >>
> >> You need at least these:
> >>
> >> 1bb32a44aea1 KVM: arm/arm64: Keep GICv2 HYP VAs in
> >> kvm_vgic_global_state
> >> 44a497abd621 KVM: arm/arm64: Do not use kern_hyp_va() with
> >> kvm_vgic_global_state
> >>
> >
> > Yeah, I only did a fairly mechanical backport based on the actual
> > diffs depend on each other, but I did spot those two as possibly
> > related.
> >
> >> But that's definitely not enough to fix the crash.
> >>
> >> >
> >> > [    0.062126] CPU: All CPU(s) started at EL1
> >> > [    0.063109] alternatives: patching kernel code
> >> > [    0.064228] random: get_random_u64 called from
> >> > compute_layout+0x94/0xe8 with crng_init=0
> >> > [    0.066313] aarch64_insn_gen_add_sub_imm: invalid immediate
> >> encoding 1904640
> >>
> >> OK, that one is really bizarre. This value (tag_val) is supposed to
> >> be
> >> a small value (only 12 significant bits out of 24 at any given
> >> time),
> >> and it is not (0x1D1000). So somehow compute_instruction() is not
> >> doing the right thing.
> >>
> >> Do you have a tree somewhere with this patches?
> >>
> >
> > Sure, thanks for having a look.
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-v4.14-backport%2b%2b
>
> OK, I tracked it down to this missing patch:
>
> 11d764079c9f arm64: insn: Allow ADD/SUB (immediate) with LSL #12
>
> I haven't tryied to boot the whole thing as a host though, only
> tested it as a guest.
>
> > Another thing I found bizarre is that we actually run this code when
> > all CPUs boot at EL1. Or is that intended?
>
> It is so that I can debug the whole thing in a guest! ;-)
>

OK, I have prepared a branch that carries all these patches in the
right order with the upstream commit reference added to the commit
log. I'll send this out for review first, before actually sending it
to -stable, unless anyone feels that this is a bad idea.

arm64: KVM: Move BP hardening vectors into .hyp.text section
arm64: insn: Add N immediate encoding
arm64: insn: Add encoder for bitwise operations using literals
arm64: KVM: Dynamically patch the kernel/hyp VA mask
arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
arm64; insn: Add encoder for the EXTR instruction
arm64: insn: Allow ADD/SUB (immediate) with LSL #12
arm64: KVM: Dynamically compute the HYP VA mask
arm64: KVM: Introduce EL2 VA randomisation
arm64: KVM: Move stashing of x0/x1 into the vector code itself
arm64: KVM: Reserve 4 additional instructions in the BPI template
arm64: KVM: Allow far branches from vector slots to the main vectors
arm64: Make BP hardening slot counter available
arm64: Move the content of bpi.S to hyp-entry.S

Marc Zyngier Dec. 3, 2019, 6:45 p.m. UTC | #6

On 2019-12-03 18:35, Ard Biesheuvel wrote:

[...]

> OK, I have prepared a branch that carries all these patches in the
> right order with the upstream commit reference added to the commit
> log. I'll send this out for review first, before actually sending it
> to -stable, unless anyone feels that this is a bad idea.
>
> arm64: KVM: Move BP hardening vectors into .hyp.text section
> arm64: insn: Add N immediate encoding
> arm64: insn: Add encoder for bitwise operations using literals
> arm64: KVM: Dynamically patch the kernel/hyp VA mask
> arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
> arm64; insn: Add encoder for the EXTR instruction
> arm64: insn: Allow ADD/SUB (immediate) with LSL #12
> arm64: KVM: Dynamically compute the HYP VA mask
> arm64: KVM: Introduce EL2 VA randomisation
> arm64: KVM: Move stashing of x0/x1 into the vector code itself
> arm64: KVM: Reserve 4 additional instructions in the BPI template
> arm64: KVM: Allow far branches from vector slots to the main vectors
> arm64: Make BP hardening slot counter available
> arm64: Move the content of bpi.S to hyp-entry.S

My personal take on this is that it is *a lot* of code that affects
a very critical path, and I'm not completely sure that it is what
we want for something as dated as 4.14.

The only valuable reason I can think of (aside of the vulnerability
mitigation aspect) would be if we need to backport more code to
the exception entry path for EL2. I don't know if there is such
need yet.

         M.

Corey Minyard Dec. 3, 2019, 7:16 p.m. UTC | #7

On Tue, Dec 03, 2019 at 06:45:46PM +0000, Marc Zyngier wrote:
> On 2019-12-03 18:35, Ard Biesheuvel wrote:
> 
> [...]
> 
> > OK, I have prepared a branch that carries all these patches in the
> > right order with the upstream commit reference added to the commit
> > log. I'll send this out for review first, before actually sending it
> > to -stable, unless anyone feels that this is a bad idea.
> > 
> > arm64: KVM: Move BP hardening vectors into .hyp.text section
> > arm64: insn: Add N immediate encoding
> > arm64: insn: Add encoder for bitwise operations using literals
> > arm64: KVM: Dynamically patch the kernel/hyp VA mask
> > arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
> > arm64; insn: Add encoder for the EXTR instruction
> > arm64: insn: Allow ADD/SUB (immediate) with LSL #12
> > arm64: KVM: Dynamically compute the HYP VA mask
> > arm64: KVM: Introduce EL2 VA randomisation
> > arm64: KVM: Move stashing of x0/x1 into the vector code itself
> > arm64: KVM: Reserve 4 additional instructions in the BPI template
> > arm64: KVM: Allow far branches from vector slots to the main vectors
> > arm64: Make BP hardening slot counter available
> > arm64: Move the content of bpi.S to hyp-entry.S
> 
> My personal take on this is that it is *a lot* of code that affects
> a very critical path, and I'm not completely sure that it is what
> we want for something as dated as 4.14.

That was my thought, too.  I was just trying to fix a simple compile
error.

> 
> The only valuable reason I can think of (aside of the vulnerability
> mitigation aspect) would be if we need to backport more code to
> the exception entry path for EL2. I don't know if there is such
> need yet.

If that is the case, IMHO this should be backported as part of other
things that require it.

I don't know this code that well, but these changes seem to me more
setup for allowing other things to be done.

-corey

> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...

Ard Biesheuvel Dec. 4, 2019, 3:51 p.m. UTC | #8

On Tue, 3 Dec 2019 at 19:16, Corey Minyard <[e-mail]> wrote:
>
> On Tue, Dec 03, 2019 at 06:45:46PM +0000, Marc Zyngier wrote:
> > On 2019-12-03 18:35, Ard Biesheuvel wrote:
> >
> > [...]
> >
> > > OK, I have prepared a branch that carries all these patches in the
> > > right order with the upstream commit reference added to the commit
> > > log. I'll send this out for review first, before actually sending it
> > > to -stable, unless anyone feels that this is a bad idea.
> > >
> > > arm64: KVM: Move BP hardening vectors into .hyp.text section
> > > arm64: insn: Add N immediate encoding
> > > arm64: insn: Add encoder for bitwise operations using literals
> > > arm64: KVM: Dynamically patch the kernel/hyp VA mask
> > > arm64: cpufeatures: Drop the ARM64_HYP_OFFSET_LOW feature flag
> > > arm64; insn: Add encoder for the EXTR instruction
> > > arm64: insn: Allow ADD/SUB (immediate) with LSL #12
> > > arm64: KVM: Dynamically compute the HYP VA mask
> > > arm64: KVM: Introduce EL2 VA randomisation
> > > arm64: KVM: Move stashing of x0/x1 into the vector code itself
> > > arm64: KVM: Reserve 4 additional instructions in the BPI template
> > > arm64: KVM: Allow far branches from vector slots to the main vectors
> > > arm64: Make BP hardening slot counter available
> > > arm64: Move the content of bpi.S to hyp-entry.S
> >
> > My personal take on this is that it is *a lot* of code that affects
> > a very critical path, and I'm not completely sure that it is what
> > we want for something as dated as 4.14.
>
> That was my thought, too.  I was just trying to fix a simple compile
> error.
>
> >
> > The only valuable reason I can think of (aside of the vulnerability
> > mitigation aspect) would be if we need to backport more code to
> > the exception entry path for EL2. I don't know if there is such
> > need yet.
>
> If that is the case, IMHO this should be backported as part of other
> things that require it.
>
> I don't know this code that well, but these changes seem to me more
> setup for allowing other things to be done.
>

Agreed. But I'd still prefer it if we could backport 4340ba80bd3a
"arm64: KVM: Move BP hardening vectors into .hyp.text section" (which
introduces CONFIG_KVM_INDIRECT_VECTORS and is a useful change by
itself) and only the below hunk taken from 5bea94c013bab "arm64: Move
the content of bpi.S to hyp-entry.S".




diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 66a37cb9a392..67d39c5e6966 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -84,7 +84,7 @@ atomic_t arm64_el2_vector_last_slot = ATOMIC_INIT(-1);

 DEFINE_PER_CPU_READ_MOSTLY(struct bp_hardening_data, bp_hardening_data);

-#ifdef CONFIG_KVM
+#ifdef CONFIG_KVM_INDIRECT_VECTORS
 extern char __smccc_workaround_1_smc_start[];
 extern char __smccc_workaround_1_smc_end[];

@@ -135,7 +135,7 @@ static void install_bp_hardening_cb(bp_hardening_cb_t fn,
 {
 __this_cpu_write(bp_hardening_data.fn, fn);
 }
-#endif /* CONFIG_KVM */
+#endif /* CONFIG_KVM_INDIRECT_VECTORS */

 #include <uapi/linux/psci.h>
 #include <linux/arm-smccc.h>

Patch

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 7d15f4cb6393..e565ec5e072f 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -82,7 +82,7 @@  cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)
 
 DEFINE_PER_CPU_READ_MOSTLY(struct bp_hardening_data, bp_hardening_data);
 
-#ifdef CONFIG_KVM
+#if defined(CONFIG_KVM) && defined(CONFIG_HARDEN_BRANCH_PREDICTOR)
 extern char __smccc_workaround_1_smc_start[];
 extern char __smccc_workaround_1_smc_end[];