Hi Ivan, On Mon, Aug 01, 2016 at 09:48:16AM +0800, Ivan Hu wrote: > This driver is used by the Firmware Test Suite (FWTS) for testing the UEFI > runtime interfaces readiness of the firmware. > > This driver exports UEFI runtime service interfaces into userspace, > which allows to use and test UEFI runtime services provided by the > firmware. > > This driver uses the efi.<service> function pointers directly instead of > going through the efivar API to allow for direct testing of the UEFI > runtime service interfaces provided by the firmware. > > Details for FWTS are available from, > <https://wiki.ubuntu.com/FirmwareTestSuite> > > Signed-off-by: Ivan Hu <ivan.hu@xxxxxxxxxxxxx> > --- > MAINTAINERS | 6 + > drivers/firmware/efi/Kconfig | 15 + > drivers/firmware/efi/Makefile | 1 + > drivers/firmware/efi/efi_test/Makefile | 1 + > drivers/firmware/efi/efi_test/efi_test.c | 726 +++++++++++++++++++++++++++++++ > drivers/firmware/efi/efi_test/efi_test.h | 110 +++++ > 6 files changed, 859 insertions(+) > create mode 100644 drivers/firmware/efi/efi_test/Makefile > create mode 100644 drivers/firmware/efi/efi_test/efi_test.c > create mode 100644 drivers/firmware/efi/efi_test/efi_test.h > [...snip] > index 0000000..18931ba > --- /dev/null > +++ b/drivers/firmware/efi/efi_test/efi_test.c > @@ -0,0 +1,726 @@ > +/* > + * EFI Test Driver for Runtime Services > + * > + * Copyright(C) 2012-2016 Canonical Ltd. > + * > + * This driver exports EFI runtime services interfaces into userspace, which > + * allow to use and test UEFI runtime services provided by firmware. > + * I have a suggestion to declare the GPL license in file description. I am not sure that the GPL in file description is a must have, because the efi-pstore.c also doesn't declare GPL in file description. Maybe 'MODULE_LICENSE("GPL")' is enough? You can consider to add it. > + */ > + > +#include <linux/version.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/proc_fs.h> > +#include <linux/efi.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +#include "efi_test.h" > + > +MODULE_AUTHOR("Ivan Hu <ivan.hu@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("EFI Test Driver"); > +MODULE_LICENSE("GPL"); > + > +/* commit 83e681897 turned efi_enabled into a function, so abstract it */ > +#ifdef EFI_RUNTIME_SERVICES > +#define EFI_RUNTIME_ENABLED efi_enabled(EFI_RUNTIME_SERVICES) > +#else > +#define EFI_RUNTIME_ENABLED efi_enabled > +#endif > + > +/* > + * 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 __user *str) > +{ > + uint16_t *s = str, c; > + size_t len; > + > + if (!str) > + return 0; > + > + /* Include terminating NULL */ > + len = sizeof(uint16_t); > + > + if (get_user(c, s++)) { > + WARN(1, "efi_test: Can't read userspace memory for size"); > + return 0; > + } > + > + while (c != 0) { > + if (get_user(c, s++)) { > + WARN(1, "efi_test: Can't read userspace memory for size"); > + return 0; > + } > + len += sizeof(uint16_t); > + } > + return len; > +} > + Looks __ucs2_strsize will return zero size when str is zero or when it can not read userspace memory. Then please see below... > +/* > + * Allocate a buffer and copy a ucs2 string from user space into it. > + */ > +static inline int > +copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) > +{ > + uint16_t *buf; > + > + if (!src) { > + *dst = NULL; > + return 0; > + } > + > + if (!access_ok(VERIFY_READ, src, 1)) > + return -EFAULT; > + > + buf = kmalloc(len, GFP_KERNEL); > + if (!buf) { > + *dst = NULL; > + return -ENOMEM; > + } > + *dst = buf; > + > + if (copy_from_user(*dst, src, len)) { > + kfree(buf); > + return -EFAULT; > + } > + > + return 0; > +} If the input len is zero, then kmalloc returns ZERO_SIZE_PTR, it causes ZERO_SIZE_PTR dereference in later copy_from_user(). > + > +/* > + * Count the bytes in 'str', including the terminating NULL. > + * > + * Just a wrap for __ucs2_strsize > + */ > +static inline int get_ucs2_strsize_from_user(uint16_t __user *src, size_t *len) > +{ > + if (!access_ok(VERIFY_READ, src, 1)) > + return -EFAULT; > + > + *len = __ucs2_strsize(src); > + if (*len == 0) > + 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; The above code prevents src is zero. > + > + len = __ucs2_strsize(src); > + return copy_ucs2_from_user_len(dst, src, len); > +} > + The __ucs2_strsize may returns zero when it can not read userspace memory, why not check that len should not be zero like get_ucs2_strsize_from_user() does? > +/* > + * 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 *getvariable; > + struct efi_getvariable getvariable_local; > + unsigned long datasize, prev_datasize, *dz; > + efi_guid_t vendor_guid, *vd = NULL; > + efi_status_t status; > + uint16_t *name = NULL; > + uint32_t attr, *at; > + void *data = NULL; > + int rv = 0; > + > + getvariable = (struct efi_getvariable __user *)arg; > + > + if (copy_from_user(&getvariable_local, getvariable, > + sizeof(getvariable_local))) > + return -EFAULT; > + if (getvariable_local.data_size && > + get_user(datasize, getvariable_local.data_size)) > + return -EFAULT; > + if (getvariable_local.vendor_guid) { > + if (copy_from_user(&vendor_guid, getvariable_local.vendor_guid, > + sizeof(vendor_guid))) > + return -EFAULT; > + vd = &vendor_guid; > + } > + > + if (getvariable_local.variable_name) { > + rv = copy_ucs2_from_user(&name, > + getvariable_local.variable_name); > + if (rv) > + return rv; > + } > + > + at = getvariable_local.attributes ? &attr : NULL; > + dz = getvariable_local.data_size ? &datasize : NULL; > + > + if (getvariable_local.data_size && getvariable_local.data) { > + data = kmalloc(datasize, GFP_KERNEL); > + if (!data) { > + kfree(name); > + return -ENOMEM; > + } > + } > + > + prev_datasize = datasize; > + status = efi.get_variable(name, vd, at, dz, data); > + kfree(name); > + > + if (data) { > + if (status == EFI_SUCCESS && prev_datasize >= datasize) > + rv = copy_to_user(getvariable_local.data, data, > + datasize); > + kfree(data); > + } > + > + if (rv) > + return rv; > + > + if (put_user(status, getvariable_local.status)) > + return -EFAULT; > + if (status == EFI_SUCCESS && prev_datasize >= datasize) { > + if (at && put_user(attr, getvariable_local.attributes)) > + return -EFAULT; > + if (dz && put_user(datasize, getvariable_local.data_size)) > + return -EFAULT; > + return 0; > + } else if (status == EFI_BUFFER_TOO_SMALL) { > + if (dz && put_user(datasize, getvariable_local.data_size)) > + return -EFAULT; > + return -EINVAL; > + } else { > + return -EINVAL; > + } > + > + return 0; > +} > + > +static long efi_runtime_set_variable(unsigned long arg) > +{ > + struct efi_setvariable __user *setvariable; > + struct efi_setvariable setvariable_local; > + efi_guid_t vendor_guid; > + efi_status_t status; > + uint16_t *name; > + void *data; > + int rv; > + > + setvariable = (struct efi_setvariable __user *)arg; > + > + if (copy_from_user(&setvariable_local, setvariable, > + sizeof(setvariable_local))) > + return -EFAULT; > + if (copy_from_user(&vendor_guid, setvariable_local.vendor_guid, > + sizeof(vendor_guid))) > + return -EFAULT; > + > + rv = copy_ucs2_from_user(&name, setvariable_local.variable_name); > + if (rv) > + return rv; In efi_runtime_get_variable(), it checks variable_name input. Why doesn't check here? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
