Re: [RFC PATCH v3 12/20] x86: Decrypt trampoline area if memory encryption is active
On 11/17/2016 12:09 PM, Borislav Petkov wrote: > On Wed, Nov 09, 2016 at 06:37:08PM -0600, Tom Lendacky wrote: >> When Secure Memory Encryption is enabled, the trampoline area must not >> be encrypted. A CPU running in real mode will not be able to decrypt >> memory that has been encrypted because it will not be able to use addresses >> with the memory encryption mask. >> >> Signed-off-by: Tom Lendacky >> --- >> arch/x86/realmode/init.c |9 + >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c >> index 5db706f1..44ed32a 100644 >> --- a/arch/x86/realmode/init.c >> +++ b/arch/x86/realmode/init.c >> @@ -6,6 +6,7 @@ >> #include >> #include >> #include >> +#include >> >> struct real_mode_header *real_mode_header; >> u32 *trampoline_cr4_features; >> @@ -130,6 +131,14 @@ static void __init set_real_mode_permissions(void) >> unsigned long text_start = >> (unsigned long) __va(real_mode_header->text_start); >> >> +/* >> + * If memory encryption is active, the trampoline area will need to >> + * be in un-encrypted memory in order to bring up other processors >> + * successfully. >> + */ >> +sme_early_mem_dec(__pa(base), size); >> +sme_set_mem_unenc(base, size); > > We're still unsure about the non-encrypted state: dec vs unenc. Please > unify those for ease of use, code reading, etc etc. > > sme_early_decrypt(__pa(base), size); > sme_mark_decrypted(base, size); > > or similar looks much more readable and understandable to me. Yeah, I'll go through and change everything so that the implication or action is expressed better. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v3 11/20] x86: Add support for changing memory encryption attribute
On 11/17/2016 11:39 AM, Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 06:36:55PM -0600, Tom Lendacky wrote:
>> This patch adds support to be change the memory encryption attribute for
>> one or more memory pages.
>
> "Add support for changing ..."
Yeah, I kind of messed up that description a bit!
>
>> Signed-off-by: Tom Lendacky
>> ---
>> arch/x86/include/asm/cacheflush.h |3 +
>> arch/x86/include/asm/mem_encrypt.h | 13 ++
>> arch/x86/mm/mem_encrypt.c | 43 +
>> arch/x86/mm/pageattr.c | 73
>>
>> 4 files changed, 132 insertions(+)
>
> ...
>
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 411210d..41cfdf9 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> extern pmdval_t early_pmd_flags;
>> int __init __early_make_pgtable(unsigned long, pmdval_t);
>> @@ -33,6 +34,48 @@ EXPORT_SYMBOL_GPL(sme_me_mask);
>> /* Buffer used for early in-place encryption by BSP, no locking needed */
>> static char sme_early_buffer[PAGE_SIZE] __aligned(PAGE_SIZE);
>>
>> +int sme_set_mem_enc(void *vaddr, unsigned long size)
>> +{
>> +unsigned long addr, numpages;
>> +
>> +if (!sme_me_mask)
>> +return 0;
>
> So those interfaces look duplicated to me: you have exported
> sme_set_mem_enc/sme_set_mem_unenc which take @size and then you have
> set_memory_enc/set_memory_dec which take numpages.
>
> And then you're testing sme_me_mask in both.
>
> What I'd prefer to have is only *two* set_memory_enc/set_memory_dec
> which take size in bytes and one workhorse __set_memory_enc_dec() which
> does it all. The user shouldn't have to care about numpages or size or
> whatever.
>
> Ok?
Yup, makes sense. I'll redo this.
>
>> +
>> +addr = (unsigned long)vaddr & PAGE_MASK;
>> +numpages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +
>> +/*
>> + * The set_memory_xxx functions take an integer for numpages, make
>> + * sure it doesn't exceed that.
>> + */
>> +if (numpages > INT_MAX)
>> +return -EINVAL;
>> +
>> +return set_memory_enc(addr, numpages);
>> +}
>> +EXPORT_SYMBOL_GPL(sme_set_mem_enc);
>> +
>> +int sme_set_mem_unenc(void *vaddr, unsigned long size)
>> +{
>> +unsigned long addr, numpages;
>> +
>> +if (!sme_me_mask)
>> +return 0;
>> +
>> +addr = (unsigned long)vaddr & PAGE_MASK;
>> +numpages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +
>> +/*
>> + * The set_memory_xxx functions take an integer for numpages, make
>> + * sure it doesn't exceed that.
>> + */
>> +if (numpages > INT_MAX)
>> +return -EINVAL;
>> +
>> +return set_memory_dec(addr, numpages);
>> +}
>> +EXPORT_SYMBOL_GPL(sme_set_mem_unenc);
>> +
>> /*
>> * This routine does not change the underlying encryption setting of the
>> * page(s) that map this memory. It assumes that eventually the memory is
>> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
>> index b8e6bb5..babf3a6 100644
>> --- a/arch/x86/mm/pageattr.c
>> +++ b/arch/x86/mm/pageattr.c
>> @@ -1729,6 +1729,79 @@ int set_memory_4k(unsigned long addr, int numpages)
>> __pgprot(0), 1, 0, NULL);
>> }
>>
>> +static int __set_memory_enc_dec(struct cpa_data *cpa)
>> +{
>> +unsigned long addr;
>> +int numpages;
>> +int ret;
>> +
>> +/* People should not be passing in unaligned addresses */
>> +if (WARN_ONCE(*cpa->vaddr & ~PAGE_MASK,
>> + "misaligned address: %#lx\n", *cpa->vaddr))
>> +*cpa->vaddr &= PAGE_MASK;
>> +
>> +addr = *cpa->vaddr;
>> +numpages = cpa->numpages;
>> +
>> +/* Must avoid aliasing mappings in the highmem code */
>> +kmap_flush_unused();
>> +vm_unmap_aliases();
>> +
>> +ret = __change_page_attr_set_clr(cpa, 1);
>> +
>> +/* Check whether we really changed something */
>> +if (!(cpa->flags & CPA_FLUSHTLB))
>> +goto out;
>
> That label is used only once - just "return ret;" here.
Yup, will do.
>
>> +/*
>> + * On success we use CLFLUSH, when the CPU supports it to
>> + * avoid the WBINVD.
>> + */
>> +if (!ret && static_cpu_has(X86_FEATURE_CLFLUSH))
>> +cpa_flush_range(addr, numpages, 1);
>> +else
>> +cpa_flush_all(1);
>> +
>> +out:
>> +return ret;
>> +}
>> +
>> +int set_memory_enc(unsigned long addr, int numpages)
>> +{
>> +struct cpa_data cpa;
>> +
>> +if (!sme_me_mask)
>> +return 0;
>> +
>> +memset(&cpa, 0, sizeof(cpa));
>> +cpa.vaddr = &addr;
>> +cpa.numpages = numpages;
>> +cpa.mask_set = __pgprot(_PAGE_ENC);
>> +cpa.mask_clr = __pgprot(0);
>> +cpa.pgd = init_mm.pgd;
>
> You could move that...
>
>> +
>> +return __set_memory_enc_dec(&cpa);
>> +}
>> +EXPORT_SYMBOL(set_memory_enc);
>> +
>> +int set_memory_d
Re: [RFC PATCH v3 10/20] Add support to access boot related data in the clear
On 11/17/2016 9:55 AM, Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 06:36:31PM -0600, Tom Lendacky wrote:
>> Boot data (such as EFI related data) is not encrypted when the system is
>> booted and needs to be accessed unencrypted. Add support to apply the
>> proper attributes to the EFI page tables and to the early_memremap and
>> memremap APIs to identify the type of data being accessed so that the
>> proper encryption attribute can be applied.
>>
>> Signed-off-by: Tom Lendacky
>> ---
>> arch/x86/include/asm/e820.h|1
>> arch/x86/kernel/e820.c | 16 +++
>> arch/x86/mm/ioremap.c | 89
>>
>> arch/x86/platform/efi/efi_64.c | 12 -
>> drivers/firmware/efi/efi.c | 33 +++
>> include/linux/efi.h|2 +
>> kernel/memremap.c |8 +++-
>> mm/early_ioremap.c | 18 +++-
>> 8 files changed, 172 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
>> index 476b574..186f1d04 100644
>> --- a/arch/x86/include/asm/e820.h
>> +++ b/arch/x86/include/asm/e820.h
>> @@ -16,6 +16,7 @@ extern struct e820map *e820_saved;
>> extern unsigned long pci_mem_start;
>> extern int e820_any_mapped(u64 start, u64 end, unsigned type);
>> extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>> +extern unsigned int e820_get_entry_type(u64 start, u64 end);
>> extern void e820_add_region(u64 start, u64 size, int type);
>> extern void e820_print_map(char *who);
>> extern int
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index b85fe5f..92fce4e 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -107,6 +107,22 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned
>> type)
>> return 0;
>> }
>>
>> +unsigned int e820_get_entry_type(u64 start, u64 end)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < e820->nr_map; i++) {
>> +struct e820entry *ei = &e820->map[i];
>> +
>> +if (ei->addr >= end || ei->addr + ei->size <= start)
>> +continue;
>> +
>> +return ei->type;
>> +}
>> +
>> +return 0;
>
> Please add a
>
> #define E820_TYPE_INVALID 0
>
> or so and return it instead of the naked number 0.
>
> Also, this patch can be split in logical parts. The e820 stuff can be a
> separate pre-patch.
>
> efi_table_address_match() and the tables definitions is a second pre-patch.
>
> The rest is then the third patch.
>
Ok, I'll add the new #define and split this into separate patches.
> ...
>
>> +}
>> +
>> /*
>> * Add a memory region to the kernel e820 map.
>> */
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index ff542cd..ee347c2 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -20,6 +20,9 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +#include
>>
>> #include "physaddr.h"
>>
>> @@ -418,6 +421,92 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>> iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>> }
>>
>> +static bool memremap_setup_data(resource_size_t phys_addr,
>> +unsigned long size)
>
> This function name doesn't read like what the function does.
>
Ok, I'll work on the naming.
>> +{
>> +u64 paddr;
>> +
>> +if (phys_addr == boot_params.hdr.setup_data)
>> +return true;
>> +
>> +paddr = boot_params.efi_info.efi_memmap_hi;
>> +paddr <<= 32;
>> +paddr |= boot_params.efi_info.efi_memmap;
>> +if (phys_addr == paddr)
>> +return true;
>> +
>> +paddr = boot_params.efi_info.efi_systab_hi;
>> +paddr <<= 32;
>> +paddr |= boot_params.efi_info.efi_systab;
>> +if (phys_addr == paddr)
>> +return true;
>> +
>> +if (efi_table_address_match(phys_addr))
>> +return true;
>> +
>> +return false;
>> +}
>
> arch/x86/built-in.o: In function `memremap_setup_data':
> /home/boris/kernel/alt-linux/arch/x86/mm/ioremap.c:444: undefined reference
> to `efi_table_address_match'
> arch/x86/built-in.o: In function `memremap_apply_encryption':
> /home/boris/kernel/alt-linux/arch/x86/mm/ioremap.c:462: undefined reference
> to `efi_mem_type'
> make: *** [vmlinux] Error 1
>
> I guess due to
>
> # CONFIG_EFI is not set
>
Good catch, I'll make sure this builds without CONFIG_EFI.
>> +
>> +static bool memremap_apply_encryption(resource_size_t phys_addr,
>> + unsigned long size)
>
> This name is misleading too: it doesn't apply encryption but checks
> whether to apply encryption for @phys_addr or not. So something like:
>
> ... memremap_should_encrypt(...)
> {
> return true - for should
> return false - for should not
>
> should make the whole thing much more straightforward. Or am I
> misunderstanding you here?
>
No, you got it. Maybe even something memre
Re: [RFC PATCH v3 09/20] x86: Insure that boot memory areas are mapped properly
On 11/17/2016 6:20 AM, Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 06:36:20PM -0600, Tom Lendacky wrote:
>> The boot data and command line data are present in memory in an
>> un-encrypted state and are copied early in the boot process. The early
>> page fault support will map these areas as encrypted, so before attempting
>> to copy them, add unencrypted mappings so the data is accessed properly
>> when copied.
>>
>> For the initrd, encrypt this data in place. Since the future mapping of the
>> initrd area will be mapped as encrypted the data will be accessed properly.
>>
>> Signed-off-by: Tom Lendacky
>> ---
>> arch/x86/include/asm/mem_encrypt.h | 13
>> arch/x86/kernel/head64.c | 21 --
>> arch/x86/kernel/setup.c|9 ++
>> arch/x86/mm/mem_encrypt.c | 56
>>
>> 4 files changed, 96 insertions(+), 3 deletions(-)
>
> ...
>
>> @@ -122,6 +131,12 @@ static void __init copy_bootdata(char *real_mode_data)
>> char * command_line;
>> unsigned long cmd_line_ptr;
>>
>> +/*
>> + * If SME is active, this will create un-encrypted mappings of the
>> + * boot data in advance of the copy operations
> ^
> |
> Fullstop--+
>
>> + */
>> +sme_map_bootdata(real_mode_data);
>> +
>> memcpy(&boot_params, real_mode_data, sizeof boot_params);
>> sanitize_boot_params(&boot_params);
>> cmd_line_ptr = get_cmd_line_ptr();
>
> ...
>
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 06235b4..411210d 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -16,8 +16,11 @@
>>
>> #include
>> #include
>> +#include
>> +#include
>>
>> extern pmdval_t early_pmd_flags;
>> +int __init __early_make_pgtable(unsigned long, pmdval_t);
>>
>> /*
>> * Since sme_me_mask is set early in the boot process it must reside in
>> @@ -126,6 +129,59 @@ void __init sme_early_mem_dec(resource_size_t paddr,
>> unsigned long size)
>> }
>> }
>>
>> +static void __init *sme_bootdata_mapping(void *vaddr, unsigned long size)
>
> So this could be called __sme_map_bootdata(). "sme_bootdata_mapping"
> doesn't tell me what the function does as there's no verb in the name.
>
Ok, makes sense.
>> +{
>> +unsigned long paddr = (unsigned long)vaddr - __PAGE_OFFSET;
>> +pmdval_t pmd_flags, pmd;
>> +void *ret = vaddr;
>
> That *ret --->
>
>> +
>> +/* Use early_pmd_flags but remove the encryption mask */
>> +pmd_flags = early_pmd_flags & ~sme_me_mask;
>> +
>> +do {
>> +pmd = (paddr & PMD_MASK) + pmd_flags;
>> +__early_make_pgtable((unsigned long)vaddr, pmd);
>> +
>> +vaddr += PMD_SIZE;
>> +paddr += PMD_SIZE;
>> +size = (size < PMD_SIZE) ? 0 : size - PMD_SIZE;
>
> size <= PMD_SIZE
>
> looks more obvious to me...
Ok, will do.
>
>> +} while (size);
>> +
>> +return ret;
>
> ---> is simply passing vaddr out. So the function can be just as well be
> void and you can do below:
>
> __sme_map_bootdata(real_mode_data, sizeof(boot_params));
>
> boot_data = (struct boot_params *)real_mode_data;
>
> ...
Ok, that simplifies the function too.
>
>> +void __init sme_map_bootdata(char *real_mode_data)
>> +{
>> +struct boot_params *boot_data;
>> +unsigned long cmdline_paddr;
>> +
>> +if (!sme_me_mask)
>> +return;
>> +
>> +/*
>> + * The bootdata will not be encrypted, so it needs to be mapped
>> + * as unencrypted data so it can be copied properly.
>> + */
>> +boot_data = sme_bootdata_mapping(real_mode_data, sizeof(boot_params));
>> +
>> +/*
>> + * Determine the command line address only after having established
>> + * the unencrypted mapping.
>> + */
>> +cmdline_paddr = boot_data->hdr.cmd_line_ptr |
>> +((u64)boot_data->ext_cmd_line_ptr << 32);
>
> < newline here.
>
>> +if (cmdline_paddr)
>> +sme_bootdata_mapping(__va(cmdline_paddr), COMMAND_LINE_SIZE);
>> +}
>> +
>> +void __init sme_encrypt_ramdisk(resource_size_t paddr, unsigned long size)
>> +{
>> +if (!sme_me_mask)
>> +return;
>> +
>> +sme_early_mem_enc(paddr, size);
>> +}
>
> So this one could simply be called sme_encrypt_area() and be used for
> other things. There's nothing special about encrypting a ramdisk, by the
> looks of it.
The sme_early_mem_enc() function is already exposed so I'll use that. I
originally had it that way but tried to hide any logic associated with
it by just calling this function. Any changes in logic in the future
would be handled within the SME function. But that can be done in the
future if needed.
Thanks,
Tom
>
_
Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm
On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote: > On 2016-11-07 22:47, Luis R. Rodriguez wrote: > > If so > > why? If this issue is present also on systems that only use ACPI is > > this possibly due to an ACPI firmware bug or the lack of some semantics > > in ACPI to express ordering in a better way? If the issue is device > > tree related only is this due to the lack of semantics in device tree > > to express some more complex dependency ? > > The main feature of device links that is used in this patch is enabling > runtime pm dependency between Exynos SYSMMU controller (called it client > device) and the device, for which it implements DMA address translation > (called master device). The assumptions are following: > 1. master device driver is completely unaware of the Exynos SYSMMU presence, >IOMMU is transparently hooked up and managed by DMA-mapping framework > 2. SYSMMU belongs to the same power domain as it's master device > 3. SYSMMU is optional, master device can fully operate without it, with >simple DMA address translation (DMA address == physical address) > 4. Master device implements runtime pm, what in turn causes respective >power domain to be turned on/off > 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU >when its master device is performing DMA operations, so SYSMMU has >to be runtime active > 6. Currently SYSMMU always sets its runtime pm status to active after >attaching to its master device to ensure proper hardware state. This >prevents power domain to be turned off, even when master device sets >its runtime pm status to suspended. > 7. Exynos SYSMMU has to be runtime active at the same time when its >master device is runtime active to it to perform DMA operations and >allow the power domain to be turned off, when master device is >runtime suspended. > 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master >device is a 'supplier'. You seem to have mixed up the consumer and supplier in point 8 above. Your code is such that the SYSMMU is the supplier and the master device is the consumer: device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME); Prototype of device_link_add: struct device_link *device_link_add(struct device *consumer, struct device *supplier, u32 flags); Your code is correct, only point 8 above is wrong. Best regards, Lukas ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
