linux-kernel - Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data

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: <a0c3ebb9-37b4-3f47-86df-eac110c07032@amd.com>
Date:   Mon, 13 Mar 2017 15:08:52 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     Brijesh Singh <brijesh.singh@....com>, <simon.guinot@...uanux.org>,
        <linux-efi@...r.kernel.org>, <kvm@...r.kernel.org>,
        <rkrcmar@...hat.com>, <matt@...eblueprint.co.uk>,
        <linux-pci@...r.kernel.org>, <linus.walleij@...aro.org>,
        <gary.hook@....com>, <linux-mm@...ck.org>,
        <paul.gortmaker@...driver.com>, <hpa@...or.com>, <cl@...ux.com>,
        <dan.j.williams@...el.com>, <aarcange@...hat.com>,
        <sfr@...b.auug.org.au>, <andriy.shevchenko@...ux.intel.com>,
        <herbert@...dor.apana.org.au>, <bhe@...hat.com>,
        <xemul@...allels.com>, <joro@...tes.org>, <x86@...nel.org>,
        <peterz@...radead.org>, <piotr.luc@...el.com>, <mingo@...hat.com>,
        <msalter@...hat.com>, <ross.zwisler@...ux.intel.com>, <bp@...e.de>,
        <dyoung@...hat.com>, <jroedel@...e.de>, <keescook@...omium.org>,
        <arnd@...db.de>, <toshi.kani@....com>,
        <mathieu.desnoyers@...icios.com>, <luto@...nel.org>,
        <devel@...uxdriverproject.org>, <bhelgaas@...gle.com>,
        <tglx@...utronix.de>, <mchehab@...nel.org>,
        <iamjoonsoo.kim@....com>, <labbott@...oraproject.org>,
        <tony.luck@...el.com>, <alexandre.bounine@....com>,
        <kuleshovmail@...il.com>, <linux-kernel@...r.kernel.org>,
        <mcgrof@...nel.org>, <mst@...hat.com>,
        <linux-crypto@...r.kernel.org>, <tj@...nel.org>,
        <pbonzini@...hat.com>, <akpm@...ux-foundation.org>,
        <davem@...emloft.net>
Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup
 data

On 3/6/2017 6:03 PM, Bjorn Helgaas wrote:
> On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote:
>> On 3/3/2017 2:42 PM, Bjorn Helgaas wrote:
>>> On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote:
>>>> From: Tom Lendacky <thomas.lendacky@....com>
>>>>
>>>> The use of ioremap will force the setup data to be mapped decrypted even
>>>> though setup data is encrypted.  Switch to using memremap which will be
>>>> able to perform the proper mapping.
>>>
>>> How should callers decide whether to use ioremap() or memremap()?
>>>
>>> memremap() existed before SME and SEV, and this code is used even if
>>> SME and SEV aren't supported, so the rationale for this change should
>>> not need the decryption argument.
>>
>> When SME or SEV is active an ioremap() will remove the encryption bit
>> from the pagetable entry when it is mapped.  This allows MMIO, which
>> doesn't support SME/SEV, to be performed successfully.  So my take is
>> that ioremap() should be used for MMIO and memremap() for pages in RAM.
>
> OK, thanks.  The commit message should say something like "this is
> RAM, not MMIO, so we should map it with memremap(), not ioremap()".
> That's the part that determines whether the change is correct.
>
> You can mention the encryption part, too, but it's definitely
> secondary because the change has to make sense on its own, without
> SME/SEV.
>

Ok, that makes sense, will do.

> The following commits (from https://github.com/codomania/tip/branches)
> all do basically the same thing so the changelogs (and summaries)
> should all be basically the same:
>
>   cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data
>   91acb68b8333 x86/pci: Use memremap when walking setup data
>   4f687503e23f x86: Access the setup data through sysfs decrypted
>   e90246b8c229 x86: Access the setup data through debugfs decrypted
>
> I would collect them all together and move them to the beginning of
> your series, since they don't depend on anything else.

I'll do that.

>
> Also, change "x86/pci: " to "x86/PCI" so it matches the previous
> convention.

Will do.

Thanks,
Tom

>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
>
>>>> ---
>>>> arch/x86/pci/common.c |    4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>>>> index a4fdfa7..0b06670 100644
>>>> --- a/arch/x86/pci/common.c
>>>> +++ b/arch/x86/pci/common.c
>>>> @@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev)
>>>>
>>>> 	pa_data = boot_params.hdr.setup_data;
>>>> 	while (pa_data) {
>>>> -		data = ioremap(pa_data, sizeof(*rom));
>>>> +		data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB);
>>>
>>> I can't quite connect the dots here.  ioremap() on x86 would do
>>> ioremap_nocache().  memremap(MEMREMAP_WB) would do arch_memremap_wb(),
>>> which is ioremap_cache().  Is making a cacheable mapping the important
>>> difference?
>>
>> The memremap(MEMREMAP_WB) will actually check to see if it can perform
>> a __va(pa_data) in try_ram_remap() and then fallback to the
>> arch_memremap_wb().  So it's actually the __va() vs the ioremap_cache()
>> that is the difference.
>>
>> Thanks,
>> Tom
>>
>>>
>>>> 		if (!data)
>>>> 			return -ENOMEM;
>>>>
>>>> @@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev)
>>>> 			}
>>>> 		}
>>>> 		pa_data = data->next;
>>>> -		iounmap(data);
>>>> +		memunmap(data);
>>>> 	}
>>>> 	set_dma_domain_ops(dev);
>>>> 	set_dev_domain_options(dev);
>>>>