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 8DD6A2095B08F for ; Tue, 10 Oct 2017 09:56:23 -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 CA8E081F01; Tue, 10 Oct 2017 16:59:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CA8E081F01 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-87.rdu2.redhat.com [10.10.120.87]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE2D660630; Tue, 10 Oct 2017 16:59:49 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Ard Biesheuvel References: <20171010085803.307284-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 10 Oct 2017 18:59:48 +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: <20171010085803.307284-1-ruiyu.ni@intel.com> 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.25]); Tue, 10 Oct 2017 16:59:50 +0000 (UTC) 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: Tue, 10 Oct 2017 16:56:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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? Thanks! Laszlo