Re: [PATCH v6 22/43] KVM: arm64: Validate register access for a Realm VM

[Steven Price <steven.price@arm.com>]

On 02/02/2025 01:22, Gavin Shan wrote:
> On 12/13/24 1:55 AM, Steven Price wrote:
>> The RMM only allows setting the GPRS (x0-x30) and PC for a realm
>> guest. Check this in kvm_arm_set_reg() so that the VMM can receive a
>> suitable error return if other registers are accessed.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v5:
>>   * Upper GPRS can be set as part of a HOST_CALL return, so fix up the
>>     test to allow them.
>> ---
>>   arch/arm64/kvm/guest.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>> index 12dad841f2a5..1ee2fe072f1a 100644
>> --- a/arch/arm64/kvm/guest.c
>> +++ b/arch/arm64/kvm/guest.c
>> @@ -73,6 +73,24 @@ static u64 core_reg_offset_from_id(u64 id)
>>       return id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK |
>> KVM_REG_ARM_CORE);
>>   }
>>   +static bool kvm_realm_validate_core_reg(u64 off)
>> +{
>> +    /*
>> +     * Note that GPRs can only sometimes be controlled by the VMM.
>> +     * For PSCI only X0-X6 are used, higher registers are ignored
>> (restored
>> +     * from the REC).
>> +     * For HOST_CALL all of X0-X30 are copied to the RsiHostCall
>> structure.
>> +     * For emulated MMIO X0 is always used.
>> +     */
>> +    switch (off) {
>> +    case KVM_REG_ARM_CORE_REG(regs.regs[0]) ...
>> +         KVM_REG_ARM_CORE_REG(regs.regs[30]):
>> +    case KVM_REG_ARM_CORE_REG(regs.pc):
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>>   static int core_reg_size_from_offset(const struct kvm_vcpu *vcpu,
>> u64 off)
>>   {
>>       int size;
>> @@ -115,6 +133,9 @@ static int core_reg_size_from_offset(const struct
>> kvm_vcpu *vcpu, u64 off)
>>       if (vcpu_has_sve(vcpu) && core_reg_offset_is_vreg(off))
>>           return -EINVAL;
>>   +    if (kvm_is_realm(vcpu->kvm) && !kvm_realm_validate_core_reg(off))
>> +        return -EPERM;
>> +
>>       return size;
>>   }
>>   @@ -783,12 +804,34 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu,
>> const struct kvm_one_reg *reg)
>>       return kvm_arm_sys_reg_get_reg(vcpu, reg);
>>   }
>>   +/*
>> + * The RMI ABI only enables setting some GPRs and PC. The selection
>> of GPRs
>> + * that are available depends on the Realm state and the reason for
>> the last
>> + * exit.  All other registers are reset to architectural or otherwise
>> defined
>> + * reset values by the RMM, except for a few configuration fields that
>> + * correspond to Realm parameters.
>> + */
>> +static bool validate_realm_set_reg(struct kvm_vcpu *vcpu,
>> +                   const struct kvm_one_reg *reg)
>> +{
>> +    if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE) {
>> +        u64 off = core_reg_offset_from_id(reg->id);
>> +
>> +        return kvm_realm_validate_core_reg(off);
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg
>> *reg)
>>   {
>>       /* We currently use nothing arch-specific in upper 32 bits */
>>       if ((reg->id & ~KVM_REG_SIZE_MASK) >> 32 != KVM_REG_ARM64 >> 32)
>>           return -EINVAL;
>>   +    if (kvm_is_realm(vcpu->kvm) && !validate_realm_set_reg(vcpu, reg))
>> +        return -EINVAL;
>> +
>>       switch (reg->id & KVM_REG_ARM_COPROC_MASK) {
>>       case KVM_REG_ARM_CORE:    return set_core_reg(vcpu, reg);
>>       case KVM_REG_ARM_FW:
> 
> It looks the core registers in kvm_arm_set_reg() has been validated for
> twice.
> 
>   ioctl(KVM_SET_ONE_REG)
>     kvm_arm_set_reg
>       validate_realm_set_reg
>         kvm_realm_validate_core_reg        // 1
>       set_core_reg
>         core_reg_offset_from_id
>         core_reg_addr
>           core_reg_offset_from_id
>           core_reg_size_from_offset
>             kvm_realm_validate_core_reg    // 2
>         copy_from_user

Indeed, it looks like the check in core_reg_size_from_offset() is
unnecessary.

> Besides, there are other types of registers that can be accessed by
> KVM_{GET, SET}_ONE_REG:
> firmware and bitmap registers, SVE registers, timer registers, system
> registers. Need we
> to hide any of them from the user space? As I can understand, the SVE
> registers are owned
> by RMM at least and won't be exposed to user space.

validate_realm_set_reg() only allows core registers permitted by
kvm_realm_validate_core_reg() (x0-x30+pc) and the three other
specifically mentioned registers (PMCR_EL0, ID_AA64DFR0_EL1 and the
pseudo register SVE_VLS).

In general most registers are hidden from the host (and user space),
even the general purpose registers are in most contexts stale. The only
real exception is during realm creation and PSCI. The registers are also
(ab)used during MMIO and HOST_CALL. For MMIO we use x0 for the value to
be read/written (even if the guest used a different register). For
HOST_CALL the guest provides a structure containing the values to be
placed in the GPRS (but the actual values of the guest's registers are
not exposed).

Steve