From: Nicolai Stange <nicstange@gmail.com>
To: Dave Young <dyoung@redhat.com>
Cc: Nicolai Stange <nicstange@gmail.com>,
Matt Fleming <matt@codeblueprint.co.uk>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, Ingo Molnar <mingo@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
hpa@zytor.com, Dan Williams <dan.j.williams@intel.com>,
mika.penttila@nextfour.com, bhsharma@redhat.com
Subject: Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code
Date: Fri, 13 Jan 2017 13:21:52 +0100 [thread overview]
Message-ID: <87o9zbno9r.fsf@gmail.com> (raw)
In-Reply-To: <20170113030404.GA14023@dhcp-128-65.nay.redhat.com> (Dave Young's message of "Fri, 13 Jan 2017 11:04:04 +0800")
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
next prev parent reply other threads:[~2017-01-13 12:21 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-12 9:41 [PATCH 0/4] efi/x86: move efi bgrt init code to early init Dave Young
2017-01-12 9:41 ` [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young
2017-01-12 11:15 ` Nicolai Stange
2017-01-12 21:29 ` Dave Young
2017-01-27 14:48 ` Matt Fleming
2017-01-27 17:04 ` Ard Biesheuvel
2017-01-27 22:13 ` Matt Fleming
2017-01-27 22:15 ` Ard Biesheuvel
2017-01-12 16:15 ` Ard Biesheuvel
2017-01-12 21:20 ` Dave Young
2017-01-13 8:10 ` Dave Young
2017-01-12 9:41 ` [PATCH 2/4] efi/x86: move efi bgrt init code to early init code Dave Young
2017-01-12 9:56 ` Dave Young
2017-01-12 11:54 ` Nicolai Stange
2017-01-12 21:39 ` Dave Young
2017-01-12 23:11 ` Nicolai Stange
2017-01-13 2:21 ` Dave Young
2017-01-13 3:04 ` Dave Young
2017-01-13 12:21 ` Nicolai Stange [this message]
2017-01-16 2:55 ` Dave Young
2017-01-12 16:20 ` Ard Biesheuvel
2017-01-12 21:33 ` Dave Young
2017-01-16 15:15 ` Bhupesh Sharma
2017-01-17 17:00 ` Ard Biesheuvel
2017-01-12 9:41 ` [PATCH 3/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c Dave Young
2017-01-12 12:08 ` Nicolai Stange
2017-01-12 21:40 ` Dave Young
2017-01-12 9:41 ` [PATCH 4/4] efi/x86: add debug code to print cooked memmap Dave Young
2017-01-12 16:18 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o9zbno9r.fsf@gmail.com \
--to=nicstange@gmail.com \
--cc=ard.biesheuvel@linaro.org \
--cc=bhsharma@redhat.com \
--cc=dan.j.williams@intel.com \
--cc=dyoung@redhat.com \
--cc=hpa@zytor.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=mika.penttila@nextfour.com \
--cc=mingo@kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).