From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6748A21CF58BD for ; Wed, 4 Oct 2017 07:37:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7A7E32579F; Wed, 4 Oct 2017 14:40:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7A7E32579F Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-111.rdu2.redhat.com [10.10.120.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id E9A8B60603; Wed, 4 Oct 2017 14:40:39 +0000 (UTC) To: Ard Biesheuvel , "Zeng, Star" Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Yao, Jiewen" , "Dong, Eric" , "leif.lindholm@linaro.org" References: <20171003171727.5641-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E276@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9CE19A@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E44C@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <5e81c006-bbe4-5124-4227-fb06805a1256@redhat.com> Date: Wed, 4 Oct 2017 16:40:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 04 Oct 2017 14:40:41 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Oct 2017 14:37:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 10/04/17 15:54, Ard Biesheuvel wrote: > On 4 October 2017 at 14:49, Zeng, Star wrote: >> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be >> rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that >> will check the VariableName against UEFI spec “Table 13. Global Variables” >> if the VendorGuid is gEfiGlobalVariableGuid. >> >> >> >> I would suspect there is a bug at other place if the code ends up calling >> this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >> > > That still does not mean you should ASSERT() here. The state of the > variable store != the internals of the code, and so it should be > considered external input to some extent. At least under some circumstances, I disagree with this. The assumption that the variable store can be written only by privileged firmware routines is core to Secure Boot, for example. > ASSERTs are meant to catch > programming errors, not errors in the varstore image. I agree. However, as a corollary to the above, if said "privileged routines" are supposed to catch all invalid inputs passed to gRT->SetVariable(), then the rest of the firmware is right to assume that the contents of the variable store are valid. If it is found invalid at some point, then it is indeed due to a programming error (somewhere in the gRT->SetVariable() machinery, that is), so the ASSERT() is justified. Another example in support of this argument is the Fault Tolerant Write machinery -- the firmware tries very hard to recover from power loss during a varstore update. If, on reboot, the error proves non-recoverable (i.e. we cannot even roll back to a previous pristine state), then that can be considered a bug (or design error) in FTW. That said, I agree with the patch. BmCharToUint() explicitly documents "conversion failed" as a return condition, and both functions that call BmCharToUint(), namely EfiBootManagerIsValidLoadOptionVariableName() and BmIsKeyOptionVariable(), check for that return condition. Thanks, Laszlo