From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 0DAE721ED1C60 for ; Wed, 7 Mar 2018 12:48:56 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D287A8163C52; Wed, 7 Mar 2018 20:55:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-198.rdu2.redhat.com [10.10.123.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id EDE582026980; Wed, 7 Mar 2018 20:55:09 +0000 (UTC) To: "Zhang, Chao B" , "Zeng, Star" , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" References: <20180307112441.20278-1-marcandre.lureau@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BA47DA0@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <580b566a-5087-b9a3-1e0e-0ef2cd2426fb@redhat.com> Date: Wed, 7 Mar 2018 21:55:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 07 Mar 2018 20:55:10 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 07 Mar 2018 20:55:10 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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 20:48:56 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/07/18 16:35, Zhang, Chao B wrote: > Star: > Why do we need to add HashInterfaceHob->SupportedHashMask = 0? HashInterfaceHob is internally maintained and accessed by HashLibRouterPei. > There is no impact to leave the value after module has been re-shadowed. There seems to be no functional requirement for clearing the SupportedHashMask field, except the original (buggy) ZeroMem() call looked like it *intended* to clear the field. So now that we have fixed the buffer overflow, we should decide whether we want to stick with the original intent (that would mean continuing to clear the field, one way or another), or to depart from the original intent -- but that would merit a comment in the code (or, it would have deserved a comment in the commit message). In short, it's not great to do two independent things in the same patch, namely (a) fix the buffer overflow, (b) silently diverge from the original intent, even if that's functionally justified. Such changes should be split to two patches, or else explained in detail. Thanks Laszlo