From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 16 Jul 2019 15:02:07 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 394E33082B3F; Tue, 16 Jul 2019 22:02:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-187.ams2.redhat.com [10.36.117.187]) by smtp.corp.redhat.com (Postfix) with ESMTP id F1355600C7; Tue, 16 Jul 2019 22:02:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 0/3] MdePkg, OvmfPkg: rewrite Base64Decode(), clean up call site From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Ard Biesheuvel , Jordan Justen , Liming Gao , =?UTF-8?Q?Marvin_H=c3=a4user?= , Michael D Kinney , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Zhichao Gao Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20190702102836.27589-1-lersek@redhat.com> Message-ID: Date: Wed, 17 Jul 2019 00:02:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190702102836.27589-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Tue, 16 Jul 2019 22:02:07 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/02/19 12:28, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: base64_decode_bz1891 > > Base64Decode() has a number of issues; see > > - > > - and the mailing list discussion linked from > . > > In my opinion, rewriting Base64Decode() from scratch, using a different > (state machine-based) approach is safer / more robust than attempting t= o > identify and patch up individual problems in the current implementation= . > The emphasis of the proposed implementation is to reject invalid input; > decoding valid input is kind of secondary. (This is the safe approach > for all parsers that process untrusted input, in my opinion.) > > My understanding is that unit tests for Base64Decode() already exist in > some repository. While I tested the new implementation through OvmfPkg'= s > EnrollDefaultKeys application -- which makes the sole calls to > Base64Decode() in the open source edk2 tree -- I didn't run a unit test > suite. Help with that (pointers to the test suite, or actual unit > testing) would be highly appreciated. > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Liming Gao > Cc: Marvin H=C3=A4user > Cc: Michael D Kinney > Cc: Philippe Mathieu-Daud=C3=A9 > Cc: Zhichao Gao > > Thanks > Laszlo > > Laszlo Ersek (3): > MdePkg/BaseLib: re-specify Base64Decode(), and add temporary stub imp= l > MdePkg/BaseLib: rewrite Base64Decode() > OvmfPkg/EnrollDefaultKeys: clean up Base64Decode() retval handling > > MdePkg/Include/Library/BaseLib.h | 99 ++++- > MdePkg/Library/BaseLib/String.c | 448 +++++++++++++-----= -- > OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 10 +- > 3 files changed, 374 insertions(+), 183 deletions(-) > Thank you all for the feedback! * Added a paragraph to the commit message of patch#2, as requested by Phil, based on the discussion with Marvin: + The intent is to only strengthen the checks (sanity and input) r= elative to + the previous implementation, hence the MAX_ADDRESS checks are re= instated. ... + [lersek@redhat.com: add last para to commit msg per talks w/ Mar= vin & Phil] * Pushed the first two patches in the series as commit range 84a459472075..35e242b698cd; closing . * Filed to track the CCS non-conformance issue, under approach "2a" (i.e., incrementally), as agreed upon with Zhichao. Assigned the BZ to myself; will post a patch soon. * Split off the last patch in the series to a separate TianoCore BZ, namely . Assigne= d that BZ to myself too. Thanks, Laszlo