From patchwork Thu Feb 13 10:21:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 206586 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, INCLUDES_PATCH, MAILING_LIST_MULTI, SIGNED_OFF_BY, SPF_HELO_NONE, SPF_PASS, USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7D2EC2BA83 for ; Thu, 13 Feb 2020 10:21:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 79FCE217F4 for ; Thu, 13 Feb 2020 10:21:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1581589273; bh=IeX0Nmb+kGa1vFykUX5swjdoGA0U15Ds2OLnYh8rk30=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=RdVQoyYFGi9NRmj2fHx4akQFZN6Z/9lSBnOdMJNgCTYOjngKC1Vd/56zROIz/iUZH AUANAE9KTB0FQ7jZVSq3xGaSBdSHXhGA6qrv6gfIz0nWZ0SC2FyIATuKaXAhrvRxc5 cCVnFF77H7xUAxYomWskXB1IALG4unugMwe+ZkTo= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729511AbgBMKVN (ORCPT ); Thu, 13 Feb 2020 05:21:13 -0500 Received: from mail.kernel.org ([198.145.29.99]:54134 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729428AbgBMKVN (ORCPT ); Thu, 13 Feb 2020 05:21:13 -0500 Received: from cam-smtp0.cambridge.arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 45558222C2; Thu, 13 Feb 2020 10:21:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1581589272; bh=IeX0Nmb+kGa1vFykUX5swjdoGA0U15Ds2OLnYh8rk30=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=jO+k2vgkHQJvqoKrupD/5sLcqsDd/WyH8/Gnw1ZWs0dmIZwpZvLiLmbOHrY7Xn9JZ 14qStVRdmi3RN/r/PO6oxNOzfMb+UR5klvUxY1GN1SojjT1YcsTIzH7T2NwaqrF2cs JasQ4QC49EVWZ1yKQs44zoPIF1vI6ABb4M4om3Zw= From: Ard Biesheuvel To: linux-efi@vger.kernel.org Cc: Ard Biesheuvel , Hans de Goede , Arvind Sankar , Andy Lutomirski Subject: [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Date: Thu, 13 Feb 2020 11:21:00 +0100 Message-Id: <20200213102102.30170-2-ardb@kernel.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20200213102102.30170-1-ardb@kernel.org> References: <20200213102102.30170-1-ardb@kernel.org> Sender: linux-efi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-efi@vger.kernel.org The mixed mode runtime wrappers are fragile when it comes to how the memory referred to by its pointer arguments are laid out in memory, due to the fact that it translates these addresses to physical addresses that the runtime services can dereference when running in 1:1 mode. As a quick (i.e., backportable) fix, copy GUID pointer arguments to the local stack into a buffer that is naturally aligned to its size, so that is guaranteed to cover only one physical page. Note that on x86, we cannot rely on the stack pointer being aligned the way the compiler expects, so we need to allocate an 8-byte aligned buffer of sufficient size, and copy the GUID into that buffer at an offset that is aligned to 16 bytes. Reported-by: Hans de Goede Signed-off-by: Ard Biesheuvel --- arch/x86/platform/efi/efi_64.c | 25 ++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index fa8506e76bbe..543edfdcd1b9 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -658,6 +658,8 @@ static efi_status_t efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor, u32 *attr, unsigned long *data_size, void *data) { + u8 buf[24] __aligned(8); + efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd)); efi_status_t status; u32 phys_name, phys_vendor, phys_attr; u32 phys_data_size, phys_data; @@ -665,8 +667,10 @@ efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor, spin_lock_irqsave(&efi_runtime_lock, flags); + *vnd = *vendor; + phys_data_size = virt_to_phys_or_null(data_size); - phys_vendor = virt_to_phys_or_null(vendor); + phys_vendor = virt_to_phys_or_null(vnd); phys_name = virt_to_phys_or_null_size(name, efi_name_size(name)); phys_attr = virt_to_phys_or_null(attr); phys_data = virt_to_phys_or_null_size(data, *data_size); @@ -683,14 +687,18 @@ static efi_status_t efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor, u32 attr, unsigned long data_size, void *data) { + u8 buf[24] __aligned(8); + efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd)); u32 phys_name, phys_vendor, phys_data; efi_status_t status; unsigned long flags; spin_lock_irqsave(&efi_runtime_lock, flags); + *vnd = *vendor; + phys_name = virt_to_phys_or_null_size(name, efi_name_size(name)); - phys_vendor = virt_to_phys_or_null(vendor); + phys_vendor = virt_to_phys_or_null(vnd); phys_data = virt_to_phys_or_null_size(data, data_size); /* If data_size is > sizeof(u32) we've got problems */ @@ -707,6 +715,8 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, u32 attr, unsigned long data_size, void *data) { + u8 buf[24] __aligned(8); + efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd)); u32 phys_name, phys_vendor, phys_data; efi_status_t status; unsigned long flags; @@ -714,8 +724,10 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, if (!spin_trylock_irqsave(&efi_runtime_lock, flags)) return EFI_NOT_READY; + *vnd = *vendor; + phys_name = virt_to_phys_or_null_size(name, efi_name_size(name)); - phys_vendor = virt_to_phys_or_null(vendor); + phys_vendor = virt_to_phys_or_null(vnd); phys_data = virt_to_phys_or_null_size(data, data_size); /* If data_size is > sizeof(u32) we've got problems */ @@ -732,14 +744,18 @@ efi_thunk_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { + u8 buf[24] __aligned(8); + efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd)); efi_status_t status; u32 phys_name_size, phys_name, phys_vendor; unsigned long flags; spin_lock_irqsave(&efi_runtime_lock, flags); + *vnd = *vendor; + phys_name_size = virt_to_phys_or_null(name_size); - phys_vendor = virt_to_phys_or_null(vendor); + phys_vendor = virt_to_phys_or_null(vnd); phys_name = virt_to_phys_or_null_size(name, *name_size); status = efi_thunk(get_next_variable, phys_name_size, @@ -747,6 +763,7 @@ efi_thunk_get_next_variable(unsigned long *name_size, spin_unlock_irqrestore(&efi_runtime_lock, flags); + *vendor = *vnd; return status; } From patchwork Thu Feb 13 10:21:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 206585 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, INCLUDES_PATCH, MAILING_LIST_MULTI, SIGNED_OFF_BY, SPF_HELO_NONE, SPF_PASS, USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0674AC2BA83 for ; Thu, 13 Feb 2020 10:21:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CF013217F4 for ; Thu, 13 Feb 2020 10:21:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1581589277; bh=9cbYbm586BLe73N2VYVbhPZNibByMC7XJYTpPqJNHaY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=PVwj0i+Sow43N7aZG4KX8uBvt27UXh2855VYUElP2y2bbJuwYiJ96Qw1dWF92Rxok olDDf4yBeNMLlvC4F62fV81ZE87sCcYDqQWlp7DPEM54gCLpngUwy9pHoeqOHPXoeU Bu1SUrO8MYCHnwQvNjzRcJXR49+HzZ/bfKYDEsaw= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729699AbgBMKVR (ORCPT ); Thu, 13 Feb 2020 05:21:17 -0500 Received: from mail.kernel.org ([198.145.29.99]:54164 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729428AbgBMKVR (ORCPT ); Thu, 13 Feb 2020 05:21:17 -0500 Received: from cam-smtp0.cambridge.arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3860424650; Thu, 13 Feb 2020 10:21:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1581589276; bh=9cbYbm586BLe73N2VYVbhPZNibByMC7XJYTpPqJNHaY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Xbv2o/y7UyDFL2bCo+3s8k9J46WAGj+aW1sRfpJZKuPsxG6WVwpbFPpMT8f0iv46I fC+XXQ005RjeRZPjJYCPOOn66lPmaupzzplXchQLWzSw1qEXhSJmg6/Dp0gVw+ewdB NG9OZv9Nyx/p1XhwFWG/ajSMXnHP9Pd24hp4H30Y= From: Ard Biesheuvel To: linux-efi@vger.kernel.org Cc: Ard Biesheuvel , Hans de Goede , Arvind Sankar , Andy Lutomirski Subject: [PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode Date: Thu, 13 Feb 2020 11:21:02 +0100 Message-Id: <20200213102102.30170-4-ardb@kernel.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20200213102102.30170-1-ardb@kernel.org> References: <20200213102102.30170-1-ardb@kernel.org> Sender: linux-efi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-efi@vger.kernel.org We currently apply some stringent checks on the variable name and buffer arguments passed to the EFI runtime services, and WARN() if they are allocated in the vmalloc space and are not aligned to their size - which must be a power of two - since in that case, it is not guaranteed that they will not cross a page boundary. However, we then simply proceed with the call, which could cause data corruption if the next physical page belongs to a mapping that is entirely unrelated. Given that with vmap'ed stacks, this condition is much more likely to trigger, let's relax the condition a bit, but fail the runtime service call if it triggers. Signed-off-by: Ard Biesheuvel --- arch/x86/platform/efi/efi_64.c | 45 +++++++++++--------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index ae398587f264..d19a2edd63cb 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -180,7 +180,7 @@ void efi_sync_low_kernel_mappings(void) static inline phys_addr_t virt_to_phys_or_null_size(void *va, unsigned long size) { - bool bad_size; + phys_addr_t pa; if (!va) return 0; @@ -188,16 +188,13 @@ virt_to_phys_or_null_size(void *va, unsigned long size) if (virt_addr_valid(va)) return virt_to_phys(va); - /* - * A fully aligned variable on the stack is guaranteed not to - * cross a page bounary. Try to catch strings on the stack by - * checking that 'size' is a power of two. - */ - bad_size = size > PAGE_SIZE || !is_power_of_2(size); + pa = slow_virt_to_phys(va); - WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size); + /* check if the object crosses a page boundary */ + if (WARN_ON((pa ^ (pa + size - 1)) & PAGE_MASK)) + return 0; - return slow_virt_to_phys(va); + return pa; } #define virt_to_phys_or_null(addr) \ @@ -615,8 +612,11 @@ efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor, phys_attr = virt_to_phys_or_null(attr); phys_data = virt_to_phys_or_null_size(data, *data_size); - status = efi_thunk(get_variable, phys_name, phys_vendor, - phys_attr, phys_data_size, phys_data); + if (!phys_name || (data && !phys_data)) + status = EFI_INVALID_PARAMETER; + else + status = efi_thunk(get_variable, phys_name, phys_vendor, + phys_attr, phys_data_size, phys_data); spin_unlock_irqrestore(&efi_runtime_lock, flags); @@ -641,9 +641,11 @@ efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor, phys_vendor = virt_to_phys_or_null(vnd); phys_data = virt_to_phys_or_null_size(data, data_size); - /* If data_size is > sizeof(u32) we've got problems */ - status = efi_thunk(set_variable, phys_name, phys_vendor, - attr, data_size, phys_data); + if (!phys_name || !phys_data) + status = EFI_INVALID_PARAMETER; + else + status = efi_thunk(set_variable, phys_name, phys_vendor, + attr, data_size, phys_data); spin_unlock_irqrestore(&efi_runtime_lock, flags); @@ -670,9 +672,11 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, phys_vendor = virt_to_phys_or_null(vnd); phys_data = virt_to_phys_or_null_size(data, data_size); - /* If data_size is > sizeof(u32) we've got problems */ - status = efi_thunk(set_variable, phys_name, phys_vendor, - attr, data_size, phys_data); + if (!phys_name || !phys_data) + status = EFI_INVALID_PARAMETER; + else + status = efi_thunk(set_variable, phys_name, phys_vendor, + attr, data_size, phys_data); spin_unlock_irqrestore(&efi_runtime_lock, flags); @@ -698,8 +702,11 @@ efi_thunk_get_next_variable(unsigned long *name_size, phys_vendor = virt_to_phys_or_null(vnd); phys_name = virt_to_phys_or_null_size(name, *name_size); - status = efi_thunk(get_next_variable, phys_name_size, - phys_name, phys_vendor); + if (!phys_name) + status = EFI_INVALID_PARAMETER; + else + status = efi_thunk(get_next_variable, phys_name_size, + phys_name, phys_vendor); spin_unlock_irqrestore(&efi_runtime_lock, flags);