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]

Date:   Fri, 13 Jan 2017 11:04:04 +0800
From:   Dave Young <dyoung@...hat.com>
To:     Nicolai Stange <nicstange@...il.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, 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 01/13/17 at 10:21am, Dave Young wrote:
> On 01/13/17 at 12:11am, Nicolai Stange wrote:
> > On Fri, Jan 13 2017, Dave Young wrote:
> > 
> > > On 01/12/17 at 12:54pm, Nicolai Stange wrote:
> > >> 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;
> > >
> > > Nicolai, sorry, I'm not sure I understand the comment, is it about above line?
> > > Could you elaborate a bit?
> > >
> > >> 
> > >> and this length check
> > >> 
> > >
> > > I also do not get this :(
> > 
> > Ah sorry, my point is this: the length check should perhaps be made
> > before doing the assignment to bgrt_tab because otherwise, we might end
> > up reading from invalid memory.
> > 
> > I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then
> > 
> >   bgrt_tab = *(struct acpi_table_bgrt *)table;
> > 
> > would read past the table's end.
> > 
> > I'm not sure whether this is a real problem though -- that is, whether
> > this read could ever hit some unmapped memory.
> 
> Nicolai, thanks for the explanation. It make sense to move it to even later
> at the end of the function.

Indeed assignment should be after the length checking, but with another
tmp variable the assignment to global var can be moved to the end to
avoid clear the image_address field..

> 
> Thanks
> Dave