linux-kernel - Re: [PATCH v4 2/4] x86/boot: Add acpitb.c to parse acpi tables

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]

Date:   Fri, 3 Aug 2018 10:00:48 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Chao Fan <fanc.fnst@...fujitsu.com>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <hpa@...or.com>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bhe@...hat.com>,
        <keescook@...omium.org>, <yasu.isimatu@...il.com>
CC:     <indou.takao@...fujitsu.com>, <caoj.fnst@...fujitsu.com>
Subject: Re: [PATCH v4 2/4] x86/boot: Add acpitb.c to parse acpi tables



At 07/23/2018 05:29 PM, Chao Fan wrote:
> Imitate the ACPI code to parse ACPI tables. Functions are simplified
> cause some operations are not needed here.
> And also, this method won't influence the initialization of ACPI.
> 
> Signed-off-by: Chao Fan <fanc.fnst@...fujitsu.com>

Hi Fan,

I know you got the code from acpica subsystem and EFI code... and do
many adaptation work for KASLR. It's awesome!

I think you can add some other simple comments.

   - what differences between your function and the function you based on
     and why did you do that?

... to make this more credible and easy to remember the details as time
goes on.

Also some concerns below.
> ---
[...]
> +	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
> +		efi_64 = false;
> +	else {
> +		debug_putstr("Wrong efi loader signature.\n");

s/efi/EFI/, also need fix in the comments below.

> +		return false;
> +	}
> +
[...]
> +		/*
> +		 * Get rsdp from efi tables.
> +		 * If we find acpi table, go on searching for acpi20 table.
> +		 * If we didn't get acpi20 table then use acpi table.
> +		 * If neither acpi table nor acpi20 table found,
> +		 * return false.
> +		 */
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)) && !acpi_20) {
> +			*rsdp_addr = (acpi_physical_address)table;
> +			acpi_20 = false;
> +			find_rsdp = true;
> +		} else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
> +			*rsdp_addr = (acpi_physical_address)table;
> +			acpi_20 = true;
> +			return true;

If we find the ACPI 2.0, we will return immediately, so the variable and
logic of _acpi_20_ is redundant.

Thanks,
	dou