Received: by 2002:ac0:8c9a:0:0:0:0:0 with SMTP id r26csp3485379ima; Sun, 3 Feb 2019 23:28:43 -0800 (PST) X-Google-Smtp-Source: ALg8bN4Yt0wmd3zb+o48Qe+aWOeTPz8ewosJ1a1EtmxhONGI483Lr9Iyx9O1T3B/xuo498GD6FdD X-Received: by 2002:a17:902:9a07:: with SMTP id v7mr44203325plp.247.1549265323864; Sun, 03 Feb 2019 23:28:43 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1549265323; cv=none; d=google.com; s=arc-20160816; b=ryTkkIHVo2t4IuOy4OAhxq8htLzQ3XCJuM4WJ3BzoTZZr1zzdkhBWk9qlP6Gogfw1a Jm6z/dnfODiM4MyhKHNWVI6BwvwzsBgj/UoqWw2Qs087VG+FyAPWkFCkOgpM2QP7Khdz UFHJSmoBi1C0b00sIyNta0NYSeoZCIPsOWadlgCd3R8jiWuEXAR5TSqcoOqUhNDBZJsM IKQRVb31EMqOe6pxFJtLpTXhGpE2Ue8L77ivtBYWDAcLL8RPYfyT7AaIuU2dPsOCb72F DjnmEOAMeeiG64UeZxXRdoSmcGQYB57ZXO87HWOudwoOBYNy8ZhwbsaFxexetL9Ky9oI iTPA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=jAAzLY4nUUOH2CgPODRoRtQscXB+WtCb4ZBQSrMoK1Y=; b=b7c3AzGUqacTUCd2QQj6C6C61jPXwHdOmNjre+J+h5pq1Td9NlrRhzY/Kgfyt637jq isYNMXz6Je1X/7PErF9RiS/2Veaf+KzmKk/nVD2dCnqw4ZE0CuQPnH/u2fNIMPJL73c3 gUPTf19mM3fY8pTUeqEL70zKEsKjVqWQibuh6ircbjHkVECL/P17eV26H5Jc6jdgn3Q1 vCCMekjRYRpwC291icnIlLljq79OITDmbpiICY8JZhFhyYr2wY67lnlPcatr1FhK101e ee/3k/ivw3z1zoKou5m8uEL6w0I2Fo4Qnw+9/GiZ6l+I0tR3OQvq936dnpGqfIQTBDdj 1kDQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=fFQ5iom2; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o89si15459821pfk.223.2019.02.03.23.28.27; Sun, 03 Feb 2019 23:28:43 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=fFQ5iom2; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728135AbfBDH2M (ORCPT + 99 others); Mon, 4 Feb 2019 02:28:12 -0500 Received: from mail-it1-f194.google.com ([209.85.166.194]:38772 "EHLO mail-it1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726185AbfBDH2L (ORCPT ); Mon, 4 Feb 2019 02:28:11 -0500 Received: by mail-it1-f194.google.com with SMTP id z20so18908788itc.3 for ; Sun, 03 Feb 2019 23:28:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jAAzLY4nUUOH2CgPODRoRtQscXB+WtCb4ZBQSrMoK1Y=; b=fFQ5iom2VdjPzxBH+62cCZKFr8DxgNh3kV524J26mtGHmBXrjwOxo+5QqZAZtC/Rgr ku/CraPfVzlopWjbeXQStGZWn1bLqApl1X5V/Q+FVBBz1ik7iLwtPuWqBzIx2mVx1qln fNJHrnMAFDVEmFD1jZQ8g/x54u8YStMxOoWsM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jAAzLY4nUUOH2CgPODRoRtQscXB+WtCb4ZBQSrMoK1Y=; b=CQ6f4BwN0G3ai2ZFEP3HamuMnpwk39IEs9DuAreewTDv91SAZ0+cBh42yckljs6ktU daBvwWJjo4WugK61inOKBmU9KyPIGXWeAj/m7TpUbNIzc0WEm5DCmZ1K5Q0ompRUSXfw 3YBDbmMB2i7rxCTjpWa48ab5zwbVk15YbSyn6enXy/p2817IbwUjzOBKvU9h6P1++V2s R3uB66/ZhymPTAl6bjwJPCoral9UDmD9Ky6Jyp6SChkd7hiv1+OC42+6pokGO7lenqx5 P561iY5zO5STH2CG8VaZzw5Cg6ABP4y4jSkWpc9mMdXkZhpnkX5xhpkmPlOcmtx8PHgZ 0gGg== X-Gm-Message-State: AHQUAua9cmNzyIiHam908zu3fPMv7HW2iGGplYtpoEGLGpkYM418KVAJ gmTDs+oL2JJK/JILBm0dARyQGHFA5wm+djpJfpsnIA== X-Received: by 2002:a02:734b:: with SMTP id a11mr1165897jae.62.1549265290404; Sun, 03 Feb 2019 23:28:10 -0800 (PST) MIME-Version: 1.0 References: <20190202094119.13230-1-ard.biesheuvel@linaro.org> <20190202094119.13230-3-ard.biesheuvel@linaro.org> <20190204071809.GA115714@gmail.com> In-Reply-To: <20190204071809.GA115714@gmail.com> From: Ard Biesheuvel Date: Mon, 4 Feb 2019 08:28:02 +0100 Message-ID: Subject: Re: [PATCH 02/10] x86/efi: Return error status if mapping EFI regions fail To: Ingo Molnar Cc: linux-efi , Thomas Gleixner , Linux Kernel Mailing List , AKASHI Takahiro , Alexander Graf , Bjorn Andersson , Borislav Petkov , Heinrich Schuchardt , Jeffrey Hugo , Lee Jones , Leif Lindholm , Linus Torvalds , Peter Jones , Peter Zijlstra , Sai Praneeth Prakhya Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 4 Feb 2019 at 08:18, Ingo Molnar wrote: > > > * Ard Biesheuvel wrote: > > > From: Sai Praneeth Prakhya > > > > efi_map_region() creates VA mappings for an given EFI region using any one > > of the two helper functions (namely __map_region() and old_map_region()). > > These helper functions *could* fail while creating mappings and presently > > their return value is not checked. Not checking for the return value of > > these functions might create issues because after these functions return > > "md->virt_addr" is set to the requested virtual address (so it's assumed > > that these functions always succeed which is not quite true). This > > assumption leads to "md->virt_addr" having invalid mapping should any of > > __map_region() or old_map_region() fail. > > > > Hence, check for the return value of these functions and if indeed they > > fail, turn off EFI Runtime Services forever because kernel cannot > > prioritize among EFI regions. > > > > This also fixes the comment "FIXME: add error handling" in > > kexec_enter_virtual_mode(). > > > > Signed-off-by: Sai Praneeth Prakhya > > Cc: Borislav Petkov > > Cc: Ingo Molnar > > Signed-off-by: Ard Biesheuvel > > --- > > arch/x86/include/asm/efi.h | 6 +++--- > > arch/x86/platform/efi/efi.c | 21 +++++++++++++----- > > arch/x86/platform/efi/efi_32.c | 6 +++--- > > arch/x86/platform/efi/efi_64.c | 39 ++++++++++++++++++++++------------ > > 4 files changed, 48 insertions(+), 24 deletions(-) > > > > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h > > index 107283b1eb1e..a37378f986ec 100644 > > --- a/arch/x86/include/asm/efi.h > > +++ b/arch/x86/include/asm/efi.h > > @@ -125,12 +125,12 @@ extern pgd_t * __init efi_call_phys_prolog(void); > > extern void __init efi_call_phys_epilog(pgd_t *save_pgd); > > extern void __init efi_print_memmap(void); > > extern void __init efi_memory_uc(u64 addr, unsigned long size); > > -extern void __init efi_map_region(efi_memory_desc_t *md); > > -extern void __init efi_map_region_fixed(efi_memory_desc_t *md); > > +extern int __init efi_map_region(efi_memory_desc_t *md); > > +extern int __init efi_map_region_fixed(efi_memory_desc_t *md); > > extern void efi_sync_low_kernel_mappings(void); > > extern int __init efi_alloc_page_tables(void); > > extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages); > > -extern void __init old_map_region(efi_memory_desc_t *md); > > +extern int __init old_map_region(efi_memory_desc_t *md); > > extern void __init runtime_code_page_mkexec(void); > > extern void __init efi_runtime_update_mappings(void); > > extern void __init efi_dump_pagetable(void); > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > > index e1cb01a22fa8..3d43ec58775b 100644 > > --- a/arch/x86/platform/efi/efi.c > > +++ b/arch/x86/platform/efi/efi.c > > @@ -581,7 +581,7 @@ void __init efi_memory_uc(u64 addr, unsigned long size) > > set_memory_uc(addr, npages); > > } > > > > -void __init old_map_region(efi_memory_desc_t *md) > > +int __init old_map_region(efi_memory_desc_t *md) > > { > > u64 start_pfn, end_pfn, end; > > unsigned long size; > > @@ -601,10 +601,14 @@ void __init old_map_region(efi_memory_desc_t *md) > > va = efi_ioremap(md->phys_addr, size, > > md->type, md->attribute); > > > > - md->virt_addr = (u64) (unsigned long) va; > > - if (!va) > > + if (!va) { > > pr_err("ioremap of 0x%llX failed!\n", > > (unsigned long long)md->phys_addr); > > + return -ENOMEM; > > + } > > + > > + md->virt_addr = (u64)(unsigned long)va; > > + return 0; > > Just wondering, shouldn't the failure path set ->virt_addr to something > safe, just in case a caller doesn't check the error and relies on it? > > That's because in this commit we've now changed it from 0 to undefined. > Indeed. We don't usually rely on the value of ->virt_addr when EFI_RUNTIME_SERVICES is unset, but there is some sysfs code, and perhaps some other places where we do reference ->virt_addr, and not assigning it at all is obviously wrong, and potentially hazardous. > > +int __init efi_map_region_fixed(efi_memory_desc_t *md) { return 0; } > > Inline functions should be marked inline ... > > > if (efi_va < EFI_VA_END) { > > - pr_warn(FW_WARN "VA address range overflow!\n"); > > - return; > > + pr_err(FW_WARN "VA address range overflow!\n"); > > + return -ENOMEM; > > } > > > > /* Do the VA map */ > > - __map_region(md, efi_va); > > + if (__map_region(md, efi_va)) > > + return -ENOMEM; > > + > > md->virt_addr = efi_va; > > + return 0; > > Same error return problem of leaving ->virt_addr undefined. > > Note that I also fixed up the grammar and readability of the changelog - > see the updated version below. > > Thanks, > > Ingo > > =============> > Subject: x86/efi: Return error status if mapping of EFI regions fails > From: Ard Biesheuvel > Date: Sat, 2 Feb 2019 10:41:11 +0100 > > From: Sai Praneeth Prakhya > > efi_map_region() creates VA mappings for a given EFI region using one > of the two helper functions (namely __map_region() and old_map_region()). > > These helper functions could fail while creating mappings and presently > their return value is not checked. > > Not checking for the return value of these functions might create bugs, > because after these functions return "md->virt_addr" is set to the > requested virtual address (so it's assumed that these functions always > succeed which is not quite true). This assumption leads to > "md->virt_addr" having invalid mapping, should any of __map_region() > or old_map_region() fail. > > Hence, check for the return value of these functions and if indeed they > fail, turn off EFI Runtime Services forever because kernel cannot > prioritize among EFI regions. > > This also fixes the comment "FIXME: add error handling" in > kexec_enter_virtual_mode(). > Thanks Ingo. Sai, could you please respin this and use Ingo's updated version of the commit log?