linux-kernel - Re: [PATCH] lib: Always NUL terminate ucs2_as

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160421121827.GE2829@codeblueprint.co.uk>
Date:	Thu, 21 Apr 2016 13:18:27 +0100
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Laszlo Ersek <lersek@...hat.com>
Cc:	Chris Wilson <chris@...is-wilson.co.uk>,
	Peter Jones <pjones@...hat.com>,
	intel-gfx@...ts.freedesktop.org,
	Jason Andryuk <jandryuk@...il.com>,
	Matthew Garrett <mjg59@...eos.com>, linux-efi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8

( 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;