| 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
| ||
|
Date: Thu, 18 Apr 2019 13:17:03 +0000
From: "Lendacky, Thomas" <Thomas.Lendacky@....com>
To: Borislav Petkov <bp@...en8.de>, lijiang <lijiang@...hat.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"luto@...nel.org" <luto@...nel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
"dyoung@...hat.com" <dyoung@...hat.com>,
"bhe@...hat.com" <bhe@...hat.com>
Subject: Re: [PATCH 1/2 RESEND v10] x86/mm, resource: add a new I/O resource
descriptor 'IORES_DESC_RESERVED'
On 4/18/19 5:01 AM, Borislav Petkov wrote:
> On Wed, Apr 17, 2019 at 02:40:09PM +0800, lijiang wrote:
>> Based on the above description, i made a draft patch, please refer to it. But it
>> seems that the code has been changed a lot.
>
> It goes in the right direction. Here is a version where I corrected some
> things:
>
> ---
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index dd73d5d74393..0c1392e76b7d 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -27,9 +27,8 @@
>
> #include "physaddr.h"
>
> -struct ioremap_mem_flags {
> - bool system_ram;
> - bool desc_other;
> +struct ioremap_desc {
> + u64 flags;
You could probably make this unsigned int for now since you're creating
the new enum used to assign to this variable and it only uses two bits
currently.
> };
>
> /*
> @@ -61,13 +60,13 @@ int ioremap_change_attr(unsigned long vaddr, unsigned long size,
> return err;
> }
>
> -static bool __ioremap_check_ram(struct resource *res)
> +static u64 __ioremap_check_ram(struct resource *res)
> {
> unsigned long start_pfn, stop_pfn;
> unsigned long i;
>
> if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM)
> - return false;
> + return 0;
>
> start_pfn = (res->start + PAGE_SIZE - 1) >> PAGE_SHIFT;
> stop_pfn = (res->end + 1) >> PAGE_SHIFT;
> @@ -75,28 +74,39 @@ static bool __ioremap_check_ram(struct resource *res)
> for (i = 0; i < (stop_pfn - start_pfn); ++i)
> if (pfn_valid(start_pfn + i) &&
> !PageReserved(pfn_to_page(start_pfn + i)))
> - return true;
> + return IORES_MAP_SYSTEM_RAM;
> }
>
> - return false;
> + return 0;
> }
>
> -static int __ioremap_check_desc_other(struct resource *res)
> +/*
> + * NONE and RESERVED should not be mapped encrypted in SEV because there
> + * the whole memory is already encrypted.
> + */
> +static unsigned long __ioremap_check_desc(struct resource *res)
If you don't make the change to unsigned int, then this should be a u64
return to match the the flags definition and the other function.
> {
> - return (res->desc != IORES_DESC_NONE);
> + if (!sev_active())
> + return 0;
> +
> + if (res->desc & (IORES_DESC_NONE | IORES_DESC_RESERVED))
> + return 0;
The IORES_DESC_xxxx enums are not bits, just sequential numbers, so you
won't be able to test them this way. You can probably get away with a
switch statement here.
Thanks,
Tom
> +
> + return IORES_MAP_ENCRYPTED;
> }
>
> static int __ioremap_res_check(struct resource *res, void *arg)
> {
> - struct ioremap_mem_flags *flags = arg;
> + struct ioremap_desc *desc = arg;
>
> - if (!flags->system_ram)
> - flags->system_ram = __ioremap_check_ram(res);
> + if (!(desc->flags & IORES_MAP_SYSTEM_RAM))
> + desc->flags |= __ioremap_check_ram(res);
>
> - if (!flags->desc_other)
> - flags->desc_other = __ioremap_check_desc_other(res);
> + if (!(desc->flags & IORES_MAP_ENCRYPTED))
> + desc->flags |= __ioremap_check_desc(res);
>
> - return flags->system_ram && flags->desc_other;
> + return ((desc->flags & (IORES_MAP_SYSTEM_RAM | IORES_MAP_ENCRYPTED)) ==
> + (IORES_MAP_SYSTEM_RAM | IORES_MAP_ENCRYPTED));
> }
>
> /*
> @@ -105,13 +115,13 @@ static int __ioremap_res_check(struct resource *res, void *arg)
> * resource described not as IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
> */
> static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
> - struct ioremap_mem_flags *flags)
> + struct ioremap_desc *desc)
> {
> u64 start, end;
>
> start = (u64)addr;
> end = start + size - 1;
> - memset(flags, 0, sizeof(*flags));
> + memset(desc, 0, sizeof(struct ioremap_desc));
>
> walk_mem_res(start, end, flags, __ioremap_res_check);
> }
> @@ -138,7 +148,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> resource_size_t last_addr;
> const resource_size_t unaligned_phys_addr = phys_addr;
> const unsigned long unaligned_size = size;
> - struct ioremap_mem_flags mem_flags;
> + struct ioremap_desc io_desc;
> struct vm_struct *area;
> enum page_cache_mode new_pcm;
> pgprot_t prot;
> @@ -157,12 +167,12 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> return NULL;
> }
>
> - __ioremap_check_mem(phys_addr, size, &mem_flags);
> + __ioremap_check_mem(phys_addr, size, &io_desc);
>
> /*
> * Don't allow anybody to remap normal RAM that we're using..
> */
> - if (mem_flags.system_ram) {
> + if (io_desc.flags & IORES_MAP_SYSTEM_RAM) {
> WARN_ONCE(1, "ioremap on RAM at %pa - %pa\n",
> &phys_addr, &last_addr);
> return NULL;
> @@ -200,7 +210,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> * resulting mapping.
> */
> prot = PAGE_KERNEL_IO;
> - if ((sev_active() && mem_flags.desc_other) || encrypted)
> + if ((io_desc.flags & IORES_MAP_ENCRYPTED) || encrypted)
> prot = pgprot_encrypted(prot);
>
> switch (pcm) {
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index da0ebaec25f0..2a5e0f1ded93 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -12,6 +12,7 @@
> #ifndef __ASSEMBLY__
> #include <linux/compiler.h>
> #include <linux/types.h>
> +#include <linux/bits.h>
> /*
> * Resources are tree-like, allowing
> * nesting etc..
> @@ -135,6 +136,14 @@ enum {
> IORES_DESC_DEVICE_PUBLIC_MEMORY = 7,
> };
>
> +/*
> + * Flags controlling ioremap() behavior.
> + */
> +enum {
> + IORES_MAP_SYSTEM_RAM = BIT(0),
> + IORES_MAP_ENCRYPTED = BIT(1),
> +};
> +
> /* helpers to define resources */
> #define DEFINE_RES_NAMED(_start, _size, _name, _flags) \
> { \
>
Powered by blists - more mailing lists