linux-kernel - Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

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]

Message-ID: <2b61fec12a3be255327d9e16678c40608ec538f8.camel@intel.com>
Date:   Mon, 20 Mar 2023 21:34:28 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Gross, Jurgen" <jgross@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
CC:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for
 software defined MTRRs

On Mon, 2023-03-20 at 14:47 +0100, Juergen Gross wrote:
> On 20.03.23 13:59, Huang, Kai wrote:
> > On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
> > > When running virtualized, MTRR access can be reduced (e.g. in Xen PV
> > > guests or when running as a SEV-SNP guest under Hyper-V). Typically
> > > the hypervisor will reset the MTRR feature in CPUID data, resulting
> > > in no MTRR memory type information being available for the kernel.
> > > 
> > > This has turned out to result in problems:
> > > 
> > > - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
> > > - Xen PV dom0 mapping memory as WB which should be UC- instead
> > > 
> > > Solve those problems by supporting to set a static MTRR state,
> > > overwriting the empty state used today. In case such a state has been
> > > set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
> > > will only be used by mtrr_type_lookup(), as in all other cases
> > > mtrr_enabled() is being checked, which will return false. Accept the
> > > overwrite call only for selected cases when running as a guest.
> > > Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
> > > just refusing them.
> > > 
> > > 
> > [...]
> > 
> > > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > > index ee09d359e08f..49b4cc923312 100644
> > > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > > @@ -8,10 +8,12 @@
> > >   #include <linux/init.h>
> > >   #include <linux/io.h>
> > >   #include <linux/mm.h>
> > > -
> > > +#include <linux/cc_platform.h>
> > >   #include <asm/processor-flags.h>
> > >   #include <asm/cacheinfo.h>
> > >   #include <asm/cpufeature.h>
> > > +#include <asm/hypervisor.h>
> > > +#include <asm/mshyperv.h>
> > 
> > Is <asm/mshyperv.h> needed here?
> 
> Yes, for hv_is_isolation_supported().
> 
> > 
> > >   #include <asm/tlbflush.h>
> > >   #include <asm/mtrr.h>
> > >   #include <asm/msr.h>
> > > @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > >   	return mtrr_state.def_type;
> > >   }
> > >   
> > > +/**
> > > + * mtrr_overwrite_state - set static MTRR state
> > > + *
> > > + * Used to set MTRR state via different means (e.g. with data obtained from
> > > + * a hypervisor).
> > 
> > +KVM list and KVM maintainers,
> > 
> > IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
> > hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
> > because they have mutual understanding?
> > 
> > What about the SNP guest running on other hypervisors such as KVM?
> > 
> > Since this code covers TDX guest too, I think eventually it makes sense for TDX
> > guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> > think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> > as I am not sure what's the point of making the TDX guest's MTRR behaviour
> > depending on specific hypervisor.
> 
> This series tries to fix the current fallout.
> 
> Boris Petkov asked for the hypervisor specific tests to be added, so I've
> added them after discussing the topic with him (he is the maintainer of
> this code after all).
> 
> > For now I don't see there's any use case for TDX guest to use non-WB memory type
> > (in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
> > the guest memory), so to me it seems it's OK to make a universal mutual
> > understanding that TDX guest will always have WB memory type for all memory.
> 
> I agree.
> 
> > But, I am not sure whether it's better to have a standard hypercall between
> > guest & hypervisor for this purpose so things can be more flexible?
> 
> Maybe. But for now we need to handle the current situation.
> 
> 

Agreed.  Thanks for explaining.