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

[Dave Young]

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