| 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: <a2ef5302-d27d-945e-3c67-0bc9855fc4a6@linux.intel.com>
Date: Thu, 11 May 2023 17:18:31 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Chao Gao <chao.gao@...el.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
seanjc@...gle.com, pbonzini@...hat.com, kai.huang@...el.com,
robert.hu@...ux.intel.com
Subject: Re: [PATCH v8 4/6] KVM: x86: Introduce untag_addr() in kvm_x86_ops
On 5/11/2023 2:03 PM, Chao Gao wrote:
> On Wed, May 10, 2023 at 02:06:09PM +0800, Binbin Wu wrote:
>> Introduce a new optional interface untag_addr() to kvm_x86_ops to untag
>> the metadata from linear address. Implement LAM version in VMX.
>>
>> When enabled feature like Intel Linear Address Masking or AMD Upper
>> Address Ignore, linear address may be tagged with metadata. Linear
>> address should be checked for modified canonicality and untagged in
>> instruction emulations or VMExit handlers if LAM or UAI is applicable.
>>
>> Introduce untag_addr() to kvm_x86_ops to hide the vendor specific code.
>> Pass the 'flags' to avoid distinguishing processor vendor in common emulator
>> path for the cases whose untag policies are different in the future.
>>
>
>> For VMX, LAM version is implemented.
>> LAM has a modified canonical check when applicable:
>> * LAM_S48 : [ 1 ][ metadata ][ 1 ]
>> 63 47
>> * LAM_U48 : [ 0 ][ metadata ][ 0 ]
>> 63 47
>> * LAM_S57 : [ 1 ][ metadata ][ 1 ]
>> 63 56
>> * LAM_U57 + 5-lvl paging : [ 0 ][ metadata ][ 0 ]
>> 63 56
>> * LAM_U57 + 4-lvl paging : [ 0 ][ metadata ][ 0...0 ]
>> 63 56..47
>> LAM identification of an address as user or supervisor is based solely on
>> the value of pointer bit 63.
>> If LAM is applicable to certain address, untag the metadata bits by
>> sign-extending the value of bit 47 (LAM48) or bit 56 (LAM57). Later
>> the untagged address will do legacy canonical check. So that LAM canonical
>> check and mask can be covered by "untag + legacy canonical check".
> Do you think it is better to remove such details from the changelog and
> instead document them in comments e.g.,
>
>> Signed-off-by: Binbin Wu <binbin.wu@...ux.intel.com>
>> Tested-by: Xuelian Guo <xuelian.guo@...el.com>
>> ---
>> arch/x86/include/asm/kvm-x86-ops.h | 1 +
>> arch/x86/include/asm/kvm_host.h | 2 ++
>> arch/x86/kvm/kvm_emulate.h | 1 +
>> arch/x86/kvm/vmx/vmx.c | 50 ++++++++++++++++++++++++++++++
>> arch/x86/kvm/vmx/vmx.h | 2 ++
>> 5 files changed, 56 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
>> index 13bc212cd4bc..c0cebe671d41 100644
>> --- a/arch/x86/include/asm/kvm-x86-ops.h
>> +++ b/arch/x86/include/asm/kvm-x86-ops.h
>> @@ -52,6 +52,7 @@ KVM_X86_OP(cache_reg)
>> KVM_X86_OP(get_rflags)
>> KVM_X86_OP(set_rflags)
>> KVM_X86_OP(get_if_flag)
>> +KVM_X86_OP_OPTIONAL(untag_addr)
>> KVM_X86_OP(flush_tlb_all)
>> KVM_X86_OP(flush_tlb_current)
>> KVM_X86_OP_OPTIONAL(flush_remote_tlbs)
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 46471dd9cc1b..bfccc0e19bf2 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1588,6 +1588,8 @@ struct kvm_x86_ops {
>> void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
>> bool (*get_if_flag)(struct kvm_vcpu *vcpu);
>>
>> + void (*untag_addr)(struct kvm_vcpu *vcpu, u64 *la, u32 flags);
>> +
>> void (*flush_tlb_all)(struct kvm_vcpu *vcpu);
>> void (*flush_tlb_current)(struct kvm_vcpu *vcpu);
>> int (*flush_remote_tlbs)(struct kvm *kvm);
>> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
>> index 5b9ec610b2cb..c2091e24a6b9 100644
>> --- a/arch/x86/kvm/kvm_emulate.h
>> +++ b/arch/x86/kvm/kvm_emulate.h
>> @@ -91,6 +91,7 @@ struct x86_instruction_info {
>> /* x86-specific emulation flags */
>> #define X86EMUL_F_FETCH BIT(0)
>> #define X86EMUL_F_WRITE BIT(1)
>> +#define X86EMUL_F_SKIPLAM BIT(2)
>>
>> struct x86_emulate_ops {
>> void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 9c80048ca30c..b52e44758a4e 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -8134,6 +8134,54 @@ static void vmx_vm_destroy(struct kvm *kvm)
>> free_pages((unsigned long)kvm_vmx->pid_table, vmx_get_pid_table_order(kvm));
>> }
>>
>> +#define LAM_S57_EN_MASK (X86_CR4_LAM_SUP | X86_CR4_LA57)
>> +static int lam_sign_extend_bit(struct kvm_vcpu *vcpu, bool user)
> we can let this function take an address ...
>
>> +{
>> + u64 cr3, cr4;
>> +
> then it is natural to add the comment -- "LAM identification ...", like
>
> /*
> * The LAM identification of a pointer as user or supervisor is
> * based solely on the value of pointer bit 63.
> */
> if (!(addr >> 63)) {
OK.
>
>> + if (user) {
>> + cr3 = kvm_read_cr3(vcpu);
>> + if (cr3 & X86_CR3_LAM_U57)
>> + return 56;
>> + if (cr3 & X86_CR3_LAM_U48)
>> + return 47;
>> + } else {
>> + cr4 = kvm_read_cr4_bits(vcpu, LAM_S57_EN_MASK);
>> + if (cr4 == LAM_S57_EN_MASK)
>> + return 56;
>> + if (cr4 & X86_CR4_LAM_SUP)
>> + return 47;
>> + }
>> + return -1;
>> +}
>> +
>> +/*
>> + * Only called in 64-bit mode.
>> + *
>> + * Metadata bits are [62:48] in LAM48 and [62:57] in LAM57. Mask metadata in
>> + * pointers by sign-extending the value of bit 47 (LAM48) or 56 (LAM57).
>> + * The resulting address after untagging isn't guaranteed to be canonical.
>> + * Callers should perform the original canonical check and raise #GP/#SS if the
>> + * address is non-canonical.
> To document the difference between KVM's emulation of LAM and real hardware
> behavior:
>
> *
> * Note that KVM masks the metadata in addresses, performs the (original)
> * canonicality checking and then walks page table. This is slightly
> * different from hardware behavior but achieves the same effect.
> * Specifically, if LAM is enabled, the processor performs a modified
> * canonicality checking where the metadata are ignored instead of
> * masked. After the modified canonicality checking, the processor masks
> * the metadata before passing addresses for paging translation.
> */
OK.
>
>> + */
>> +void vmx_untag_addr(struct kvm_vcpu *vcpu, u64 *la, u32 flags)
> ^
> nit: "gla" or "gva" is slightly better.
> It may be hard to immediately
> connect "la" with "linear address".
>
> or even better, use "gva_t gva"?
Will update it to use "gva_t gva", thanks.
>> +{
>> + int sign_ext_bit;
>> +
>> + /*
>> + * Check LAM_U48 in cr3_ctrl_bits to avoid guest_cpuid_has().
>> + * If not set, vCPU doesn't supports LAM.
>> + */
>> + if (!(vcpu->arch.cr3_ctrl_bits & X86_CR3_LAM_U48) ||
>> + (flags & X86EMUL_F_SKIPLAM) || WARN_ON_ONCE(!is_64_bit_mode(vcpu)))
>> + return;
>> +
>> + sign_ext_bit = lam_sign_extend_bit(vcpu, !(*la >> 63));
>> + if (sign_ext_bit > 0)
>> + *la = (sign_extend64(*la, sign_ext_bit) & ~BIT_ULL(63)) |
>> + (*la & BIT_ULL(63));
> nit: curly braces are needed.
Even if it's only one statement (although splited to two lines), curly
braces are needed?
>
> Overall the patch looks good to me, so
>
> Reviewed-by: Chao Gao <chao.gao@...el.com>
>
> (regardless of whether you choose to make the suggested changes to the
> changelog/comments)
>
>> +}
>> +
>> static struct kvm_x86_ops vmx_x86_ops __initdata = {
>> .name = KBUILD_MODNAME,
>>
>> @@ -8182,6 +8230,8 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>> .set_rflags = vmx_set_rflags,
>> .get_if_flag = vmx_get_if_flag,
>>
>> + .untag_addr = vmx_untag_addr,
>> +
>> .flush_tlb_all = vmx_flush_tlb_all,
>> .flush_tlb_current = vmx_flush_tlb_current,
>> .flush_tlb_gva = vmx_flush_tlb_gva,
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index 9e66531861cf..e1e6d2e03b61 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -433,6 +433,8 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type);
>> u64 vmx_get_l2_tsc_offset(struct kvm_vcpu *vcpu);
>> u64 vmx_get_l2_tsc_multiplier(struct kvm_vcpu *vcpu);
>>
>> +void vmx_untag_addr(struct kvm_vcpu *vcpu, u64 *la, u32 flags);
>> +
>> static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
>> int type, bool value)
>> {
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists