Messages in this thread | | | From | Nicolai Stange <> | Subject | Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code | Date | Fri, 13 Jan 2017 00:11:53 +0100 |
| |
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.
>> > - if (bgrt_tab->header.length < sizeof(*bgrt_tab)) { >> > + if (bgrt_tab.header.length < sizeof(bgrt_tab)) { >> > pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n", >> > - bgrt_tab->header.length, sizeof(*bgrt_tab)); >> > + bgrt_tab.header.length, sizeof(bgrt_tab)); >> > return; >> > } >> >> ?
Thanks,
Nicolai
| |