| 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
| ||
|
Message-ID: <87o9zbno9r.fsf@gmail.com>
Date: Fri, 13 Jan 2017 13:21:52 +0100
From: Nicolai Stange <nicstange@...il.com>
To: Dave Young <dyoung@...hat.com>
Cc: Nicolai Stange <nicstange@...il.com>,
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 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
Powered by blists - more mailing lists