| Message ID | 1396535003-28253-5-git-send-email-matt@console-pimps.org |
|---|---|
| State | Rejected |
| Headers | show |
On 03/04/14 15:23, Matt Fleming wrote: > From: Matt Fleming <matt.fleming@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@alien8.de> > Signed-off-by: Matt Fleming <matt.fleming@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@canonical.com>
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;