Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8

[Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]

( Good Lord, I hate doing string manipulation in C )

On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
> 
> So, "len" does not include the room for the terminating NUL-byte here.
> When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
> a NUL byte will be produced in "name", but it will be at the price of a
> genuine character from the input variable name.

Right, and this is a problem because we're trying to keep the names
consistent between efivarfs and the EFI variable data. Force
NUL-terminating the string is wrong, because if you have no room for
the NUL the caller should check for that. Sadly none do.

On the flip-side, passing around non-NUL terminated strings is just
begging for these kinds of issues to come up.

The fact is that the callers of ucs2_as_utf8() are passing it the
wrong 'len' argument. We want a NUL-terminated utf8 string and we're
passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
has enough room to copy the NUL.

Wouldn't this work (minus the return value checking)?

---

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 0ac594c0a234..13a837c70e90 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -235,13 +235,12 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 	unsigned long utf8_size;
 	u8 *utf8_name;
 
-	utf8_size = ucs2_utf8size(var_name);
-	utf8_name = kmalloc(utf8_size + 1, GFP_KERNEL);
+	utf8_size = ucs2_utf8size(var_name) + 1;
+	utf8_name = kmalloc(utf8_size, GFP_KERNEL);
 	if (!utf8_name)
 		return false;
 
 	ucs2_as_utf8(utf8_name, var_name, utf8_size);
-	utf8_name[utf8_size] = '\0';
 
 	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
 		const char *name = variable_validate[i].name;
@@ -250,7 +249,7 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		if (efi_guidcmp(vendor, variable_validate[i].vendor))
 			continue;
 
-		if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+		if (variable_matches(utf8_name, utf8_size, name, &match)) {
 			if (variable_validate[i].validate == NULL)
 				break;
 			kfree(utf8_name);
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index dd029d13ea61..be5a02721b41 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -136,7 +136,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	if (!name)
 		goto fail;
 
-	ucs2_as_utf8(name, entry->var.VariableName, len);
+	ucs2_as_utf8(name, entry->var.VariableName, len + 1);
 
 	if (efivar_variable_is_removable(entry->var.VendorGuid, name, len))
 		is_removable = true;