linux-kernel - Re: [PATCH v2 08/25] KVM: x86/mmu: split cpu_mode from mmu

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:   Tue, 8 Mar 2022 18:49:09 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        dmatlack@...gle.com
Subject: Re: [PATCH v2 08/25] KVM: x86/mmu: split cpu_mode from mmu_role

On 3/8/22 18:36, Sean Christopherson wrote:
> The idea was to trigger fireworks due to a incoherent state (e.g. direct mmu_role with
> non-direct hooks) if the nested_mmu was ever used as a "real" MMU (handling faults,
> installing SPs/SPTEs, etc...).  For a walk-only MMU, "direct" has no meaning and so
> rather than arbitrarily leave it '0', I arbitrarily set it '1'.
> 
> Maybe this?
> 
>    The nested MMU now has only the CPU mode; and in fact the new function
>    kvm_calc_cpu_mode is analogous to the previous kvm_calc_nested_mmu_role,
>    except that it has role.base.direct equal to CR0.PG.  Having "direct"
>    track CR0.PG has the serendipitious side effect of being an even better
>    sentinel than arbitrarily setting direct to true for the nested MMU, as
>    KVM will run afoul of sanity checks for both direct and indirect MMUs if
>    KVM attempts to use the nested MMU as a "real" MMU, e.g. for page faults.

Hmm, actually it is set to CR0.PG *negated*, so that future patches can 
get rid of role.ext.cr0_pg.  But really anybody trying to use nested_mmu 
for real would get NULL pointer dereferences left and right in all 
likelihood.  This will be even clearer by the end of the series, when 
the function pointers are initialized at vCPU creation time.

>>
>> +	role.base.direct = !____is_cr0_pg(regs);
>> +	if (!role.base.direct) {
> 
> Can we check ____is_cr0_pg() instead of "direct"?  IMO that's more intuitive for
> understanding why the bits below are left zero.  I was scratching my head trying
> to figure out whether or not this was safe/correct for direct MMUs...

Yes, that's good.

Paolo