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::22b; helo=mail-wm0-x22b.google.com; envelope-from=marcandre.lureau@gmail.com; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (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 830DF22135D28 for ; Wed, 7 Mar 2018 06:49:26 -0800 (PST) Received: by mail-wm0-x22b.google.com with SMTP id t3so5330884wmc.2 for ; Wed, 07 Mar 2018 06:55:41 -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=keinQidZQ+QvFIhwBSAHV+yyPsLKupqR4nZwrIECCD8=; b=tmx2Z3MoTRMe1LqBB/9hIv5BhWw92FkM1K9Jror9f3u5Ia2AYEk0V9kvzvcPY8/RD6 RtTC2NrMYfsybsBOQyx6VjF/e98PBD2w2EuvHOudIGuNrEq9irJ0ZsgL12MtYdCesKfZ sQghxevoyuas7dne8XPf+uVIHV+h8Y4itKKjE0S6OIb2pQg8ksp4MGPtIm+H9lyx7MEQ WNglz9ZTTKcSboLnZWzorQ5hkpJXkkAunBe81OzxWJiHctXr7ionM3txmw/5GD8w9WPI y3UJBaJbDt9pfTYllsTWzMn37X08ggjZHGyYVg0G6yWPLf08z2554/TqJHKj0ro8lGfg OK2w== 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=keinQidZQ+QvFIhwBSAHV+yyPsLKupqR4nZwrIECCD8=; b=PBWjc/ici451Skv0ahUXR3heJQc8qq4IaBEmRjPtI1t9hPSVKCV8DLvtgG0ere0juq JLlE2C52qnpi4wZHfmMDEzylFHL721OzDnHoBhn/+A3Uy2u3LrfSmA7tANASiSqN5vHj WGhHEMm896ZEXij232nvREnUBS1Nd5TqrAvjF5QvOj/6LeHwLAS8RNUg3jZwUgpXler5 /58oHZfEQcT1S/RXSjN6JdvqS4vjT8LGAzCxADq2Y9W9ELjUV0WZbArXJcp6n1ENafhm fUPZP77M+taWKWVq6BhffGUcqIfAqnhneNbzgQT1aEefT+VNkNW+U2a/Lz1W5IlA7INI BInQ== X-Gm-Message-State: AElRT7FzZ5KMwdicThFF35CR0sT54z0KMLj/pHF2wd6sofTsd31mRaSv yJjsEsczYzzXiNJXT25MqUl+nXVdJvg4k6H1qY98JA== X-Google-Smtp-Source: AG47ELtNohF3wxfPEZsSAfOc11vQ+yiqop6GC217PnLcqAsVBNj7EU+HaXnVqswCh5gd5KHLeGXnlSjpCw4P9ikA1cY= X-Received: by 10.28.145.136 with SMTP id t130mr14238597wmd.36.1520434539379; Wed, 07 Mar 2018 06:55:39 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.185.67 with HTTP; Wed, 7 Mar 2018 06:55:38 -0800 (PST) In-Reply-To: <20180307112441.20278-1-marcandre.lureau@redhat.com> References: <20180307112441.20278-1-marcandre.lureau@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Wed, 7 Mar 2018 15:55:38 +0100 Message-ID: To: edk2-devel@lists.01.org Cc: Jiewen Yao , Star Zeng , Chao Zhang , Laszlo Ersek Subject: Re: [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob 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 14:49:27 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Wed, Mar 7, 2018 at 12:24 PM, wrote: > From: Marc-Andr=C3=A9 Lureau > > The ZeroMem() call goes beyond the HashInterfaceHob structure, causing > HOB list corruption. The intent was to clear all but the Identifier, > that is starting from HashInterfaceCount. > I see v1 is already applied. I'll send another patch to clear the mask in the Ovmf/TPM series. Thanks > Quoting Laszlo Ersek: > Therefore I think the *first* approach to actually fixing this bug woul= d > be to simply add the "Count" suffix: > > > diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseC= ryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseC= ryptoRouterPei.c > > index dbee0f2531bc..a869fffae3fe 100644 > > --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRo= uterPei.c > > +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRo= uterPei.c > > @@ -424,7 +424,10 @@ HashLibBaseCryptoRouterPeiConstructor ( > > // This is the second execution of this module, clear the hash i= nterface > > // information registered at its first execution. > > // > > - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfac= eHob) - 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. > > This patch takes Laszlo suggestion to fix the issue. > > 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 | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCrypt= oRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCrypt= oRouterPei.c > index dbee0f2531bc..84c1aaae70eb 100644 > --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouter= Pei.c > +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouter= Pei.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 inter= face > // information registered at its first execution. > // > - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHob= ) - sizeof (EFI_GUID)); > + ClearStartOffset =3D OFFSET_OF (HASH_INTERFACE_HOB, HashInterfaceCou= nt); > + ZeroMem ( > + (UINT8 *)HashInterfaceHob + ClearStartOffset, > + sizeof (*HashInterfaceHob) - ClearStartOffset > + ); > } > > // > -- > 2.16.2.346.g9779355e34 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel --=20 Marc-Andr=C3=A9 Lureau