Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8
From: Chris Wilson
Date: Wed Apr 20 2016 - 05:41:42 EST
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 Wilson, Intel Open Source Technology Centre