linux-kernel - Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID

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] [day] [month] [year] [list]

Date:   Fri, 26 Jun 2020 11:35:07 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     "Luck, Tony" <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Joerg Roedel <joro@...tes.org>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Christoph Hellwig <hch@...radeed.org>,
        Ashok Raj <ashok.raj@...el.com>,
        Jacob Jun Pan <jacob.jun.pan@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Sohil Mehta <sohil.mehta@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, iommu@...ts.linux-foundation.org
Subject: Re: [PATCH v4 12/12] x86/traps: Fix up invalid PASID

Hi, Dave,

On Fri, Jun 26, 2020 at 11:23:12AM -0700, Dave Hansen wrote:
> On 6/26/20 11:10 AM, Luck, Tony wrote:
> > On Fri, Jun 26, 2020 at 11:44:50AM +0200, Peter Zijlstra wrote:
> >> On Thu, Jun 25, 2020 at 01:17:22PM -0700, Fenghua Yu wrote:
> >>
> >>> +static bool fixup_pasid_exception(void)
> >>> +{
> >>> +	if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
> >>> +		return false;
> >>> +	if (!static_cpu_has(X86_FEATURE_ENQCMD))
> >>> +		return false;
> >>
> >> elsewhere you had another variation:
> >>
> >> +       if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
> >> +               return;
> >> +
> >> +       if (!cpu_feature_enabled(X86_FEATURE_ENQCMD))
> >> +               return;
> >>
> >> Which is it, and why do we need the CONFIG thing when combined with the
> >> enabled thing?
> > 
> > Do we have a standard way of coding for a feature that depends on multiple
> > other features?  For this case the system needs to both suport the ENQCMD
> > instruction, and also have kernel code that programs the IOMMU.
> 
> Not really a standard one.
> 
> We could setup_clear_cpu_cap(X86_FEATURE_ENQCMD) during boot if we see
> that CONFIG_INTEL_IOMMU_SVM=n or if we don't have a detected IOMMU.
> That would at least get static value patching which would make some of
> the feature checks very cheap.
> 
> That means we can't use ENQCMD at all in the kernel, though.  Is that an
> issue?  Is the CPU feature truly dependent on IOMMU_SVM?

ENQCMD instruction needs bind_mm()/unbind_mm() defined in svm.c
which is only compiled by:
obj-$(CONFIG_INTEL_IOMMU_SVM) += intel/svm.o

So I think ENQCMD instruction is truly dependent on CONFIG_INTEL_IOMMU_SVM.

> 
> > And/or guidance on when to use each of the very somewhat simlar looking
> > 	boot_cpu_has()
> > 	static_cpu_has()
> > 	IS_ENABLED()
> > 	cpu_feature_enabled(X86_FEATURE_ENQCMD)
> > options?
> 
> cpu_feature_enabled() is by go-to for checking X86_FEATUREs.  It has
> compile-time checking along with static checking.
> 
> If you use cpu_feature_enabled(), and we added:
> 
> #ifdef CONFIG_INTEL_IOMMU_SVM
> # define DISABLE_ENQCMD         0
> #else
> # define DISABLE_ENQCMD           (1 << (X86_FEATURE_ENQCMD & <bitval>))
> #endif
> 
> to arch/x86/include/asm/disabled-features.h, then we could check only
> X86_FEATURE_ENQCMD, and we'd get that IS_ENABLED() check for "free".

This makes code simpler and cleaner.

Thanks.

-Fenghua