EFI: re-check {get,set}-variable name strings after copying in

A malicious guest given permission to invoke XENPF_efi_runtime_call may
play with the strings underneath Xen sizing them and copying them in.
Guard against this by re-checking the copyied in data for consistency
with the initial sizing. At the same time also check that the actual
copy-in is in fact successful, and switch to the lighter weight non-
checking flavor of the function.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b9f461505c152f6fa187750935b54c89a8abefe2..a6f84c945aad69e5c152685603320c22c265f02c 100644 (file)
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -281,16 +281,6 @@ static int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n)
     return n ? *s1 - *s2 : 0;
 }
 
-static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n)
-{
-    while ( n && *s != c )
-    {
-        --n;
-        ++s;
-    }
-    return n ? s : NULL;
-}
-
 static CHAR16 *__init s2w(union string *str)
 {
     const char *s = str->s;

diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index 6b9c56ead1a4bdedaf7d8a954bf0738dbe9ee2eb..2e38d05f3d4bfbc278edb22db8a61a4a3d607222 100644 (file)
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -39,3 +39,5 @@ extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size,
 
 extern UINT64 efi_apple_properties_addr;
 extern UINTN efi_apple_properties_len;
+
+const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 8c2ece468de97c7448b0ef0cad8b440de8cbae51..752e6043902902b3349dc1284d9a77fd4d5ec423 100644 (file)
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -194,7 +194,18 @@ void efi_reset_system(bool warm)
 }
 
 #endif /* CONFIG_ARM */
-#endif
+
+const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n)
+{
+    while ( n && *s != c )
+    {
+        --n;
+        ++s;
+    }
+    return n ? s : NULL;
+}
+
+#endif /* COMPAT */
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
 int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
@@ -465,7 +476,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
         name = xmalloc_array(CHAR16, ++len);
         if ( !name )
            return -ENOMEM;
-        __copy_from_guest(name, op->u.get_variable.name, len);
+        if ( __copy_from_guest(name, op->u.get_variable.name, len) ||
+             wmemchr(name, 0, len) != name + len - 1 )
+        {
+            xfree(name);
+            return -EIO;
+        }
 
         size = op->u.get_variable.size;
         if ( size )
@@ -513,7 +529,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
         name = xmalloc_array(CHAR16, ++len);
         if ( !name )
            return -ENOMEM;
-        __copy_from_guest(name, op->u.set_variable.name, len);
+        if ( __copy_from_guest(name, op->u.set_variable.name, len) ||
+             wmemchr(name, 0, len) != name + len - 1 )
+        {
+            xfree(name);
+            return -EIO;
+        }
 
         data = xmalloc_bytes(op->u.set_variable.size);
         if ( !data )