Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756680AbbDOS4S (ORCPT ); Wed, 15 Apr 2015 14:56:18 -0400 Received: from mail-la0-f51.google.com ([209.85.215.51]:33383 "EHLO mail-la0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752040AbbDOS4H (ORCPT ); Wed, 15 Apr 2015 14:56:07 -0400 MIME-Version: 1.0 In-Reply-To: <20150415131826.GE4804@codeblueprint.co.uk> References: <1429058752-13478-1-git-send-email-roy.franz@linaro.org> <20150415131826.GE4804@codeblueprint.co.uk> Date: Wed, 15 Apr 2015 11:56:05 -0700 Message-ID: Subject: Re: [PATCH] x86_64/efi: enforce 32 bit address for command line buffer From: Roy Franz To: Matt Fleming Cc: Linux Kernel Mailing List , "linux-efi@vger.kernel.org" , Matt Fleming , "H. Peter Anvin" , mingo@redhat.com, x86@kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2929 Lines: 69 On Wed, Apr 15, 2015 at 6:18 AM, Matt Fleming wrote: > On Tue, 14 Apr, at 05:45:52PM, Roy Franz wrote: >> The boot_params structure has a 32 bit field for storing the address of >> the kernel command line. When the EFI stub allocates memory for the command >> line, it allocates at as low and address as possible, but does not ensure >> that the address of memory allocated is below 4G. >> This patch enforces this limit, and the stub now returns an error if the >> command line buffer is allocated at too high of an address. >> For 32 bit systems, the EFI mandated 1-1 memory mapping ensures >> that all memory is 32 bit addressable, so we don't have a problem. >> Also, mixed-mode booting on EFI platforms does not use the stub >> code, so we don't need to handle the case of booting a 32 bit >> kernel on a 64 bit EFI platform. >> >> Signed-off-by: Roy Franz >> --- >> arch/x86/boot/compressed/eboot.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c >> index ef17683..82dbe27 100644 >> --- a/arch/x86/boot/compressed/eboot.c >> +++ b/arch/x86/boot/compressed/eboot.c >> @@ -1108,6 +1108,19 @@ struct boot_params *make_boot_params(struct efi_config *c) >> cmdline_ptr = efi_convert_cmdline(sys_table, image, &options_size); >> if (!cmdline_ptr) >> goto fail; >> + >> +#ifdef CONFIG_X86_64 >> + /* >> + * hdr->cmd_line_ptr is a 32 bit field, so on 64 bit systems we need >> + * to ensure that the allocated buffer for the commandline is 32 bit >> + * addressable. >> + */ >> + if ((u64)(cmdline_ptr) + options_size > (u64)U32_MAX) { >> + efi_printk(sys_table, "Failed to alloc lowmem for command line\n"); >> + efi_free(sys_table, options_size, (unsigned long)cmdline_ptr); >> + goto fail; >> + } >> +#endif /* CONFIG_X86_64 */ >> hdr->cmd_line_ptr = (unsigned long)cmdline_ptr; >> >> hdr->ramdisk_image = 0; > > Good catch. But actually, we have boot_params->ext_cmd_line_ptr for > exactly this problem. > > So yes, that's a valid bug, but I don't think this is how we should fix > it. Yeah, I guess it shouldn't surprise me that there is support for 64 bit addresses there :) I'l spin another patch that sets boot_params->ext_cmd_line_ptr with the upper 32 bits of the address. Should I conditionalize this with #ifdef CONFIG_X86_64, or just do it unconditionally, with it being a NOP on 32 bit? (I guess I may end up with an extra cast for the 32 bit case) Roy > > -- > Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/