From mboxrd@z Thu Jan 1 00:00:00 1970 From: joeyli Subject: Re: [PATCH V2] efi: add efi_test driver for exporting UEFI runtime service interfaces Date: Thu, 4 Aug 2016 19:08:21 +0800 Message-ID: <20160804110821.GM16090@linux-rxt1.site> References: <1470016096-5063-1-git-send-email-ivan.hu@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1470016096-5063-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ivan Hu Cc: matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org, linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-efi@vger.kernel.org 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. 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, > > > Signed-off-by: Ivan Hu > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "efi_test.h" > + > +MODULE_AUTHOR("Ivan Hu "); > +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