linux-kernel - Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code

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]

Message-ID: <871sw8pk73.fsf@gmail.com>
Date:   Thu, 12 Jan 2017 12:54:40 +0100
From:   Nicolai Stange <nicstange@...il.com>
To:     Dave Young <dyoung@...hat.com>
Cc:     Matt Fleming <matt@...eblueprint.co.uk>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        x86@...nel.org, Nicolai Stange <nicstange@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>, hpa@...or.com,
        Dan Williams <dan.j.williams@...el.com>,
        mika.penttila@...tfour.com, bhsharma@...hat.com
Subject: Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code

On Thu, Jan 12 2017, Dave Young wrote:

> -void __init efi_bgrt_init(void)
> +void __init efi_bgrt_init(struct acpi_table_header *table)
>  {
> -	acpi_status status;
>  	void *image;
>  	struct bmp_header bmp_header;
>  
>  	if (acpi_disabled)
>  		return;
>  
> -	status = acpi_get_table("BGRT", 0,
> -	                        (struct acpi_table_header **)&bgrt_tab);
> -	if (ACPI_FAILURE(status))
> -		return;


Not sure, but wouldn't it be safer to reverse the order of this assignment

> +	bgrt_tab = *(struct acpi_table_bgrt *)table;

and this length check

> -	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
> +	if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
>  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -		       bgrt_tab->header.length, sizeof(*bgrt_tab));
> +		       bgrt_tab.header.length, sizeof(bgrt_tab));
>  		return;
>  	}

?

Also, from here on, all error paths should zero out
bgrt_tab.image_address (or so) to signal failure to bgrt_init():
bgrt_init() now checks for !bgrt_tab.image_address whereas before it had
tested bgrt_image and the latter used to be set at the very end of
efi_bgrt_init().


> -	if (bgrt_tab->version != 1) {
> +	if (bgrt_tab.version != 1) {
>  		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> -		       bgrt_tab->version);
> +		       bgrt_tab.version);
>  		return;
>  	}
> -	if (bgrt_tab->status & 0xfe) {
> +	if (bgrt_tab.status & 0xfe) {
>  		pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> -		       bgrt_tab->status);
> +		       bgrt_tab.status);
>  		return;
>  	}
> -	if (bgrt_tab->image_type != 0) {
> +	if (bgrt_tab.image_type != 0) {
>  		pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
> -		       bgrt_tab->image_type);
> +		       bgrt_tab.image_type);
>  		return;
>  	}
> -	if (!bgrt_tab->image_address) {
> +	if (!bgrt_tab.image_address) {
>  		pr_notice("Ignoring BGRT: null image address\n");
>  		return;
>  	}
>  
> -	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> +	image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header));
>  	if (!image) {
>  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
>  		return;
>  	}
>  
>  	memcpy(&bmp_header, image, sizeof(bmp_header));
> -	memunmap(image);
> +	early_memunmap(image, sizeof(bmp_header));
>  	if (bmp_header.id != 0x4d42) {
>  		pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
>  			bmp_header.id);
> @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void)
>  	}
>  	bgrt_image_size = bmp_header.size;
>  
> -	bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
> -	if (!bgrt_image) {
> -		pr_notice("Ignoring BGRT: failed to map image memory\n");
> -		bgrt_image = NULL;
> -		return;
> -	}
> -
> -	efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
> +	efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size);
>  }
> --- linux-x86.orig/drivers/acpi/bgrt.c
> +++ linux-x86/drivers/acpi/bgrt.c
> @@ -15,40 +15,41 @@
>  #include <linux/sysfs.h>
>  #include <linux/efi-bgrt.h>
>  
> +static void *bgrt_image;

[...]

> @@ -84,9 +85,17 @@ static int __init bgrt_init(void)
>  {
>  	int ret;
>  
> -	if (!bgrt_image)
> +	if (!bgrt_tab.image_address)
>  		return -ENODEV;
>  
> +	bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
> +			      MEMREMAP_WB);
> +	if (!bgrt_image) {
> +		pr_notice("Ignoring BGRT: failed to map image memory\n");
> +		bgrt_image = NULL;
> +		return -ENOMEM;
> +	}
> +
>  	bin_attr_image.private = bgrt_image;
>  	bin_attr_image.size = bgrt_image_size;
>  

Thanks,

Nicolai