linux-kernel - Re: [PATCH v10 1/9] KVM: x86/mmu: Use GENMASK_ULL() to define __PT_BASE_ADDR

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Aug 2023 12:06:20 +0800
From:   Binbin Wu <binbin.wu@...ux.intel.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, chao.gao@...el.com, kai.huang@...el.com,
        David.Laight@...lab.com, robert.hu@...ux.intel.com,
        guang.zeng@...el.com
Subject: Re: [PATCH v10 1/9] KVM: x86/mmu: Use GENMASK_ULL() to define
 __PT_BASE_ADDR_MASK



On 8/17/2023 5:00 AM, Sean Christopherson wrote:
> On Wed, Jul 19, 2023, Binbin Wu wrote:
>> Use GENMASK_ULL() to define __PT_BASE_ADDR_MASK.
> Using GENMASK_ULL() is an opportunistic cleanup, it is not the main purpose for
> this patch.  The main purpose is to extract the maximum theoretical mask for guest
> MAXPHYADDR so that it can be used to strip bits from CR3.
>
> And rather than bury the actual use in "KVM: x86: Virtualize CR3.LAM_{U48,U57}",
> I think it makes sense to do the masking in this patch.  That change only becomes
> _necessary_ when LAM comes along, but it's completely valid without LAM.
>
> That will also provide a place to explain why we decided to unconditionally mask
> the pgd (it's harmless for 32-bit guests, querying 64-bit mode would be more
> expensive, and for EPT the mask isn't tied to guest mode).
OK.

> And it should also
> explain that using PT_BASE_ADDR_MASK would actually be wrong (PAE has 64-bit
> elements _except_ for CR3).
Hi Sean, I am not sure if I understand it correctly.
Do you mean when KVM shadows the page table of guest using 32-bit paging 
or PAE paging,
guest CR3 is or can be 32 bits for 32-bit paging or PAE paging, so that 
apply the mask to a 32-bit
value CR3 "would actually be wrong" ?


>
> E.g. end up with a shortlog for this patch along the lines of:
>
>    KVM: x86/mmu: Drop non-PA bits when getting GFN for guest's PGD
>
> and write the changelog accordingly.
>
>> No functional change intended.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@...ux.intel.com>
>> ---
>>   arch/x86/kvm/mmu/mmu_internal.h | 1 +
>>   arch/x86/kvm/mmu/paging_tmpl.h  | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
>> index d39af5639ce9..7d2105432d66 100644
>> --- a/arch/x86/kvm/mmu/mmu_internal.h
>> +++ b/arch/x86/kvm/mmu/mmu_internal.h
>> @@ -21,6 +21,7 @@ extern bool dbg;
>>   #endif
>>   
>>   /* Page table builder macros common to shadow (host) PTEs and guest PTEs. */
>> +#define __PT_BASE_ADDR_MASK GENMASK_ULL(51, 12)
>>   #define __PT_LEVEL_SHIFT(level, bits_per_level)	\
>>   	(PAGE_SHIFT + ((level) - 1) * (bits_per_level))
>>   #define __PT_INDEX(address, level, bits_per_level) \
>> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
>> index 0662e0278e70..00c8193f5991 100644
>> --- a/arch/x86/kvm/mmu/paging_tmpl.h
>> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
>> @@ -62,7 +62,7 @@
>>   #endif
>>   
>>   /* Common logic, but per-type values.  These also need to be undefined. */
>> -#define PT_BASE_ADDR_MASK	((pt_element_t)(((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1)))
>> +#define PT_BASE_ADDR_MASK	((pt_element_t)__PT_BASE_ADDR_MASK)
>>   #define PT_LVL_ADDR_MASK(lvl)	__PT_LVL_ADDR_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
>>   #define PT_LVL_OFFSET_MASK(lvl)	__PT_LVL_OFFSET_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
>>   #define PT_INDEX(addr, lvl)	__PT_INDEX(addr, lvl, PT_LEVEL_BITS)
>> -- 
>> 2.25.1
>>