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

[Nicolai Stange]

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.

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:'


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...

Thanks,

Nicolai