| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8b617aef-2bff-8af0-df47-f9f863ab6fa0@arm.com>
Date: Fri, 2 Oct 2020 16:30:47 +0100
From: Steven Price <steven.price@....com>
To: Andrew Jones <drjones@...hat.com>
Cc: Catalin Marinas <catalin.marinas@....com>,
Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Peter Maydell <peter.maydell@...aro.org>,
"Dr. David Alan Gilbert" <dgilbert@...hat.com>,
Haibo Xu <Haibo.Xu@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
qemu-devel@...gnu.org, Dave Martin <Dave.Martin@....com>,
Juan Quintela <quintela@...hat.com>,
Richard Henderson <richard.henderson@...aro.org>,
linux-kernel@...r.kernel.org, James Morse <james.morse@....com>,
Julien Thierry <julien.thierry.kdev@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
kvmarm@...ts.cs.columbia.edu, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/2] arm64: kvm: Introduce MTE VCPU feature
On 02/10/2020 15:30, Andrew Jones wrote:
> On Fri, Sep 25, 2020 at 10:36:07AM +0100, Steven Price wrote:
>> Add a new VM feature 'KVM_ARM_CAP_MTE' which enables memory tagging
>> for a VM. This exposes the feature to the guest and automatically tags
>> memory pages touched by the VM as PG_mte_tagged (and clears the tags
>> storage) to ensure that the guest cannot see stale tags, and so that the
>> tags are correctly saved/restored across swap.
>>
>> Signed-off-by: Steven Price <steven.price@....com>
>> ---
>> arch/arm64/include/asm/kvm_emulate.h | 3 +++
>> arch/arm64/include/asm/kvm_host.h | 3 +++
>> arch/arm64/kvm/arm.c | 9 +++++++++
>> arch/arm64/kvm/mmu.c | 15 +++++++++++++++
>> arch/arm64/kvm/sys_regs.c | 6 +++++-
>> include/uapi/linux/kvm.h | 1 +
>> 6 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 49a55be2b9a2..4923a566ae6e 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -79,6 +79,9 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>> if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
>> vcpu_el1_is_32bit(vcpu))
>> vcpu->arch.hcr_el2 |= HCR_TID2;
>> +
>> + if (vcpu->kvm->arch.mte_enabled)
>> + vcpu->arch.hcr_el2 |= HCR_ATA;
>> }
>>
>> static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 4f4360dd149e..1379300c1487 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -110,6 +110,9 @@ struct kvm_arch {
>> * supported.
>> */
>> bool return_nisv_io_abort_to_user;
>> +
>> + /* Memory Tagging Extension enabled for the guest */
>> + bool mte_enabled;
>> };
>>
>> struct kvm_vcpu_fault_info {
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 46dc3d75cf13..624edca0a1fa 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -87,6 +87,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>> r = 0;
>> kvm->arch.return_nisv_io_abort_to_user = true;
>> break;
>> + case KVM_CAP_ARM_MTE:
>> + if (!system_supports_mte() || kvm->created_vcpus)
>> + return -EINVAL;
>> + r = 0;
>> + kvm->arch.mte_enabled = true;
>> + break;
>> default:
>> r = -EINVAL;
>> break;
>> @@ -206,6 +212,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> */
>> r = 1;
>> break;
>> + case KVM_CAP_ARM_MTE:
>> + r = system_supports_mte();
>> + break;
>> default:
>> r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
>> break;
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index ba00bcc0c884..befb9e1f0aa6 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1949,6 +1949,21 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> if (vma_pagesize == PAGE_SIZE && !force_pte)
>> vma_pagesize = transparent_hugepage_adjust(memslot, hva,
>> &pfn, &fault_ipa);
>> + if (system_supports_mte() && kvm->arch.mte_enabled && pfn_valid(pfn)) {
>
> 'system_supports_mte() && kvm->arch.mte_enabled' is redundant, but I
> assume system_supports_mte() is there to improve the efficiency of the
> branch, as it's using cpus_have_const_cap().
system_supports_mte() compiles to 0 when MTE support isn't built in, so
this code can be removed by the compiler, whereas with
kvm->arch.mte_enabled I doubt the compiler can deduce that it is never set.
> Maybe a helper like
>
> static inline bool kvm_arm_mte_enabled(struct kvm *kvm)
> {
> return system_supports_mte() && kvm->arch.mte_enabled;
> }
>
> would allow both the more efficient branch and look less confusing
> where it gets used.
I wasn't sure it was worth having a helper since this was the only place
checking this condition. It's also a bit tricky putting this in a
logical header file, kvm_host.h doesn't work because struct kvm hasn't
been defined by then.
Steve
>> + /*
>> + * VM will be able to see the page's tags, so we must ensure
>> + * they have been initialised.
>> + */
>> + struct page *page = pfn_to_page(pfn);
>> + long i, nr_pages = compound_nr(page);
>> +
>> + /* if PG_mte_tagged is set, tags have already been initialised */
>> + for (i = 0; i < nr_pages; i++, page++) {
>> + if (!test_and_set_bit(PG_mte_tagged, &page->flags))
>> + mte_clear_page_tags(page_address(page));
>> + }
>> + }
>> +
>> if (writable)
>> kvm_set_pfn_dirty(pfn);
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index a655f172b5ad..5010a47152b4 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1132,7 +1132,8 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>> val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>> val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
>> } else if (id == SYS_ID_AA64PFR1_EL1) {
>> - val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
>> + if (!vcpu->kvm->arch.mte_enabled)
>> + val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
>> } else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
>> val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
>> (0xfUL << ID_AA64ISAR1_API_SHIFT) |
>> @@ -1394,6 +1395,9 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>> const struct sys_reg_desc *rd)
>> {
>> + if (vcpu->kvm->arch.mte_enabled)
>> + return 0;
>> +
>> return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>> }
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index f6d86033c4fa..87678ed82ab4 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
>> #define KVM_CAP_LAST_CPU 184
>> #define KVM_CAP_SMALLER_MAXPHYADDR 185
>> #define KVM_CAP_S390_DIAG318 186
>> +#define KVM_CAP_ARM_MTE 188
>>
>> #ifdef KVM_CAP_IRQ_ROUTING
>>
>> --
>> 2.20.1
>>
>>
>
> Besides the helper suggestion nit
>
> Reviewed-by: Andrew Jones <drjones@...hat.com>
>
Powered by blists - more mailing lists