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

[Dave Young <dyoung@redhat.com>]

On 01/13/17 at 01:21pm, Nicolai Stange wrote:
> On Fri, Jan 13 2017, Dave Young wrote:
> 
> > 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..
> 
> I had a look at your updated patches at
> http://people.redhat.com/~ruyang/efi-bgrt/ and they look fine to me.

Many thanks~

> 
> One minor remark:
> 
> sizeof(acpi_table_bgrt) == 56 and it might be better to avoid the extra
> tmp copy in efi_bgrt_init() by
> - assigning directly to bgrt_tab
> - do a 'goto err' rather than a 'return' from all the error paths
> - do a memset(&bgrt_tab, 0, sizeof(bgrt_tab)) at 'err:'

Updated in V2, indeed text size shrunk from 1199 to 762.

> 
> 
> With the copy to the on-stack 'bgrt', gcc 6.2.0 emits this for each of
> the two copies:
> 
>   41:   8a 07                   mov    (%rdi),%al
>   43:   88 45 d7                mov    %al,-0x29(%rbp)
>   46:   8a 47 01                mov    0x1(%rdi),%al
>   49:   88 45 d6                mov    %al,-0x2a(%rbp)
>   4c:   8a 47 02                mov    0x2(%rdi),%al
>   4f:   88 45 d5                mov    %al,-0x2b(%rbp)
>   52:   8a 47 03                mov    0x3(%rdi),%al
>   55:   88 45 d4                mov    %al,-0x2c(%rbp)
>   58:   8a 47 08                mov    0x8(%rdi),%al
>   5b:   88 45 d3                mov    %al,-0x2d(%rbp)
>   5e:   8a 47 09                mov    0x9(%rdi),%al
>   61:   88 45 d2                mov    %al,-0x2e(%rbp)
>   64:   8a 47 0a                mov    0xa(%rdi),%al
>   67:   88 45 d1                mov    %al,-0x2f(%rbp)
>   6a:   8a 47 0b                mov    0xb(%rdi),%al
>   6d:   88 45 d0                mov    %al,-0x30(%rbp)
>   70:   8a 47 0c                mov    0xc(%rdi),%al
>   73:   88 45 cf                mov    %al,-0x31(%rbp)
>   76:   8a 47 0d                mov    0xd(%rdi),%al
>   79:   88 45 ce                mov    %al,-0x32(%rbp)
>   7c:   8a 47 0e                mov    0xe(%rdi),%al
>   7f:   88 45 cd                mov    %al,-0x33(%rbp)
>   82:   8a 47 0f                mov    0xf(%rdi),%al
>   85:   88 45 cc                mov    %al,-0x34(%rbp)
>   88:   8a 47 10                mov    0x10(%rdi),%al
>   8b:   88 45 cb                mov    %al,-0x35(%rbp)
>   8e:   8a 47 11                mov    0x11(%rdi),%al
>   91:   88 45 ca                mov    %al,-0x36(%rbp)
>   94:   8a 47 12                mov    0x12(%rdi),%al
>   97:   88 45 c9                mov    %al,-0x37(%rbp)
>   9a:   8a 47 13                mov    0x13(%rdi),%al
>   9d:   88 45 c8                mov    %al,-0x38(%rbp)
>   a0:   8a 47 14                mov    0x14(%rdi),%al
>   a3:   8a 5f 26                mov    0x26(%rdi),%bl
>   a6:   0f b6 77 27             movzbl 0x27(%rdi),%esi
>   aa:   4c 8b 67 28             mov    0x28(%rdi),%r12
>   ae:   88 45 c7                mov    %al,-0x39(%rbp)
>   b1:   8a 47 15                mov    0x15(%rdi),%al
>   b4:   44 8b 6f 30             mov    0x30(%rdi),%r13d
>   b8:   44 8b 7f 34             mov    0x34(%rdi),%r15d
>   bc:   88 45 c6                mov    %al,-0x3a(%rbp)
>   bf:   8a 47 16                mov    0x16(%rdi),%al
>   c2:   88 45 c5                mov    %al,-0x3b(%rbp)
>   c5:   8a 47 17                mov    0x17(%rdi),%al
>   c8:   88 45 c4                mov    %al,-0x3c(%rbp)
>   cb:   8b 47 18                mov    0x18(%rdi),%eax
>   ce:   89 45 c0                mov    %eax,-0x40(%rbp)
>   d1:   8a 47 1c                mov    0x1c(%rdi),%al
>   d4:   88 45 bf                mov    %al,-0x41(%rbp)
>   d7:   8a 47 1d                mov    0x1d(%rdi),%al
>   da:   88 45 be                mov    %al,-0x42(%rbp)
>   dd:   8a 47 1e                mov    0x1e(%rdi),%al
>   e0:   88 45 bd                mov    %al,-0x43(%rbp)
>   e3:   8a 47 1f                mov    0x1f(%rdi),%al
>   e6:   88 45 bc                mov    %al,-0x44(%rbp)
>   e9:   8b 47 20                mov    0x20(%rdi),%eax
>   ec:   89 45 b8                mov    %eax,-0x48(%rbp)
>   ef:   66 8b 47 24             mov    0x24(%rdi),%ax
> 
> Not sure why gcc would think that storing bgrt in reversed order on the
> stack might be a good idea, but well...

I have no idea about this..

> 
> Thanks,
> 
> Nicolai

Thanks
Dave