Date: Thu, 21 Apr 2016 11:13:44 -0400
From: Peter Jones <pjones@redhat.com>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Laszlo Ersek <lersek@redhat.com>, Chris Wilson <chris@chris-wilson.co.uk>,
        intel-gfx@lists.freedesktop.org, 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: <20160421151343.GA3763@redhat.com>
References: <1461141427-16361-1-git-send-email-chris@chris-wilson.co.uk>
 <5717834C.6070802@redhat.com>
 <20160421121827.GE2829@codeblueprint.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20160421121827.GE2829@codeblueprint.co.uk>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1236
Lines: 32

On Thu, Apr 21, 2016 at 01:18:27PM +0100, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )

(yep)

> 
> 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)?

I agree with your analysis, and your patch looks plausible.

-- 
  Peter