ACK: [PATCH 4/4] efi_runtime: Do not pass user addresses to firmware
Colin Ian King
colin.king at canonical.com
Thu Apr 3 16:37:37 UTC 2014
On 03/04/14 15:23, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming at intel.com>
>
> 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 <bp at alien8.de>
> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> ---
> efi_runtime/efi_runtime.c | 239 +++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 215 insertions(+), 24 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index ffe107341470..d90d24eb151b 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -24,7 +24,7 @@
> #include <linux/init.h>
> #include <linux/proc_fs.h>
> #include <linux/efi.h>
> -
> +#include <linux/slab.h>
> #include <linux/uaccess.h>
>
> #include "efi_runtime.h"
> @@ -100,6 +100,110 @@ 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 __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;
> +}
> +
> +/*
> + * Copy a ucs2 string from user space into a newly allocated kernel
> + * buffer.
> + *
> + * We take an explicit number of bytes to copy, and therefore do not
> + * make any assumptions about 'src' (such as it being a valid string).
> + */
> +static inline int
> +get_ucs2_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;
> +}
> +
> +/*
> + * Copy a ucs2 string from user space into a newly allocated kernel
> + * buffer.
> + *
> + * If a non-zero value is returned, the caller MUST NOT access 'dst'.
> + */
> +static inline int get_ucs2(uint16_t **dst, uint16_t __user *src)
> +{
> + size_t len;
> +
> + if (!access_ok(VERIFY_READ, src, 1))
> + return -EFAULT;
> +
> + len = __strsize(src);
> + return get_ucs2_len(dst, src, len);
> +}
> +
> +/*
> + * Write a ucs2 string to a user buffer.
> + *
> + * 'len' specifies the number of bytes to copy.
> + */
> +static inline int
> +put_ucs2_len(uint16_t *src, uint16_t __user *dst, size_t len)
> +{
> + if (!src)
> + return 0;
> +
> + if (!access_ok(VERIFY_WRITE, dst, 1))
> + return -EFAULT;
> +
> + return copy_to_user(dst, src, len);
> +}
> +
> +/*
> + * Write a NUL-terminated ucs2 string to a user buffer.
> + *
> + * We calculate the number of bytes to write from the ucs2 string 'src',
> + * including the terminating NUL.
> + */
> +static inline int put_ucs2(uint16_t *src, uint16_t __user *dst)
> +{
> + size_t len;
> +
> + if (!access_ok(VERIFY_WRITE, dst, 1))
> + return -EFAULT;
> +
> + len = __strsize(src);
> + return put_ucs2_len(src, dst, len);
> +}
> +
> static long efi_runtime_get_variable(unsigned long arg)
> {
> struct efi_getvariable __user *pgetvariable;
> @@ -107,7 +211,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 +224,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 = get_ucs2(&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 +267,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 +280,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 = get_ucs2(&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 +408,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 +424,18 @@ 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 = get_ucs2_len(&name, pgetnextvariablename->VariableName, 1024);
> + if (rv)
> + return rv;
> +
> + status = efi.get_next_variable(&name_size, name, &vendor);
> +
> + rv = put_ucs2_len(name, pgetnextvariablename->VariableName, name_size);
> + kfree(name);
> +
> + if (rv)
> + return -EFAULT;
> +
> if (put_user(status, pgetnextvariablename->status))
> return -EFAULT;
> convert_to_guid(&vendor, &vendor_guid);
> @@ -302,14 +455,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 +479,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 +487,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 +509,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;
> +
> + 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;
>
> - if (put_user(status, pquerycapsulecapabilities->status))
> + 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;
>
>
Thanks for this fix. I've also given it a test and it works fine on the
various kernels and pieces of UEFI kit I have, so..
Acked-by: Colin Ian King <colin.king at canonical.com>
More information about the fwts-devel
mailing list