Re: [PATCH] kvm/x86/mmu: use the correct inherited permissions to get shadow page

[Paolo Bonzini]

On 26/11/20 01:05, Sean Christopherson wrote:
> On Fri, Nov 20, 2020, Lai Jiangshan wrote:
> > From: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> >
> > Commit 41074d07c78b ("KVM: MMU: Fix inherited permissions for emulated
> > guest pte updates") said role.access is common access permissions for
> > all ptes in this shadow page, which is the inherited permissions from
> > the parent ptes.
> >
> > But the commit did not enforce this definition when kvm_mmu_get_page()
> > is called in FNAME(fetch). Rather, it uses a random (last level pte's
> > combined) access permissions.
>
> I wouldn't say it's random, the issue is specifically that all shadow pages end
> up using the combined set of permissions of the entire walk, as opposed to the
> only combined permissions of its parents.
>
> > And the permissions won't be checked again in next FNAME(fetch) since the
> > spte is present. It might fail to meet guest's expectation when guest sets up
> > spaghetti pagetables.
>
> Can you provide details on the exact failure scenario?  It would be very helpful
> for documentation and understanding.  I can see how using the full combined
> permissions will cause weirdness for upper level SPs in kvm_mmu_get_page(), but
> I'm struggling to connect the dots to understand how that will cause incorrect
> behavior for the guest.  AFAICT, outside of the SP cache, KVM only consumes
> role.access for the final/last SP.

Agreed, a unit test would be even better, but just a description in the 
commit message would be enough.

Paolo