| Message ID | 20220214065746.1230608-10-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On Sun, Feb 13, 2022 at 10:57:28PM -0800, Reiji Watanabe wrote: > This patch adds id_reg_info for ID_AA64MMFR1_EL1 to make it > writable by userspace. > > Hardware update of Access flag and/or Dirty state in translation > table needs to be disabled for the guest to let userspace set > ID_AA64MMFR1_EL1.HAFDBS field to a lower value. It requires trapping > the guest's accessing TCR_EL1, which KVM doesn't always do (in order > to trap it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which > also traps many other virtual memory control registers). > So, userspace won't be allowed to modify the value of the HAFDBS field. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/kvm/sys_regs.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4ed15ae7f160..1c137f8c236f 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -570,6 +570,30 @@ static int validate_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu, > return 0; > } > > +static int validate_id_aa64mmfr1_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int hafdbs, lim_hafdbs; > + > + hafdbs = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_HADBS_SHIFT); > + lim_hafdbs = cpuid_feature_extract_unsigned_field(limit, ID_AA64MMFR1_HADBS_SHIFT); > + > + /* > + * Don't allow userspace to modify the value of HAFDBS. > + * Hardware update of Access flag and/or Dirty state in translation > + * table needs to be disabled for the guest to let userspace set > + * HAFDBS field to a lower value. It requires trapping the guest's > + * accessing TCR_EL1, which KVM doesn't always do (in order to trap > + * it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which also > + * traps many other virtual memory control registers). > + */ > + if (hafdbs != lim_hafdbs) > + return -EINVAL; Are we going to require that any hidden feature be trappable going forward? It doesn't seem to me like there's much risk to userspace hiding any arbitrary feature so long as identity remains architectural. Another example of this is AArch32 at EL0. Without FGT, there is no precise trap for KVM to intervene and prevent an AArch32 EL0. Nonetheless, userspace might still want to hide this from its guests even if a misbehaved guest could still use it. -- Thanks, Oliver
Hi Oliver, On Tue, Feb 15, 2022 at 10:53 AM Oliver Upton <oupton@google.com> wrote: > > Hi Reiji, > > On Sun, Feb 13, 2022 at 10:57:28PM -0800, Reiji Watanabe wrote: > > This patch adds id_reg_info for ID_AA64MMFR1_EL1 to make it > > writable by userspace. > > > > Hardware update of Access flag and/or Dirty state in translation > > table needs to be disabled for the guest to let userspace set > > ID_AA64MMFR1_EL1.HAFDBS field to a lower value. It requires trapping > > the guest's accessing TCR_EL1, which KVM doesn't always do (in order > > to trap it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which > > also traps many other virtual memory control registers). > > So, userspace won't be allowed to modify the value of the HAFDBS field. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 4ed15ae7f160..1c137f8c236f 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -570,6 +570,30 @@ static int validate_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu, > > return 0; > > } > > > > +static int validate_id_aa64mmfr1_el1(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *id_reg, u64 val) > > +{ > > + u64 limit = id_reg->vcpu_limit_val; > > + unsigned int hafdbs, lim_hafdbs; > > + > > + hafdbs = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_HADBS_SHIFT); > > + lim_hafdbs = cpuid_feature_extract_unsigned_field(limit, ID_AA64MMFR1_HADBS_SHIFT); > > + > > + /* > > + * Don't allow userspace to modify the value of HAFDBS. > > + * Hardware update of Access flag and/or Dirty state in translation > > + * table needs to be disabled for the guest to let userspace set > > + * HAFDBS field to a lower value. It requires trapping the guest's > > + * accessing TCR_EL1, which KVM doesn't always do (in order to trap > > + * it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which also > > + * traps many other virtual memory control registers). > > + */ > > + if (hafdbs != lim_hafdbs) > > + return -EINVAL; > > Are we going to require that any hidden feature be trappable going > forward? It doesn't seem to me like there's much risk to userspace > hiding any arbitrary feature so long as identity remains architectural. That wasn't my intention, and I will drop this patch. (I wonder why I thought this was needed...) Thank you for catching this ! Regards, Reiji > > Another example of this is AArch32 at EL0. Without FGT, there is no > precise trap for KVM to intervene and prevent an AArch32 EL0. > Nonetheless, userspace might still want to hide this from its guests > even if a misbehaved guest could still use it. > > -- > Thanks, > Oliver
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4ed15ae7f160..1c137f8c236f 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -570,6 +570,30 @@ static int validate_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu, return 0; } +static int validate_id_aa64mmfr1_el1(struct kvm_vcpu *vcpu, + const struct id_reg_info *id_reg, u64 val) +{ + u64 limit = id_reg->vcpu_limit_val; + unsigned int hafdbs, lim_hafdbs; + + hafdbs = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_HADBS_SHIFT); + lim_hafdbs = cpuid_feature_extract_unsigned_field(limit, ID_AA64MMFR1_HADBS_SHIFT); + + /* + * Don't allow userspace to modify the value of HAFDBS. + * Hardware update of Access flag and/or Dirty state in translation + * table needs to be disabled for the guest to let userspace set + * HAFDBS field to a lower value. It requires trapping the guest's + * accessing TCR_EL1, which KVM doesn't always do (in order to trap + * it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which also + * traps many other virtual memory control registers). + */ + if (hafdbs != lim_hafdbs) + return -EINVAL; + + return 0; +} + static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) { u64 limit = id_reg->vcpu_limit_val; @@ -675,6 +699,11 @@ static struct id_reg_info id_aa64mmfr0_el1_info = { .validate = validate_id_aa64mmfr0_el1, }; +static struct id_reg_info id_aa64mmfr1_el1_info = { + .sys_reg = SYS_ID_AA64MMFR1_EL1, + .validate = validate_id_aa64mmfr1_el1, +}; + /* * An ID register that needs special handling to control the value for the * guest must have its own id_reg_info in id_reg_info_table. @@ -689,6 +718,7 @@ static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { [IDREG_IDX(SYS_ID_AA64ISAR0_EL1)] = &id_aa64isar0_el1_info, [IDREG_IDX(SYS_ID_AA64ISAR1_EL1)] = &id_aa64isar1_el1_info, [IDREG_IDX(SYS_ID_AA64MMFR0_EL1)] = &id_aa64mmfr0_el1_info, + [IDREG_IDX(SYS_ID_AA64MMFR1_EL1)] = &id_aa64mmfr1_el1_info, }; static int validate_id_reg(struct kvm_vcpu *vcpu, u32 id, u64 val)
This patch adds id_reg_info for ID_AA64MMFR1_EL1 to make it writable by userspace. Hardware update of Access flag and/or Dirty state in translation table needs to be disabled for the guest to let userspace set ID_AA64MMFR1_EL1.HAFDBS field to a lower value. It requires trapping the guest's accessing TCR_EL1, which KVM doesn't always do (in order to trap it without FEAT_FGT, HCR_EL2.TVM needs to be set to 1, which also traps many other virtual memory control registers). So, userspace won't be allowed to modify the value of the HAFDBS field. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/kvm/sys_regs.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)