From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laszlo Ersek Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8 Date: Wed, 20 Apr 2016 14:45:04 +0200 Message-ID: <571779D0.1040508@redhat.com> References: <1461141427-16361-1-git-send-email-chris@chris-wilson.co.uk> <57174DA5.6030602@redhat.com> <20160420094133.GF14602@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160420094133.GF14602@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Peter Jones , intel-gfx@lists.freedesktop.org, Matt Fleming , Jason Andryuk , Matthew Garrett , linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-efi@vger.kernel.org T24gMDQvMjAvMTYgMTE6NDEsIENocmlzIFdpbHNvbiB3cm90ZToKPiBPbiBXZWQsIEFwciAyMCwg MjAxNiBhdCAxMTozNjozN0FNICswMjAwLCBMYXN6bG8gRXJzZWsgd3JvdGU6Cj4+IE9uIDA0LzIw LzE2IDEwOjM3LCBDaHJpcyBXaWxzb24gd3JvdGU6Cj4+PiBJZiB0aGUgY2FsbGVyLCBpbiB0aGlz IGNhc2UgZWZpdmFyZnNfY2FsbGJhY2soKSwgb25seSBwcm92aWRlcyBzdWZmaWNlbnQKPj4+IHJv b20gZm9yIHRoZSBleHBhbmRlZCB1dGY4IGFuZCBub3QgZW5vdWdoIHRvIGluY2x1ZGUgdGhlIHRl cm1pbmF0aW5nIE5VTAo+Pj4gYnl0ZSwgdGhhdCBOVUwgYnl0ZSBpcyBza2lwcGVkLgo+Pgo+PiBI b3cgZG9lcyB0aGF0IG9jY3VyPyBJbiBlZml2YXJmc19jYWxsYmFjaygpIFtmcy9lZml2YXJmcy9z dXBlci5jXSwgd2UgaGF2ZQo+Pgo+PiAJbGVuID0gdWNzMl91dGY4c2l6ZShlbnRyeS0+dmFyLlZh cmlhYmxlTmFtZSk7Cj4+Cj4+IAkvKiBuYW1lLCBwbHVzICctJywgcGx1cyBHVUlELCBwbHVzIE5V TCovCj4+IAluYW1lID0ga21hbGxvYyhsZW4gKyAxICsgRUZJX1ZBUklBQkxFX0dVSURfTEVOICsg MSwgR0ZQX0tFUk5FTCk7Cj4+IAlpZiAoIW5hbWUpCj4+IAkJZ290byBmYWlsOwo+Pgo+PiAJdWNz Ml9hc191dGY4KG5hbWUsIGVudHJ5LT52YXIuVmFyaWFibGVOYW1lLCBsZW4pOwo+Pgo+PiBJbnN0 ZWFkLCBJIHRoaW5rIHRoZSBmb2xsb3dpbmcgbWlnaHQgYmUgaGFwcGVuaW5nIChub3RlIHRoYXQg UklQIHBvaW50cwo+PiBpbnRvIGVmaXZhcl92YXJpYWJsZV9pc19yZW1vdmFibGUoKSwgYW5kIEkg Z3Vlc3MgdmFyaWFibGVfbWF0Y2hlcygpCj4+ICh3aGljaCBpcyBzdGF0aWMpIGlzIGlubGluZWQp Ogo+Pgo+PiBlZml2YXJmc19jYWxsYmFjaygpICAgICAgICAgICAgICBbZnMvZWZpdmFyZnMvc3Vw ZXIuY10KPj4gICBlZml2YXJfdmFyaWFibGVfaXNfcmVtb3ZhYmxlKCkgW2RyaXZlcnMvZmlybXdh cmUvZWZpL3ZhcnMuY10KPj4gICAgIHZhcmlhYmxlX21hdGNoZXMoKSAgICAgICAgICAgW2RyaXZl cnMvZmlybXdhcmUvZWZpL3ZhcnMuY10KPj4KPj4gVGhlIGJ1ZyBzZWVtcyB0byBiZSBpbiB2YXJp YWJsZV9tYXRjaGVzKCksIHdoaWNoIGRvZXNuJ3QgY29uc2lkZXIgdGhlCj4+ICJsZW4iIHBhcmFt ZXRlciBlYXJseSBlbm91Z2guIE5hbWVseSwgY29uc2lkZXIgdGhhdCB3ZSBoYXZlIHRoZQo+PiBm b2xsb3dpbmcgaW5wdXQ6Cj4+Cj4+IC0gdmFyX25hbWU6ICJhIgo+PiAtIGxlbjogMQo+PiAtIG1h dGNoX25hbWUgImFiIgo+Pgo+PiBJbiB0aGUgZmlyc3QgaXRlcmF0aW9uIG9mIHRoZSBsb29wIChp LmUuLCAqbWF0Y2ggPT0gMCk6Cj4+IC0gYyA9ICdhJwo+PiAtIHUgPSAnYScKPj4gLSAqbWF0Y2gg Z2V0cyBpbmNyZW1lbnRlZCB0byAxLgo+Pgo+PiBJbiB0aGUgc2Vjb25kIGl0ZXJhdGlvbiBvZiB0 aGUgbG9vcCAoaS5lLiwgKm1hdGNoID09IDEpOgo+PiAtIGMgPSAnYicKPj4gLSB1ID0gPGluZGV0 ZXJtaW5hdGUgdmFsdWU+ICh0aGF0IGlzLCB1bmRlZmluZWQgYmVoYXZpb3IpLAo+PiAgIGJlY2F1 c2UgKCptYXRjaCA9PSBsZW4pLgo+Pgo+PiBUaGlzIHNlZW1zIHRvIGJlIGNvbnNpc3RlbnQgd2l0 aCB0aGUgZXJyb3IgbWVzc2FnZSAiQ2F1Z2h0IDgtYml0IHJlYWQKPj4gZnJvbSB1bmluaXRpYWxp emVkIG1lbW9yeSI6IG5hbWVseSwgdGhlIGFycmF5IGFsbG9jYXRlZCBmb3IgIm5hbWUiIGluCj4+ IGVmaXZhcmZzX2NhbGxiYWNrKCkgaXMgaW5kZWVkIG5vdCBwcmUtemVyb2VkLCBhbmQgdGhlIHVj czJfYXNfdXRmOCgpCj4+IGZ1bmN0aW9uIGRvZXMgbm90IHBvcHVsYXRlIG5hbWVbbGVuXSAtLSBj b3JyZWN0bHksIEkgd291bGQgc2F5Lgo+IAo+IHVjczJfYXNfdXRmOCByZXBvcnRzIHRoYXQgaXQg cmV0dXJucyBhIE5VTCB0ZXJtaW5hdGVkIHN0cmluZy4KCkkgZG9uJ3QgdGhpbmsgaXQgZG9lcy4g SGVyZSdzIHRoZSBjb21tZW50OgoKLyoKICogY29weSBhdCBtb3N0IG1heGxlbmd0aCBieXRlcyBv ZiB3aG9sZSB1dGY4IGNoYXJhY3RlcnMgdG8gZGVzdCBmcm9tIHRoZQogKiB1Y3MyIHN0cmluZyBz cmMuCiAqCiAqIFRoZSByZXR1cm4gdmFsdWUgaXMgdGhlIG51bWJlciBvZiBjaGFyYWN0ZXJzIGNv cGllZCwgbm90IGluY2x1ZGluZyB0aGUKICogZmluYWwgTlVMIGNoYXJhY3Rlci4KICovCgpJdCBk b2Vzbid0IHNlZW0gdG8gcHJvbWlzZSB0aGF0IHRoZSBvdXRwdXQgd2lsbCBhbHdheXMgYmUgTlVM LXRlcm1pbmF0ZWQuIEFuZCwgdGhlIGNvZGUgZXhwbGljaXRseSBjb25zaWRlcnMgdGhlIGNhc2Ug d2hlbiB0aGVyZSBpcyBubyByb29tIGZvciB0aGUgZmluYWwgTlVMLgoKQSBzdHJpY3RseSBOVUwt dGVybWluYXRlZCBvdXRwdXQgbWlnaHQgbWFrZSBmb3IgYSBiZXR0ZXIgaW50ZXJmYWNlLCBidXQg dGhlbiB0aGUgY29tbWVudCBzaG91bGQgYmUgdXBkYXRlZCBhcyB3ZWxsLiBQbHVzLCBJJ20gdW5z dXJlIGlmIGl0IHdvdWxkIGJlIGFsaWduZWQgd2l0aCBQZXRlcidzIG9yaWdpbmFsIGdvYWwgKGFu ZCB0aGUgY3VycmVudCBjYWxsIHNpdGVzKS4gSSdtIG5vdCBhZ2FpbnN0IGNoYW5naW5nIHRoZSBp bnRlcmZhY2UgY29udHJhY3Q7IEknbGwgbGV0IFBldGVyIHNwZWFrIHVwLgoKPiBJdCBkaWRuJ3QK PiBpbiB0aGlzIGNhc2UuCj4gLUNocmlzCj4gCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755008AbcDTMpM (ORCPT ); Wed, 20 Apr 2016 08:45:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36280 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751791AbcDTMpI (ORCPT ); Wed, 20 Apr 2016 08:45:08 -0400 Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8 To: Chris Wilson , Peter Jones , intel-gfx@lists.freedesktop.org, Matt Fleming , Jason Andryuk , Matthew Garrett , 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> <20160420094133.GF14602@nuc-i3427.alporthouse.com> From: Laszlo Ersek Message-ID: <571779D0.1040508@redhat.com> Date: Wed, 20 Apr 2016 14:45:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160420094133.GF14602@nuc-i3427.alporthouse.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/20/16 11:41, Chris Wilson wrote: > 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 = (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. I don't think it does. Here's the comment: /* * copy at most maxlength bytes of whole utf8 characters to dest from the * ucs2 string src. * * The return value is the number of characters copied, not including the * final NUL character. */ It doesn't seem to promise that the output will always be NUL-terminated. And, the code explicitly considers the case when there is no room for the final NUL. A strictly NUL-terminated output might make for a better interface, but then the comment should be updated as well. Plus, I'm unsure if it would be aligned with Peter's original goal (and the current call sites). I'm not against changing the interface contract; I'll let Peter speak up. > It didn't > in this case. > -Chris >