Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S933354AbcDTJlj (ORCPT <rfc822;w@1wt.eu>);
	Wed, 20 Apr 2016 05:41:39 -0400
Received: from mail.fireflyinternet.com ([87.106.93.118]:60055 "EHLO
	fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org
	with ESMTP id S932079AbcDTJli (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 20 Apr 2016 05:41:38 -0400
X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138;
Date: Wed, 20 Apr 2016 10:41:33 +0100
From: Chris Wilson <chris@chris-wilson.co.uk>
To: Laszlo Ersek <lersek@redhat.com>
Cc: Peter Jones <pjones@redhat.com>, intel-gfx@lists.freedesktop.org,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Jason Andryuk <jandryuk@gmail.com>, Matthew Garrett <mjg59@coreos.com>,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8
Message-ID: <20160420094133.GF14602@nuc-i3427.alporthouse.com>
Mail-Followup-To: Chris Wilson <chris@chris-wilson.co.uk>,
	Laszlo Ersek <lersek@redhat.com>, Peter Jones <pjones@redhat.com>,
	intel-gfx@lists.freedesktop.org,
	Matt Fleming <matt@codeblueprint.co.uk>,
	Jason Andryuk <jandryuk@gmail.com>,
	Matthew Garrett <mjg59@coreos.com>, linux-efi@vger.kernel.org,
	linux-kernel@vger.kernel.org
References: <1461141427-16361-1-git-send-email-chris@chris-wilson.co.uk>
 <57174DA5.6030602@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <57174DA5.6030602@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1923
Lines: 54

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