From patchwork Thu Nov 29 17:12:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 152420 Delivered-To: patch@linaro.org Received: by 2002:a2e:299d:0:0:0:0:0 with SMTP id p29-v6csp2634223ljp; Thu, 29 Nov 2018 09:13:06 -0800 (PST) X-Google-Smtp-Source: AFSGD/XTgvMtjM0b2kzUwqI7gQXmzwFtCwpxwtVkJpNBbtLTFF8pqeB0hKAu/jWYqt1pApnwcSfL X-Received: by 2002:a17:902:24e1:: with SMTP id l30-v6mr2173466plg.42.1543511586195; Thu, 29 Nov 2018 09:13:06 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1543511586; cv=none; d=google.com; s=arc-20160816; b=BBVOGxUQ7gh1627VTNMah9o3VxCFKhHxnSI8Diq88Ot3Ce8OywpK5BzGWlG8aCpX95 VAiI/pW3K056EtAALIDtHqlKSfwPmB+UR79upntlP+bIyb7pAOcZK0yiOK/C18q6XzT6 2vJNGL9RctLEIjlEwMvcSUbsb1bCDgjD6CQwYSORUa+qFu2e2gqqBVXKU3MxZCzRkk1O VJCjb6qOyRbKXkAO/NUMUsIDfsuxF5c5CMhuArGAMyFFpsuu+e2gwqCwiMFuSz+gvNls 8FliuFPYvgy0B+M64cU5W7yz7pL0MYz18+LonE3WfbzMlbnv7yt2bpzv8S62SYe/BvAL 15SQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=phTFw25GwhUds+/fULRXb1CISzvAjAuut2sdVRJvPds=; b=YAq/+2R1AVx1xf8KaZOM5XAwCxVPO2bs7rlQISAmGSgbb0xEz6rvF6vNTyA7dL7ErM nFB+E2gvNFCS7vktZjWLh+bn2QT3RXcrI3nPoWLn/ju0MMcwpyciQ6XoWSZ4OsCrnxGR MymzO6JTCEhBW1e63t+tQ7KUPF1W+VqXc4yjTdG94B9tJ0rd6eIYCQiuh0NdpKPuXZ9V hPX9ffvNfKzaY1uO1A8Rft7mpK4pe1Nfi779tzTRvb5O9xIB9SjXJ8+0WiGsJu5wRMse xFiKYc/KNHD1CDksanm+Wqv+ePaRZPN4JAEEqJTR7IUJKYbp/Pcx/Zu4tRKhh6MLXiDZ XdoQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Ggxv7iOy; 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 t18si2217885pgu.476.2018.11.29.09.13.05; Thu, 29 Nov 2018 09:13:06 -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=Ggxv7iOy; 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 S1730633AbeK3ETI (ORCPT + 32 others); Thu, 29 Nov 2018 23:19:08 -0500 Received: from mail-wr1-f65.google.com ([209.85.221.65]:38130 "EHLO mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728551AbeK3ETG (ORCPT ); Thu, 29 Nov 2018 23:19:06 -0500 Received: by mail-wr1-f65.google.com with SMTP id v13so2679655wrw.5 for ; Thu, 29 Nov 2018 09:13:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=phTFw25GwhUds+/fULRXb1CISzvAjAuut2sdVRJvPds=; b=Ggxv7iOyEyttur6m//qGQl3LeVPtDPb7YfuDpyWPri8vWXX604IlM02Hee+r7CPhBI jGMR/mS2BcklYerUzbqqmCSzGv34txBlNdPTWStxuiPIfL0+E4yUEFlC3r9KeX2fOjRN XMaRKVRKQVJS5ud5sGHN4vvoFm/yym5ww4OnY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=phTFw25GwhUds+/fULRXb1CISzvAjAuut2sdVRJvPds=; b=ivbdiVd45amvArWsX5pT7x1EGCFsLE4HayaJBom37FgBD+or/Brb36DSSNgjX2rIsj DSlpCrZC2EnoiaXmNNWdMj6Zgy7WoP8HLzDwgHTnyxQDBy8N5Ydhsu9IFGtUdF8hF01V tbheiYllZzV9WUZ6cnNsA5aJvCj07LQK7ekORPImfBdoYqEuDL2nd+wx72huiXZsnaQ/ RKym+M91P5JhOsRVjBc4N1Rnc13yUKJ4mBVmvF/kzrhfqjGntf5oKm+00N23U0M6ey+t slJOCdHQbAwTUCHJRpMIPBqwr5LdHG4UJRN0tXRoIBVyanpuWVBsmv/b1fmwIuceexY3 xnYg== X-Gm-Message-State: AA+aEWbwazElv6R5vWlSSf1a60g+hXoq7MXR+9KhbfxJa6IfMo5Zs0B1 HJ3TZ99B3Hup7utiTU5omqPPih8Mq7I= X-Received: by 2002:a5d:4dc8:: with SMTP id f8mr2209772wru.45.1543511581073; Thu, 29 Nov 2018 09:13:01 -0800 (PST) Received: from harold.home ([2a01:cb1d:112:6f00:f070:d240:312e:9f99]) by smtp.gmail.com with ESMTPSA id y185sm1593882wmg.34.2018.11.29.09.12.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 29 Nov 2018 09:13:00 -0800 (PST) From: Ard Biesheuvel To: linux-efi@vger.kernel.org, Ingo Molnar , Thomas Gleixner Cc: Ard Biesheuvel , linux-kernel@vger.kernel.org, Andy Lutomirski , Arend van Spriel , Bhupesh Sharma , Borislav Petkov , Dave Hansen , Eric Snowberg , Hans de Goede , Joe Perches , Jon Hunter , Julien Thierry , Marc Zyngier , Nathan Chancellor , Peter Zijlstra , Sai Praneeth Prakhya , Sedat Dilek , YiFei Zhu Subject: [PATCH 08/11] firmware: efi: add NULL pointer checks in efivars api functions Date: Thu, 29 Nov 2018 18:12:27 +0100 Message-Id: <20181129171230.18699-9-ard.biesheuvel@linaro.org> X-Mailer: git-send-email 2.19.1 In-Reply-To: <20181129171230.18699-1-ard.biesheuvel@linaro.org> References: <20181129171230.18699-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Arend van Spriel Since commit: ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from EFI variables") we have a device driver accessing the efivars API. Several functions in the efivars API assume __efivars is set, i.e., that they will be accessed only after efivars_register() has been called. However, the following NULL pointer access was reported calling efivar_entry_size() from the brcmfmac device driver. Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = 60bfa5f1 [00000008] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM ... Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events request_firmware_work_func PC is at efivar_entry_size+0x28/0x90 LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac] pc : [] lr : [] psr: a00d0113 sp : ede7fe28 ip : ee983410 fp : c1787f30 r10: 00000000 r9 : 00000000 r8 : bf2b2258 r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0 r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: ad16804a DAC: 00000051 Disassembly showed that the local static variable __efivars is NULL, which is not entirely unexpected given that it is a non-EFI platform. So add a NULL pointer check to efivar_entry_size(), and to related functions while at it. In efivars_register() a couple of sanity checks are added as well. Cc: Hans de Goede Reported-by: Jon Hunter Signed-off-by: Arend van Spriel Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/vars.c | 99 +++++++++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 21 deletions(-) -- 2.19.1 diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index 9336ffdf6e2c..fceaafd67ec6 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -318,7 +318,12 @@ EXPORT_SYMBOL_GPL(efivar_variable_is_removable); static efi_status_t check_var_size(u32 attributes, unsigned long size) { - const struct efivar_operations *fops = __efivars->ops; + const struct efivar_operations *fops; + + if (!__efivars) + return EFI_UNSUPPORTED; + + fops = __efivars->ops; if (!fops->query_variable_store) return EFI_UNSUPPORTED; @@ -329,7 +334,12 @@ check_var_size(u32 attributes, unsigned long size) static efi_status_t check_var_size_nonblocking(u32 attributes, unsigned long size) { - const struct efivar_operations *fops = __efivars->ops; + const struct efivar_operations *fops; + + if (!__efivars) + return EFI_UNSUPPORTED; + + fops = __efivars->ops; if (!fops->query_variable_store) return EFI_UNSUPPORTED; @@ -429,13 +439,18 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid, int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), void *data, bool duplicates, struct list_head *head) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; unsigned long variable_name_size = 1024; efi_char16_t *variable_name; efi_status_t status; efi_guid_t vendor_guid; int err = 0; + if (!__efivars) + return -EFAULT; + + ops = __efivars->ops; + variable_name = kzalloc(variable_name_size, GFP_KERNEL); if (!variable_name) { printk(KERN_ERR "efivars: Memory allocation failed.\n"); @@ -583,12 +598,14 @@ static void efivar_entry_list_del_unlock(struct efivar_entry *entry) */ int __efivar_entry_delete(struct efivar_entry *entry) { - const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - status = ops->set_variable(entry->var.VariableName, - &entry->var.VendorGuid, - 0, 0, NULL); + if (!__efivars) + return -EINVAL; + + status = __efivars->ops->set_variable(entry->var.VariableName, + &entry->var.VendorGuid, + 0, 0, NULL); return efi_status_to_err(status); } @@ -607,12 +624,17 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete); */ int efivar_entry_delete(struct efivar_entry *entry) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_status_t status; if (down_interruptible(&efivars_lock)) return -EINTR; + if (!__efivars) { + up(&efivars_lock); + return -EINVAL; + } + ops = __efivars->ops; status = ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid, 0, 0, NULL); @@ -650,13 +672,19 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete); int efivar_entry_set(struct efivar_entry *entry, u32 attributes, unsigned long size, void *data, struct list_head *head) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_status_t status; efi_char16_t *name = entry->var.VariableName; efi_guid_t vendor = entry->var.VendorGuid; if (down_interruptible(&efivars_lock)) return -EINTR; + + if (!__efivars) { + up(&efivars_lock); + return -EINVAL; + } + ops = __efivars->ops; if (head && efivar_entry_find(name, vendor, head, false)) { up(&efivars_lock); return -EEXIST; @@ -687,12 +715,17 @@ static int efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor, u32 attributes, unsigned long size, void *data) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_status_t status; if (down_trylock(&efivars_lock)) return -EBUSY; + if (!__efivars) { + up(&efivars_lock); + return -EINVAL; + } + status = check_var_size_nonblocking(attributes, size + ucs2_strsize(name, 1024)); if (status != EFI_SUCCESS) { @@ -700,6 +733,7 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor, return -ENOSPC; } + ops = __efivars->ops; status = ops->set_variable_nonblocking(name, &vendor, attributes, size, data); @@ -727,9 +761,13 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor, int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes, bool block, unsigned long size, void *data) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_status_t status; + if (!__efivars) + return -EINVAL; + + ops = __efivars->ops; if (!ops->query_variable_store) return -ENOSYS; @@ -829,13 +867,18 @@ EXPORT_SYMBOL_GPL(efivar_entry_find); */ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_status_t status; *size = 0; if (down_interruptible(&efivars_lock)) return -EINTR; + if (!__efivars) { + up(&efivars_lock); + return -EINVAL; + } + ops = __efivars->ops; status = ops->get_variable(entry->var.VariableName, &entry->var.VendorGuid, NULL, size, NULL); up(&efivars_lock); @@ -861,12 +904,14 @@ EXPORT_SYMBOL_GPL(efivar_entry_size); int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes, unsigned long *size, void *data) { - const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - status = ops->get_variable(entry->var.VariableName, - &entry->var.VendorGuid, - attributes, size, data); + if (!__efivars) + return -EINVAL; + + status = __efivars->ops->get_variable(entry->var.VariableName, + &entry->var.VendorGuid, + attributes, size, data); return efi_status_to_err(status); } @@ -882,14 +927,19 @@ EXPORT_SYMBOL_GPL(__efivar_entry_get); int efivar_entry_get(struct efivar_entry *entry, u32 *attributes, unsigned long *size, void *data) { - const struct efivar_operations *ops = __efivars->ops; efi_status_t status; if (down_interruptible(&efivars_lock)) return -EINTR; - status = ops->get_variable(entry->var.VariableName, - &entry->var.VendorGuid, - attributes, size, data); + + if (!__efivars) { + up(&efivars_lock); + return -EINVAL; + } + + status = __efivars->ops->get_variable(entry->var.VariableName, + &entry->var.VendorGuid, + attributes, size, data); up(&efivars_lock); return efi_status_to_err(status); @@ -921,7 +971,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_get); int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, unsigned long *size, void *data, bool *set) { - const struct efivar_operations *ops = __efivars->ops; + const struct efivar_operations *ops; efi_char16_t *name = entry->var.VariableName; efi_guid_t *vendor = &entry->var.VendorGuid; efi_status_t status; @@ -940,6 +990,11 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, if (down_interruptible(&efivars_lock)) return -EINTR; + if (!__efivars) { + err = -EINVAL; + goto out; + } + /* * Ensure that the available space hasn't shrunk below the safe level */ @@ -956,6 +1011,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, } } + ops = __efivars->ops; + status = ops->set_variable(name, vendor, attributes, *size, data); if (status != EFI_SUCCESS) { err = efi_status_to_err(status);