From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750960AbdALXMA (ORCPT ); Thu, 12 Jan 2017 18:12:00 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34036 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbdALXL6 (ORCPT ); Thu, 12 Jan 2017 18:11:58 -0500 From: Nicolai Stange To: Dave Young Cc: Nicolai Stange , Matt Fleming , Ard Biesheuvel , linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org, Ingo Molnar , Thomas Gleixner , hpa@zytor.com, Dan Williams , mika.penttila@nextfour.com, bhsharma@redhat.com Subject: Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code References: <20170112094118.815108042@redhat.com> <20170112094214.909117564@redhat.com> <871sw8pk73.fsf@gmail.com> <20170112213939.GD2709@dhcp-128-65.nay.redhat.com> Date: Fri, 13 Jan 2017 00:11:53 +0100 In-Reply-To: <20170112213939.GD2709@dhcp-128-65.nay.redhat.com> (Dave Young's message of "Fri, 13 Jan 2017 05:39:39 +0800") Message-ID: <87shonooue.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <87shonooue.fsf@gmail.com> References: <20170112094118.815108042@redhat.com> <20170112094214.909117564@redhat.com> <871sw8pk73.fsf@gmail.com> <20170112213939.GD2709@dhcp-128-65.nay.redhat.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <20170112213939.GD2709-0VdLhd/A9Pl+NNSt+8eSiB/sF2h8X+2i0E9HWUfgJXw@public.gmane.org> (Dave Young's message of "Fri, 13 Jan 2017 05:39:39 +0800") Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Dave Young Cc: Nicolai Stange , Matt Fleming , Ard Biesheuvel , linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Ingo Molnar , Thomas Gleixner , hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org, Dan Williams , mika.penttila-MRsr7dthA9VWk0Htik3J/w@public.gmane.org, bhsharma-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org List-Id: linux-efi@vger.kernel.org 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