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::22f; helo=mail-io0-x22f.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x22f.google.com (mail-io0-x22f.google.com [IPv6:2607:f8b0:4001:c06::22f]) (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 AE42F21F7D4E4 for ; Wed, 11 Oct 2017 09:06:47 -0700 (PDT) Received: by mail-io0-x22f.google.com with SMTP id 101so2375418ioj.3 for ; Wed, 11 Oct 2017 09:10:17 -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; bh=1ehH1b82oXHtsVlsX368dMmSF36IAGpl5bQVm/NL8ss=; b=OMhn2fqKSpMSYrE85SXBvak4Ba/KBorfum1d7J2e00n4Eztqf9Rv3KPYJea4IYfvfZ rJ4t65OuKlsjxjydPzQwQf1SnrGC2QvPOC0nqJmBGhdF5iIwP5AImx/FwPgJBMJxO3gR C4af/9ZcrJTW3nFc3P3hC5gdfP7VaUoSbRwy8= 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; bh=1ehH1b82oXHtsVlsX368dMmSF36IAGpl5bQVm/NL8ss=; b=gc1jHH1BA4uWJK6NAieJ1ebq8L1zGZD6LGIJGpjt7TJkV/onkIOxgK65jBR2WRKD8L jsvnK+bG5ReWji1mpbxBcm3ceccQLavXmelkS1P67Rpq8OiumbOkkx+Z24zOI+ewAm36 CiEeqYqGINzdE3TZvVASsHRCTwLawf211hJk/cNmaGO7W8Rm8TdhVy9cY0D2GO/xtHjg hrVASX2GiRxErB3uVhNojKZVJkotGUu85y2wjxY7YalxAW5OJ9Nb1M85N6srztgKsrqe a1fX6sqboC33Pu1Elrk3rJ3FPES+hDqk2LjCy/9hD3olVOTDuZtaC0y04GkWCtVuXA3J NLtA== X-Gm-Message-State: AMCzsaWg6fm3RDTfyCZoIUQG4U6zVzOxLkkkQowHTaPSoILEmB3gaTEg xrMWBgfAklaCt56y1yn36kFYBj5WEduPfhhgDJbB4g== X-Google-Smtp-Source: ABhQp+RD89Yr2NioQkc0FE6EY0kQjOsKYAosPONNS+bBiIwrc42mP5QkWhtSbowSn8+6pncv9X9jcVqYWDYHbwkYUSA= X-Received: by 10.107.59.211 with SMTP id i202mr144681ioa.79.1507738216390; Wed, 11 Oct 2017 09:10:16 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Wed, 11 Oct 2017 09:10:15 -0700 (PDT) In-Reply-To: References: <20171010085803.307284-1-ruiyu.ni@intel.com> From: Ard Biesheuvel Date: Wed, 11 Oct 2017 17:10:15 +0100 Message-ID: To: Laszlo Ersek Cc: Ruiyu Ni , "edk2-devel@lists.01.org" Subject: Re: [PATCH] MdeModulePkg/Bds: Check variable name even OptionNumber is NULL 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, 11 Oct 2017 16:06:47 -0000 Content-Type: text/plain; charset="UTF-8" On 10 October 2017 at 17:59, Laszlo Ersek wrote: > On 10/10/17 10:58, Ruiyu Ni wrote: >> Current implementation skips to check whether the last four >> characters are digits when the OptionNumber is NULL. >> Even worse, it may incorrectly return FALSE when OptionNumber is >> NULL. >> >> The patch fixes it to always check the variable name even >> OptionNumber is NULL. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ruiyu Ni >> Cc: Ard Biesheuvel >> Cc: Laszlo Ersek >> --- >> .../Library/UefiBootManagerLib/BmLoadOption.c | 45 ++++++++++++++-------- >> 1 file changed, 30 insertions(+), 15 deletions(-) >> >> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c >> index b0a35058d0..32918caf32 100644 >> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c >> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c >> @@ -785,6 +785,8 @@ EfiBootManagerIsValidLoadOptionVariableName ( >> UINTN VariableNameLen; >> UINTN Index; >> UINTN Uint; >> + EFI_BOOT_MANAGER_LOAD_OPTION_TYPE LocalOptionType; >> + UINT16 LocalOptionNumber; >> >> if (VariableName == NULL) { >> return FALSE; >> @@ -792,39 +794,52 @@ EfiBootManagerIsValidLoadOptionVariableName ( >> >> VariableNameLen = StrLen (VariableName); >> >> + // >> + // Return FALSE when the variable name length is too small. >> + // >> if (VariableNameLen <= 4) { >> return FALSE; >> } >> >> - for (Index = 0; Index < ARRAY_SIZE (mBmLoadOptionName); Index++) { >> - if ((VariableNameLen - 4 == StrLen (mBmLoadOptionName[Index])) && >> - (StrnCmp (VariableName, mBmLoadOptionName[Index], VariableNameLen - 4) == 0) >> + // >> + // Return FALSE when the variable name doesn't start with Driver/SysPrep/Boot/PlatformRecovery. >> + // >> + for (LocalOptionType = 0; LocalOptionType < ARRAY_SIZE (mBmLoadOptionName); LocalOptionType++) { >> + if ((VariableNameLen - 4 == StrLen (mBmLoadOptionName[LocalOptionType])) && >> + (StrnCmp (VariableName, mBmLoadOptionName[LocalOptionType], VariableNameLen - 4) == 0) >> ) { >> break; >> } >> } >> + if (LocalOptionType == ARRAY_SIZE (mBmLoadOptionName)) { >> + return FALSE; >> + } >> >> - if (Index == ARRAY_SIZE (mBmLoadOptionName)) { >> + // >> + // Return FALSE when the last four characters are not hex digits. >> + // >> + LocalOptionNumber = 0; >> + for (Index = VariableNameLen - 4; Index < VariableNameLen; Index++) { >> + Uint = BmCharToUint (VariableName[Index]); >> + if (Uint == -1) { >> + break; >> + } else { >> + LocalOptionNumber = (UINT16) Uint + LocalOptionNumber * 0x10; >> + } >> + } >> + if (Index != VariableNameLen) { >> return FALSE; >> } >> >> if (OptionType != NULL) { >> - *OptionType = (EFI_BOOT_MANAGER_LOAD_OPTION_TYPE) Index; >> + *OptionType = LocalOptionType; >> } >> >> if (OptionNumber != NULL) { >> - *OptionNumber = 0; >> - for (Index = VariableNameLen - 4; Index < VariableNameLen; Index++) { >> - Uint = BmCharToUint (VariableName[Index]); >> - if (Uint == -1) { >> - break; >> - } else { >> - *OptionNumber = (UINT16) Uint + *OptionNumber * 0x10; >> - } >> - } >> + *OptionNumber = LocalOptionNumber; >> } >> >> - return (BOOLEAN) (Index == VariableNameLen); >> + return TRUE; >> } >> >> /** >> > > I have some superficial comments: > > - I think the subject should say > > MdeModulePkg/Bds: Check variable name even *if* OptionNumber is NULL > > - I'm not a huge fan of using enum types for loop counters; however, in > this case, I verified that there is LoadOptionTypeMax, and it equals > ARRAY_SIZE (mBmLoadOptionName). So it should be safe. If we want to be > more explicit about this, we could replace > > CHAR16 *mBmLoadOptionName[] = { > > with > > CHAR16 *mBmLoadOptionName[LoadOptionTypeMax] = { > > Anyway, > > Reviewed-by: Laszlo Ersek > > Ard, can you test this patch in your original environment? > I will try this as soon as I get a chance (I have to switch back to an older branch of the platform I am currently developing for)