From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::235; helo=mail-wm0-x235.google.com; envelope-from=marcandre.lureau@gmail.com; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x235.google.com (mail-wm0-x235.google.com [IPv6:2a00:1450:400c:c09::235]) (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 724FE20954CC2 for ; Wed, 7 Mar 2018 02:47:28 -0800 (PST) Received: by mail-wm0-x235.google.com with SMTP id z9so3810703wmb.3 for ; Wed, 07 Mar 2018 02:53:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=uYZowKkavMWBj560WzJkQOroeQHEvjJz8Ko2o9Qjkzs=; b=V3xyUx5BoJYtfXoahT5dnOw4qafiGPfvjj3OSCVEBY4MGpBggbsScChgzOpHewVMjQ MEmlSV22pfLy6jMirjjzQRTw5ioa6w/0vq627hbS+K8Fgte5H61q4ImL8eFdM0mxBSye kvK0fat14tsSAtCrEr7RwNz1uKNXMtNr2Qts/0jyvYXAO6D3LqkFyhoKRkLNJY2vEHFN T6jk+xYWCUXiuCSrzXEjHsfsDaHea7RWEqIKK8PsfPYv8QrNVUyVOqGA6KJ39mz6y5wt UJ7qVG3rHoUi/kSUYOLdldHIlrtdIw0g8iDUJV1PPt5OQEPWJLvlHoi6/2l1QJ/HPuIU 4ayQ== 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=uYZowKkavMWBj560WzJkQOroeQHEvjJz8Ko2o9Qjkzs=; b=P0YaTKvuC3Bk5e+mcuGK5mwpS0Q6oPXXc2uAE9JU0nD6MuTxBQZcTBlOKMZ8T0kPJz k77bcGl7cSwmHgLjRgFJ4dwhpxsEkSi7rOIMMwh94RaPhm0pHyfsh8pZhxB+IZnLtgrL HkVqE+wRHiEXF7MXCgr3Vo0NYea0WY4EcXGYuvAKblfwoHLj8QwuwOxzqpBuksB+iJRI 63I0YvheoeBbdz3/ZtDPuBLFw2NvtVBcaolgTlcEApTPvKW8z9IaOqyRHoxUk7TZ6Ej3 6UGbrWLIJbIJmg6gwzywiSeKrNeF8mXsnDmM+t5I7OGWsu8xRXtTabMtdNDFJx2Z63pk 6MNA== X-Gm-Message-State: AElRT7FqBpPLGB4QQzhHT6dNeqDic2bYs8Xmj8vZqw5Oe7ausFfFr2Ck Vw4WCV056Xl42ExwyqkpiaZeyHue1YUv1BaKSqU= X-Google-Smtp-Source: AG47ELt10vzuObieTPnCRM8xt5Lv4/TBniL+iee6vMU0zDL6j4hOzUTL2XKVM6rEUw6Y4RmokJw8Amty/eWeByH67qE= X-Received: by 10.28.53.6 with SMTP id c6mr7425424wma.18.1520420021656; Wed, 07 Mar 2018 02:53:41 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.185.67 with HTTP; Wed, 7 Mar 2018 02:53:41 -0800 (PST) In-Reply-To: <1dbffca1-c83e-0839-45b5-62875b0cfae0@redhat.com> References: <20180306202718.4061-1-marcandre.lureau@redhat.com> <1dbffca1-c83e-0839-45b5-62875b0cfae0@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Wed, 7 Mar 2018 11:53:41 +0100 Message-ID: To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Jiewen Yao , Star Zeng , Chao Zhang Subject: Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Mar 2018 10:47:29 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Wed, Mar 7, 2018 at 10:06 AM, Laszlo Ersek wrote: > Hi Marc-Andr=C3=A9, > > On 03/06/18 21:27, marcandre.lureau@redhat.com wrote: >> From: Marc-Andr=C3=A9 Lureau >> >> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing >> HOB list corruption. Instead, just clear the HashInterface fields, as >> I suppose was originally intended. >> >> Cc: Jiewen Yao >> Cc: Chao Zhang >> Cc: Star Zeng >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Marc-Andr=C3=A9 Lureau >> --- >> .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c >> index dbee0f2531bc..361a4f6508a0 100644 >> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> @@ -424,7 +424,8 @@ HashLibBaseCryptoRouterPeiConstructor ( >> // This is the second execution of this module, clear the hash inte= rface >> // information registered at its first execution. >> // >> - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHo= b) - sizeof (EFI_GUID)); >> + ZeroMem (&HashInterfaceHob->HashInterface, sizeof (HashInterfaceHob= ->HashInterface)); >> + HashInterfaceHob->HashInterfaceCount =3D 0; >> } >> >> // >> > > Excellent catch! :) > > I do have a question however, for the other reviewers on this patch, in > line with your commit message saying, "clear the HashInterface fields, > as *I suppose* was originally intended". > > I don't think the proposed update matches the original intent 100%. > Namely, this is the definition of the HASH_INTERFACE_HOB type (from the > same source file): > >> typedef struct { >> // >> // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consume= s HashLib >> // or the hash algorithm bitmap of LAST module which consumes HashLi= b. >> // HashInterfaceCount and HashInterface are all 0. >> // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and Supporte= dHashMask >> // are the hash interface information of CURRENT module which consum= es HashLib. >> // >> EFI_GUID Identifier; >> UINTN HashInterfaceCount; >> HASH_INTERFACE HashInterface[HASH_COUNT]; >> UINT32 SupportedHashMask; >> } HASH_INTERFACE_HOB; > > Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all > three* subsequent fields carry information of the current module that > consumes HashLib. Those three fields include "SupportedHashMask". > > Furthermore, the comment just preceding the bug / ZeroMem() call says, > >> // >> // In PEI phase, some modules may call RegisterForShadow and will be >> // shadowed and executed again after memory is discovered. >> // This is the second execution of this module, clear the hash inter= face >> // information registered at its first execution. >> // > > And note that here we are just past the check > >> HashInterfaceHob =3D InternalGetHashInterfaceHob (&gEfiCallerIdGuid); >> if (HashInterfaceHob !=3D NULL) { > > in other words, the "Identifier equals gEfiCallerIdGuid" branch from the > type definition applies; meaning that "SupportedHashMask" is defined > too. > > Thus, I'd like the other reviewers to confirm whether we should clear > "SupportedHashMask". > > From the original (buggy) code, I think we *should* clear > SupportedHashMask as well. The "Length" argument of the original > ZeroMem() call implies precisely that: > > sizeof (*HashInterfaceHob) - sizeof (EFI_GUID) > > This stands for "all fields in *HashInterfaceHob except the leading > EFI_GUID Identifier field". > > So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it > is a typo. Certainly the intent must have been "start clearing from the > first field right after the EFI_GUID Identifier field", namely > "HashInterfaceCount": > > &HashInterfaceHob->HashInterfaceCount > ^^^^^ > > Therefore I think the *first* approach to actually fixing this bug would > be to simply add the "Count" suffix: > >> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c >> index dbee0f2531bc..a869fffae3fe 100644 >> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> @@ -424,7 +424,10 @@ HashLibBaseCryptoRouterPeiConstructor ( >> // This is the second execution of this module, clear the hash inte= rface >> // information registered at its first execution. >> // >> - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHo= b) - sizeof (EFI_GUID)); >> + ZeroMem ( >> + &HashInterfaceHob->HashInterfaceCount, >> + sizeof (*HashInterfaceHob) - sizeof (EFI_GUID) >> + ); >> } >> >> // > > On the other hand, I don't think such a fix would be great. First of > all, the HASH_INTERFACE_HOB struct definition is not wrapped in > > #pragma pack (1) > ... > #pragma pack () > > in other words, HASH_INTERFACE_HOB is not packed, hence there could be > an unspecified amount of padding between "Identifier" and > "HashInterfaceCount". That would lead to the same kind of buffer > overflow, because "Length" includes the padding, but the "Buffer" > argument doesn't. > > Second, even if there were no padding (and thus the byte count would be > a perfect match for the full HOB structure), in the "Buffer" argument > we take the address of the "HashInterfaceCount" field, and with the > "Length" field, we still overflow that *individual* field. Although we > wouldn't overflow the containing HOB structure, this is still (arguably) > undefined behavior -- I seem to remember that some compilers actually > blow up on this when you use the standard ISO C memcpy() or memset() > functions in such contexts. > > So, long story short, I suggest the following fix instead: > >> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryp= toRouterPei.c >> index dbee0f2531bc..84c1aaae70eb 100644 >> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute= rPei.c >> @@ -397,6 +397,7 @@ HashLibBaseCryptoRouterPeiConstructor ( >> { >> EFI_STATUS Status; >> HASH_INTERFACE_HOB *HashInterfaceHob; >> + UINTN ClearStartOffset; >> >> HashInterfaceHob =3D InternalGetHashInterfaceHob (&gZeroGuid); >> if (HashInterfaceHob =3D=3D NULL) { >> @@ -424,7 +425,11 @@ HashLibBaseCryptoRouterPeiConstructor ( >> // This is the second execution of this module, clear the hash inte= rface >> // information registered at its first execution. >> // >> - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHo= b) - sizeof (EFI_GUID)); >> + ClearStartOffset =3D OFFSET_OF (HASH_INTERFACE_HOB, HashInterfaceCo= unt); >> + ZeroMem ( >> + (UINT8 *)HashInterfaceHob + ClearStartOffset, >> + sizeof (*HashInterfaceHob) - ClearStartOffset >> + ); >> } >> >> // > > Thoughts? Looking at RegisterHashInterfaceLib() code, I agree it makes sense to clear both the array and the mask. Both seem to be closely related during registration. I agree with your argument as well about structure packing. I'll resend the patch with your suggested changes. thanks