[PATCH][v3] efi_runtime: ensure we don't allocate a zero byte buffer (LP: #1429890)]

[Colin King]

From: Colin Ian King <colin.king at canonical.com>

When requesting a test on a zero sized buffer we must ensure that a
buffer is allocated and is of zero size when passed to the EFI service.

If we kalloc zero bytes, then a ZERO_SIZE_PTR is returned, which has
no meaning to the EFI service, so one strategy is to convert this to
a NULL before passing it to the EFI service. However, this is handled
by the service as an invalid parameter and returns with
EFI_INVALID_PARAMETER rather than EFI_BUFFER_TOO_SMALL (the latter
being what we are trying to actually exercise).

The best way forward is to allocate a buffer that is 1 wide char too
long, and return back the buffer offset by 1 wide char (i.e skipping
the leading 2 bytes). This allows us to always return a valid sized
buffer that is passed to the EFI service, even with zero length requests.
For zero sized buffers, if EFI writes into the zero sized buffer it will
corrupt the member following the buffer.  I believe further checks should
probably be added to also sanity check for these potential buffer overruns.

Across various platforms this arrangement trips the expected
EFI_BUFFER_TOO_SMALL error return, as originally expected in the fwts userspace
test case.

Signed-off-by: Colin Ian King <colin.king at canonical.com>
---
 efi_runtime/efi_runtime.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
index ff31685..2708b4b 100644
--- a/efi_runtime/efi_runtime.c
+++ b/efi_runtime/efi_runtime.c
@@ -133,6 +133,15 @@ static inline size_t __ucs2_strsize(uint16_t  __user *str)
 }
 
 /*
+ * Free a buffer allocated by copy_ucs2_from_user_len()
+ */
+static inline void ucs2_kfree(uint16_t *buf)
+{
+	if (buf)
+		kfree(buf - 1);
+}
+
+/*
  * 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
@@ -141,11 +150,22 @@ static inline size_t __ucs2_strsize(uint16_t  __user *str)
  *
  * If a non-zero value is returned, the caller MUST NOT access 'dst'.
  *
- * It is the caller's responsibility to free 'dst'.
+ * It is the caller's responsibility to free 'dst' using ucs2_kfree()
+ *
+ * We cater for zero sized len by always allocating a buffer that is 1
+ * uint16_t larger than the requested size and passing back the buffer
+ * offset by 1 uint16_t.  That way, the returned buffer size is the
+ * size that is requested and the buffer ptr is a valid pointer (and not
+ * NULL or ZERO_SIZE_PTR).  This allows EFI services to be passed a valid
+ * allocated buffer of zero length size and to see if the services handle
+ * this as an EFI_BUFFER_TOO_SMALL error.
+ * 
  */
 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;
@@ -154,12 +174,15 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len)
 	if (!access_ok(VERIFY_READ, src, 1))
 		return -EFAULT;
 
-	*dst = kmalloc(len, GFP_KERNEL);
-	if (!*dst)
+	buf = kmalloc(len + sizeof(uint16_t), GFP_KERNEL);
+	if (!buf) {
+		*dst = 0;
 		return -ENOMEM;
+	}
+	*dst = buf + 1;
 
 	if (copy_from_user(*dst, src, len)) {
-		kfree(*dst);
+		kfree(buf);
 		return -EFAULT;
 	}
 
@@ -242,14 +265,13 @@ static long efi_runtime_get_variable(unsigned long arg)
 
 	data = kmalloc(datasize, GFP_KERNEL);
 	if (!data) {
-		kfree(name);
+		ucs2_kfree(name);
 		return -ENOMEM;
 	}
 
 	prev_datasize = datasize;
 	status = efi.get_variable(name, &vendor, &attr, &datasize, data);
-
-	kfree(name);
+	ucs2_kfree(name);
 
 	if (status == EFI_SUCCESS && prev_datasize >= datasize)
 		rv = copy_to_user(pgetvariable_local.Data, data, datasize);
@@ -301,12 +323,12 @@ static long efi_runtime_set_variable(unsigned long arg)
 
 	data = kmalloc(psetvariable_local.DataSize, GFP_KERNEL);
 	if (!data) {
-		kfree(name);
+		ucs2_kfree(name);
 		return -ENOMEM;
 	}
 	if (copy_from_user(data, psetvariable_local.Data,
 			   psetvariable_local.DataSize)) {
-		kfree(data);
+		ucs2_kfree(data);
 		kfree(name);
 		return -EFAULT;
 	}
@@ -315,7 +337,7 @@ static long efi_runtime_set_variable(unsigned long arg)
 				  psetvariable_local.DataSize, data);
 
 	kfree(data);
-	kfree(name);
+	ucs2_kfree(name);
 
 	if (put_user(status, psetvariable_local.status))
 		return -EFAULT;
@@ -469,7 +491,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
 	if (status == EFI_SUCCESS && prev_name_size >= name_size)
 		rv = copy_ucs2_to_user_len(pgetnextvariablename_local.VariableName,
 					   name, name_size);
-	kfree(name);
+	ucs2_kfree(name);
 
 	if (rv)
 		return -EFAULT;
-- 
2.1.4