linux-kernel - Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp

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:   Tue, 25 Dec 2018 15:43:12 +0800
From:   Chao Fan <fanc.fnst@...fujitsu.com>
To:     Ingo Molnar <mingo@...nel.org>
CC:     <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <bp@...en8.de>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <hpa@...or.com>,
        <keescook@...omium.org>, <bhe@...hat.com>, <msys.mizuma@...il.com>,
        <indou.takao@...fujitsu.com>, <caoj.fnst@...fujitsu.com>
Subject: Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find
 RSDP from EFI table

On Mon, Dec 17, 2018 at 06:36:05PM +0100, Ingo Molnar wrote:
>
>* Ingo Molnar <mingo@...nel.org> wrote:
>
>> > +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> > +			rsdp_addr = (acpi_physical_address)table;
>> > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> > +			return (acpi_physical_address)table;
>> 
>> 'return' is not a function.
>

Hi Ingo,

I will remove the typecast in next version.
But when doing more tests, I found a special condition.

>Disregard this - I got confused by the type cast.
>
>The type cast is ugly nevertheless. 'table' is an 'unsigned long'.
>
>So 'acpi_physical_address' is basically an u64/u32, depending on 
>ACPI_MACHINE_WIDTH, right?
>
>Since this is x86, can ACPI_MACHINE_WIDTH ever get out of sync with the 
>native 'unsigned long' size?

Not always.
Here is define of acpi_physical_address:

#if ACPI_MACHINE_WIDTH == 64
typedef u64 acpi_physical_address;
#elif ACPI_MACHINE_WIDTH == 32
#ifdef ACPI_32BIT_PHYSICAL_ADDRESS
typedef u32 acpi_physical_address;
#else                           /* ACPI_32BIT_PHYSICAL_ADDRESS */

/*
 * It is reported that, after some calculations, the physical addresses
can
 * wrap over the 32-bit boundary on 32-bit PAE environment.
 * https://bugzilla.kernel.org/show_bug.cgi?id=87971
 */
typedef u64 acpi_physical_address;
#endif

That means in 32-bit with PAE, acpi_physical_address is u64.
I set up a debian9-32bit with PAE in QEMU and found size of
acpi_physical_address is 8. So they are not always sync.
Of course, that doesn't influence the conclusion, since return table directly
can always work. It will return u32 to u32, u64 to u64, or u32 to u64.
So the typecast about acpi_physical_address can be removed.

Thanks,
Chao Fan

>
>If not then why not make the return type 'unsigned long', instead of 
>'acpi_physical_address' that you have to wade through a couple of headers 
>to figure out its true size. Does that cause complications elsewhere?
>
>I.e. the excessive type casts are ugly and make the code somewhat 
>fragile.
>
>Thanks,
>
>	Ingo
>
>