Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752209AbcDUMSd (ORCPT ); Thu, 21 Apr 2016 08:18:33 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:36077 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751611AbcDUMSb (ORCPT ); Thu, 21 Apr 2016 08:18:31 -0400 Date: Thu, 21 Apr 2016 13:18:27 +0100 From: Matt Fleming To: Laszlo Ersek Cc: Chris Wilson , Peter Jones , intel-gfx@lists.freedesktop.org, Jason Andryuk , Matthew Garrett , linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8 Message-ID: <20160421121827.GE2829@codeblueprint.co.uk> References: <1461141427-16361-1-git-send-email-chris@chris-wilson.co.uk> <5717834C.6070802@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5717834C.6070802@redhat.com> User-Agent: Mutt/1.5.24+41 (02bc14ed1569) (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2613 Lines: 68 ( 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;