From patchwork Fri Apr 4 15:26:42 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Fleming X-Patchwork-Id: 337029 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id D264614007E for ; Sat, 5 Apr 2014 02:27:02 +1100 (EST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WW61D-000886-Mt; Fri, 04 Apr 2014 15:26:59 +0000 Received: from arkanian.console-pimps.org ([212.110.184.194]) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WW617-00087u-LZ for fwts-devel@lists.ubuntu.com; Fri, 04 Apr 2014 15:26:53 +0000 Received: by arkanian.console-pimps.org (Postfix, from userid 1002) id ABF906C055; Fri, 4 Apr 2014 16:26:52 +0100 (BST) Received: from localhost (unknown [94.8.38.27]) by arkanian.console-pimps.org (Postfix) with ESMTPSA id 3D8AF6C055; Fri, 4 Apr 2014 16:26:49 +0100 (BST) From: Matt Fleming To: fwts-devel@lists.ubuntu.com Subject: [PATCH 3/4 v2] efi_runtime: Do not pass user addresses to firmware Date: Fri, 4 Apr 2014 16:26:42 +0100 Message-Id: <1396625202-2542-1-git-send-email-matt@console-pimps.org> X-Mailer: git-send-email 1.8.5.3 In-Reply-To: <1396535003-28253-1-git-send-email-matt@console-pimps.org> References: <1396535003-28253-1-git-send-email-matt@console-pimps.org> Cc: Matt Fleming , Borislav Petkov X-BeenThere: fwts-devel@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Firmware Test Suite Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: fwts-devel-bounces@lists.ubuntu.com Sender: fwts-devel-bounces@lists.ubuntu.com From: Matt Fleming Currently there's some inconsistency with how arguments are passed to the firmware from the efi_runtime driver. Some values have the standard get_user()/put_user() calls, others do not. Passing userspace pointers directly to the firmware is a bug because those addresses may fault. And if they are going to fault we'd like to know about it in the kernel rather than at some later time when executing in firmware context. Furthermore, beginning with v3.14 of the kernel the current tests actually cause the kernel to crash because firmware calls are now done with their own, entirely separate, page tables - no user addresses are mapped during execution of runtime services. This change doesn't require predication on a particular kernel version because the efi_runtime should really have always done this copying to/from userspace for every argument of the runtime services. This patch is heavily based on one from Borislav. Cc: Borislav Petkov Signed-off-by: Matt Fleming Acked-by: Borislav Petkov Acked-by: Ivan Hu --- Changes in v2: - Delete the unused put_ucs2() function - Rename get_ucs"* to copy_ucs2_* to dodge the reference counting implications of the names - Change function arguments to copy_ucs2_from_user_len() to mirror copy_from_user() instead of put_user() to avoid confusion - Expand comments for new helper functions efi_runtime/efi_runtime.c | 238 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 214 insertions(+), 24 deletions(-) diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c index ffe1073..4f9d554 100644 --- a/efi_runtime/efi_runtime.c +++ b/efi_runtime/efi_runtime.c @@ -24,7 +24,7 @@ #include #include #include - +#include #include #include "efi_runtime.h" @@ -100,6 +100,107 @@ static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid) vendor_guid->Data4[i] = vendor->b[i+8]; } +/* + * Count the bytes in 'str', including the terminating NULL. + * + * Note this function returns the number of *bytes*, not the number of + * ucs2 characters. + */ +static inline size_t __ucs2_strsize(uint16_t *str) +{ + uint16_t *s = str; + size_t len; + + if (!str) + return 0; + + /* Include terminating NULL */ + len = sizeof(uint16_t); + + while (*s++ != 0) + len += sizeof(uint16_t); + + return len; +} + +/* + * Allocate a buffer and copy a ucs2 string from user space into it. + * + * We take an explicit number of bytes to use for the allocation and + * copy, and therefore do not make any assumptions about 'src' (such as + * it pointing to a valid string). + * + * If a non-zero value is returned, the caller MUST NOT access 'dst'. + * + * It is the caller's responsibility to free 'dst'. + */ +static inline int +copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) +{ + if (!src) { + *dst = NULL; + return 0; + } + + if (!access_ok(VERIFY_READ, src, 1)) + return -EFAULT; + + *dst = kmalloc(len, GFP_KERNEL); + if (!*dst) + return -ENOMEM; + + if (copy_from_user(*dst, src, len)) { + kfree(*dst); + return -EFAULT; + } + + return 0; +} + +/* + * Calculate the required buffer allocation size and copy a ucs2 string + * from user space into it. + * + * This function differs from copy_ucs2_from_user_len() because it + * calculates the size of the buffer to allocate by taking the length of + * the string 'src'. + * + * If a non-zero value is returned, the caller MUST NOT access 'dst'. + * + * It is the caller's responsibility to free 'dst'. + */ +static inline int copy_ucs2_from_user(uint16_t **dst, uint16_t __user *src) +{ + size_t len; + + if (!access_ok(VERIFY_READ, src, 1)) + return -EFAULT; + + len = __ucs2_strsize(src); + return copy_ucs2_from_user_len(dst, src, len); +} + +/* + * Copy a ucs2 string to a user buffer. + * + * This function is a simple wrapper around copy_to_user() that does + * nothing if 'src' is NULL, which is useful for reducing the amount of + * NULL checking the caller has to do. + * + * 'len' specifies the number of bytes to copy. + */ +static inline int +copy_ucs2_to_user_len(uint16_t __user *dst, uint16_t *src, size_t len) +{ + if (!src) + return 0; + + if (!access_ok(VERIFY_WRITE, dst, 1)) + return -EFAULT; + + return copy_to_user(dst, src, len); +} + static long efi_runtime_get_variable(unsigned long arg) { struct efi_getvariable __user *pgetvariable; @@ -107,7 +208,10 @@ static long efi_runtime_get_variable(unsigned long arg) EFI_GUID vendor_guid; efi_guid_t vendor; efi_status_t status; + uint16_t *name; uint32_t attr; + void *data; + int rv = 0; pgetvariable = (struct efi_getvariable __user *)arg; @@ -117,8 +221,27 @@ static long efi_runtime_get_variable(unsigned long arg) return -EFAULT; convert_from_guid(&vendor, &vendor_guid); - status = efi.get_variable(pgetvariable->VariableName, &vendor, - &attr, &datasize, pgetvariable->Data); + + rv = copy_ucs2_from_user(&name, pgetvariable->VariableName); + if (rv) + return rv; + + data = kmalloc(datasize, GFP_KERNEL); + if (!data) { + kfree(name); + return -ENOMEM; + } + + status = efi.get_variable(name, &vendor, &attr, &datasize, data); + + kfree(name); + + rv = copy_to_user(pgetvariable->Data, data, datasize); + kfree(data); + + if (rv) + return rv; + if (put_user(status, pgetvariable->status)) return -EFAULT; if (status == EFI_SUCCESS) { @@ -141,7 +264,10 @@ static long efi_runtime_set_variable(unsigned long arg) EFI_GUID vendor_guid; efi_guid_t vendor; efi_status_t status; + uint16_t *name; uint32_t attr; + void *data; + int rv; psetvariable = (struct efi_setvariable __user *)arg; if (get_user(datasize, &psetvariable->DataSize) || @@ -151,8 +277,21 @@ static long efi_runtime_set_variable(unsigned long arg) return -EFAULT; convert_from_guid(&vendor, &vendor_guid); - status = efi.set_variable(psetvariable->VariableName, &vendor, - attr, datasize, psetvariable->Data); + + rv = copy_ucs2_from_user(&name, psetvariable->VariableName); + if (rv) + return rv; + + data = kmalloc(datasize, GFP_KERNEL); + if (copy_from_user(data, psetvariable->Data, datasize)) { + kfree(name); + return -EFAULT; + } + + status = efi.set_variable(name, &vendor, attr, datasize, data); + + kfree(data); + kfree(name); if (put_user(status, psetvariable->status)) return -EFAULT; @@ -266,6 +405,8 @@ static long efi_runtime_get_nextvariablename(unsigned long arg) efi_status_t status; efi_guid_t vendor; EFI_GUID vendor_guid; + uint16_t *name; + int rv; pgetnextvariablename = (struct efi_getnextvariablename __user *)arg; @@ -280,9 +421,20 @@ static long efi_runtime_get_nextvariablename(unsigned long arg) convert_from_guid(&vendor, &vendor_guid); - status = efi.get_next_variable(&name_size, - pgetnextvariablename->VariableName, - &vendor); + rv = copy_ucs2_from_user_len(&name, + pgetnextvariablename->VariableName, 1024); + if (rv) + return rv; + + status = efi.get_next_variable(&name_size, name, &vendor); + + rv = copy_ucs2_to_user_len(pgetnextvariablename->VariableName, + name, name_size); + kfree(name); + + if (rv) + return -EFAULT; + if (put_user(status, pgetnextvariablename->status)) return -EFAULT; convert_to_guid(&vendor, &vendor_guid); @@ -302,14 +454,18 @@ static long efi_runtime_get_nexthighmonocount(unsigned long arg) { struct efi_getnexthighmonotoniccount __user *pgetnexthighmonotoniccount; efi_status_t status; + uint32_t count; pgetnexthighmonotoniccount = (struct efi_getnexthighmonotoniccount __user *)arg; - status = efi.get_next_high_mono_count(pgetnexthighmonotoniccount - ->HighCount); + status = efi.get_next_high_mono_count(&count); if (put_user(status, pgetnexthighmonotoniccount->status)) return -EFAULT; + + if (put_user(count, pgetnexthighmonotoniccount->HighCount)) + return -EFAULT; + if (status != EFI_SUCCESS) return -EINVAL; @@ -322,6 +478,7 @@ static long efi_runtime_query_variableinfo(unsigned long arg) { struct efi_queryvariableinfo __user *pqueryvariableinfo; efi_status_t status; + uint64_t max_storage, remaining, max_size; uint32_t attr; pqueryvariableinfo = (struct efi_queryvariableinfo __user *)arg; @@ -329,10 +486,18 @@ static long efi_runtime_query_variableinfo(unsigned long arg) if (get_user(attr, &pqueryvariableinfo->Attributes)) return -EFAULT; - status = efi.query_variable_info(attr, - pqueryvariableinfo->MaximumVariableStorageSize, - pqueryvariableinfo->RemainingVariableStorageSize - , pqueryvariableinfo->MaximumVariableSize); + status = efi.query_variable_info(attr, &max_storage, + &remaining, &max_size); + + if (put_user(max_storage, pqueryvariableinfo->MaximumVariableStorageSize)) + return -EFAULT; + + if (put_user(remaining, pqueryvariableinfo->RemainingVariableStorageSize)) + return -EFAULT; + + if (put_user(max_size, pqueryvariableinfo->MaximumVariableSize)) + return -EFAULT; + if (put_user(status, pqueryvariableinfo->status)) return -EFAULT; if (status != EFI_SUCCESS) @@ -343,21 +508,46 @@ static long efi_runtime_query_variableinfo(unsigned long arg) static long efi_runtime_query_capsulecaps(unsigned long arg) { - struct efi_querycapsulecapabilities __user *pquerycapsulecapabilities; + struct efi_querycapsulecapabilities __user *u_caps; + struct efi_querycapsulecapabilities caps; + EFI_CAPSULE_HEADER *capsules; efi_status_t status; + uint64_t max_size; + int i, reset_type; - pquerycapsulecapabilities = (struct - efi_querycapsulecapabilities __user *)arg; + u_caps = (struct efi_querycapsulecapabilities __user *)arg; - status = efi.query_capsule_caps( - (efi_capsule_header_t **) - pquerycapsulecapabilities->CapsuleHeaderArray, - pquerycapsulecapabilities->CapsuleCount, - pquerycapsulecapabilities->MaximumCapsuleSize, - (int *)pquerycapsulecapabilities->ResetType); + if (copy_from_user(&caps, u_caps, sizeof(caps))) + return -EFAULT; - if (put_user(status, pquerycapsulecapabilities->status)) + capsules = kcalloc(caps.CapsuleCount + 1, + sizeof(EFI_CAPSULE_HEADER), GFP_KERNEL); + if (!capsules) + return -ENOMEM; + + for (i = 0; i < caps.CapsuleCount; i++) { + if (copy_from_user(&capsules[i], + (EFI_CAPSULE_HEADER *)u_caps->CapsuleHeaderArray[i], + sizeof(EFI_CAPSULE_HEADER))) + return -EFAULT; + } + + caps.CapsuleHeaderArray = &capsules; + + status = efi.query_capsule_caps((efi_capsule_header_t **) + caps.CapsuleHeaderArray, + caps.CapsuleCount, + &max_size, &reset_type); + + if (put_user(status, u_caps->status)) return -EFAULT; + + if (put_user(max_size, u_caps->MaximumCapsuleSize)) + return -EFAULT; + + if (put_user(reset_type, u_caps->ResetType)) + return -EFAULT; + if (status != EFI_SUCCESS) return -EINVAL;