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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 20 Apr 2016 10:41:33 +0100
From:	Chris Wilson <chris@...is-wilson.co.uk>
To:	Laszlo Ersek <lersek@...hat.com>
Cc:	Peter Jones <pjones@...hat.com>, intel-gfx@...ts.freedesktop.org,
	Matt Fleming <matt@...eblueprint.co.uk>,
	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

On Wed, Apr 20, 2016 at 11:36:37AM +0200, Laszlo Ersek wrote:
> On 04/20/16 10:37, Chris Wilson wrote:
> > If the caller, in this case efivarfs_callback(), only provides sufficent
> > room for the expanded utf8 and not enough to include the terminating NUL
> > byte, that NUL byte is skipped.
> 
> How does that occur? In efivarfs_callback() [fs/efivarfs/super.c], we have
> 
> 	len = ucs2_utf8size(entry->var.VariableName);
> 
> 	/* name, plus '-', plus GUID, plus NUL*/
> 	name = kmalloc(len + 1 + EFI_VARIABLE_GUID_LEN + 1, GFP_KERNEL);
> 	if (!name)
> 		goto fail;
> 
> 	ucs2_as_utf8(name, entry->var.VariableName, len);
> 
> Instead, I think the following might be happening (note that RIP points
> into efivar_variable_is_removable(), and I guess variable_matches()
> (which is static) is inlined):
> 
> efivarfs_callback()              [fs/efivarfs/super.c]
>   efivar_variable_is_removable() [drivers/firmware/efi/vars.c]
>     variable_matches()           [drivers/firmware/efi/vars.c]
> 
> The bug seems to be in variable_matches(), which doesn't consider the
> "len" parameter early enough. Namely, consider that we have the
> following input:
> 
> - var_name: "a"
> - len: 1
> - match_name "ab"
> 
> In the first iteration of the loop (i.e., *match == 0):
> - c = 'a'
> - u = 'a'
> - *match gets incremented to 1.
> 
> In the second iteration of the loop (i.e., *match == 1):
> - c = 'b'
> - u = <indeterminate value> (that is, undefined behavior),
>   because (*match == len).
> 
> This seems to be consistent with the error message "Caught 8-bit read
> from uninitialized memory": namely, the array allocated for "name" in
> efivarfs_callback() is indeed not pre-zeroed, and the ucs2_as_utf8()
> function does not populate name[len] -- correctly, I would say.

ucs2_as_utf8 reports that it returns a NUL terminated string. It didn't
in this case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre