| 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: Fri, 12 Jun 2009 11:17:49 -0700
From: Yinghai Lu <yinghai@...nel.org>
To: "H. Peter Anvin" <hpa@...or.com>
CC: Cliff Wickman <cpw@....com>, linux-kernel@...r.kernel.org,
"Huang, Ying" <ying.huang@...el.com>, pj@....com
Subject: Re: [PATCH] x86: vendor reserved memory type
H. Peter Anvin wrote:
> Yinghai, Huang, Paul: looks good to you [see patch at end]? Anyone else
> we should have look at this?
>
> -hpa
>
>
> Cliff Wickman wrote:
>>> There is no difference between that and E820_RESERVED, so there is no
>>> reason to distinguish them. The semantics are exactly the same.
>> I thought a new type would be clearer, but if it would break an e820
>> standard I withdraw the idea. All is good as long as the memory gets reserved.
>
> We *could* add private types with negative numbers if we had to, but
> that means adding some infrastructure, and this doesn't seem justified
> for this case. There is also a cost involved, since different types
> can't be range-merged.
>
>>> The real problem is that this condition is too lenient:
>>>
>>> if (md->attribute & EFI_MEMORY_WB)
>>> e820_type = E820_RAM;
>>> else
>>> e820_type = E820_RESERVED;
>>>
>>> It really should be something like:
>>>
>>> switch (md->type) {
>>> case EFI_LOADER_CODE:
>>> case EFI_LOADER_DATA:
>>> case EFI_BOOT_SERVICES_CODE:
>>> case EFI_BOOT_SERVICES_DATA:
>>> case EFI_CONVENTIONAL_MEMORY:
>>> if (md->attribute & EFI_MEMORY_WB)
>>> e820_type = E820_RAM;
>>> else
>>> e820_type = E820_RESERVED;
>>> break;
>>> case EFI_ACPI_RECLAIM_MEMORY:
>>> e820_type = E820_ACPI;
>>> break;
>>> case EFI_ACPI_MEMORY_NVS:
>>> e820_type = E820_NVS;
>>> break;
>>> case EFI_UNUSABLE_MEMORY:
>>> e820_type = E820_UNUSUABLE;
>>> break;
>>> default:
>>> e820_type = E820_RESERVED;
>>> break;
>>> }
>> Okay. I buy that as more straightforward.
>>
>>> Personally, it's not clear to me if this should do add any non-memory
>>> ranges, as the boot loader should have done that, but I guess in this
>>> particular case we have already horked out.
>>>
>>> Another problem is that the comment is wrong. sanitize_e820_map() will
>>> coalesce adjacent entries, as it should.
>>>
>>> Finally, randomly definiting a standard value in E820 with new semantics
>>> isn't going to fly; it's likely to conflict with official allocations.
>>>
>>> -hpa
>> I propose to submit your code (basically) in the form of the below patch.
>> It works for me. Does it look okay to you?
>>
>>
>>
>> Subject: [PATCH] x86: efi/e820 table merge fix
>>
>> This patch causes all the EFI_RESERVED_TYPE memory reservations to be recorded
>> in the e820 table as type E820_RESERVED.
>>
>> Without this patch EFI_RESERVED_TYPE memory reservations may be
>> marked usable in the e820 table. There may be a collision between
>> kernel use and some reserver's use of this memory.
>>
>> (An example use of this functionality is the UV system, which
>> will access extremely large areas of memory with a memory engine
>> that allows a user to address beyond the processor's range. Such
>> areas are reserved in the EFI table by the BIOS.
>> Some loaders have a restricted number of entries possible in the e820 table,
>> hence the need to record the reservations in the unrestricted EFI table.)
>>
>> The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
>> on the kernel command line.
>>
>> Diffed against 2.6.30-rc8
>>
>> Signed-off-by: Cliff Wickman <cpw@....com>
>> ---
>> arch/x86/kernel/efi.c | 31 ++++++++++++++++++++++++++++---
>> 1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> Index: linux/arch/x86/kernel/efi.c
>> ===================================================================
>> --- linux.orig/arch/x86/kernel/efi.c
>> +++ linux/arch/x86/kernel/efi.c
>> @@ -240,10 +240,35 @@ static void __init do_add_efi_memmap(voi
>> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
>> int e820_type;
>>
>> - if (md->attribute & EFI_MEMORY_WB)
>> - e820_type = E820_RAM;
>> - else
>> + switch (md->type) {
>> + case EFI_LOADER_CODE:
>> + case EFI_LOADER_DATA:
>> + case EFI_BOOT_SERVICES_CODE:
>> + case EFI_BOOT_SERVICES_DATA:
>> + case EFI_CONVENTIONAL_MEMORY:
>> + if (md->attribute & EFI_MEMORY_WB)
>> + e820_type = E820_RAM;
>> + else
>> + e820_type = E820_RESERVED;
>> + break;
>> + case EFI_ACPI_RECLAIM_MEMORY:
>> + e820_type = E820_ACPI;
>> + break;
>> + case EFI_ACPI_MEMORY_NVS:
>> + e820_type = E820_NVS;
>> + break;
>> + case EFI_UNUSABLE_MEMORY:
>> + e820_type = E820_UNUSABLE;
>> + break;
>> + default:
>> + /*
>> + * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
>> + * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
>> + * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
>> + */
>> e820_type = E820_RESERVED;
then, if those entries are near TOML, and it is E820_RESERVED now, and it could not be directly mapped at first point.
but later efi_remap will direct map it if the size is too big for runtime service.
not sure others.
YH
>> + break;
>> + }
>> e820_add_region(start, size, e820_type);
>> }
>> sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists