From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::233; helo=mail-io0-x233.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3B79D21CF58B6 for ; Wed, 4 Oct 2017 07:58:06 -0700 (PDT) Received: by mail-io0-x233.google.com with SMTP id k101so10795442iod.0 for ; Wed, 04 Oct 2017 08:01:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=g4BRfrm1nX7i9blePQTauYmfF1oJho8oVr91oZl0oH8=; b=PHD+RXr3zIJcseGsnnMzGSlU7hMNOJFlsyzMa9MNyMs3ycEvBrmahwLcPlDvqWKBZN ZZ8+ucLhbzbk2WhXyk5+rbikD/TI9YFoi+sB5vce77l/bPpOYww/KrvfVAOSGKsBcUWB ffqHE/qinh72G1nQOMjt2lMgtywM3PID9BoW0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=g4BRfrm1nX7i9blePQTauYmfF1oJho8oVr91oZl0oH8=; b=Czcrto5w8mO9Fpwg6slStke5eIN92r5F1La74x8N2yl1CKgsrp9XuXK345ucC+JnkB 0YeEWFo4qJlmfWfq3TbnAzJLWjCYKB52VgxOqETcbWwxPQg5I7eF0Cq55UAl1E4Npv4p 64KR7YDpCd30Xwx314lZO0JND3Ol8jid3HyzJ3N1iSRPMdbLR1FaOaLro1t/3rHvS3dx BQpZEVOBIeDVdgZuLRk9XDeTnEWvCY42/nlrpWsewW0BUXb2t7CtlPZv0S88voFtQbqY bCrdr6s0ssjf0yrc4tiEYgCbojgj/lw+VDd2+QKk/xso5fl87GY7Xi/3FI6z17qCoyvY U7nw== X-Gm-Message-State: AMCzsaU1YRBmCVlpGqegzdGMGVAdhwRihPIgumiJWkmoQbynSv9eLyFm mS33PhpPZ4ZOu2EdzP8Gi9CmEyt4SEXOfMmsyH6s7g== X-Google-Smtp-Source: AOwi7QA90BINKPDQfhkONNFeJbV6nf3qyLLb68/KCWcMzTUm7iMMe0PWaebu6lKnF7bJ45D8ey52INUkFyjYinRChfc= X-Received: by 10.107.27.149 with SMTP id b143mr31505984iob.2.1507129285066; Wed, 04 Oct 2017 08:01:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.152.18 with HTTP; Wed, 4 Oct 2017 08:01:24 -0700 (PDT) In-Reply-To: <5e81c006-bbe4-5124-4227-fb06805a1256@redhat.com> 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> <5e81c006-bbe4-5124-4227-fb06805a1256@redhat.com> From: Ard Biesheuvel Date: Wed, 4 Oct 2017 16:01:24 +0100 Message-ID: To: Laszlo Ersek Cc: "Zeng, Star" , "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Yao, Jiewen" , "Dong, Eric" , "leif.lindholm@linaro.org" 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:58:07 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 4 October 2017 at 15:40, Laszlo Ersek wrote: > 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 wil= l be >>> rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf th= at >>> will check the VariableName against UEFI spec =E2=80=9CTable 13. Global= Variables=E2=80=9D >>> if the VendorGuid is gEfiGlobalVariableGuid. >>> >>> >>> >>> I would suspect there is a bug at other place if the code ends up calli= ng >>> this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNex= t". >>> >> >> That still does not mean you should ASSERT() here. The state of the >> variable store !=3D 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. > That is true. But the firmware that wrote to the varstore may be a different version from the one reading it. >> 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, Ard.