Re: [PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

D Scott Phillips
2025-04-16T23:42:14Z

Marc Zyngier <maz@kernel.org> writes:

> On Tue, 15 Apr 2025 16:46:55 +0100,
> D Scott Phillips <scott@os.amperecomputing.com> wrote:
>>
>> In the skip_mmu_switch case, config.hcr was used uninitialized. On my
>> machine that caused garbage to be written to HCR_EL2 and then the CPU
>> got stuck at the synchronous exception handler. Also, the restore of
>> HCR_EL2 was missing at the end of the function in the same case.
>
> Huh, how embarrassing. Thanks for spotting this one.
>
>>
>> In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.
>>
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> ---
>> arch/arm64/kvm/at.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
>> index f74a66ce3064b..ff4b06ce661af 100644
>> --- a/arch/arm64/kvm/at.c
>> +++ b/arch/arm64/kvm/at.c
>> @@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>> * the right one (as we trapped from vEL2). If not, save the
>> * full MMU context.
>> */
>> - if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
>> + if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
>> + config.hcr = read_sysreg(hcr_el2);
>> goto skip_mmu_switch;
>> + }
>>
>> /*
>> * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
>> @@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>> if (!fail)
>> par = read_sysreg_par();
>>
>> - if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
>> + if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
>> + write_sysreg(config.hcr, hcr_el2);
>> + else
>> __mmu_config_restore(&config);
>>
>> return par;
>
> I think the diff below should do the trick (and incidently matches
> your commit message).

Looks good Marc, thanks

Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>

>
> Thanks,
>
> M.
>
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index f74a66ce3064b..773e3b4d5c7e5 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1214,7 +1214,7 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> */
> static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> {
> - struct mmu_config config;
> + struct mmu_config config = { .hcr = HCR_HOST_VHE_FLAGS, };
> struct kvm_s2_mmu *mmu;
> bool fail;
> u64 par;
> @@ -1301,6 +1301,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>
> if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
> __mmu_config_restore(&config);
> + else
> + write_sysreg(config.hcr, hcr_el2);
>
> return par;
> }
>
> --
> Without deviation from the norm, progress is not possible.