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 3999621184E8B for ; Mon, 12 Nov 2018 02:14:35 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8971788310; Mon, 12 Nov 2018 10:14:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-236.rdu2.redhat.com [10.10.120.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id 403D61974D; Mon, 12 Nov 2018 10:14:31 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Andrew Fish , Leif Lindholm , Michael D Kinney , Liming Gao , Ruiyu Ni , Jiewen Yao , Star Zeng References: <20181112013425.28588-1-hao.a.wu@intel.com> <20181112013425.28588-2-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <783ee24f-2925-e480-8b8a-0e0f2b829c2a@redhat.com> Date: Mon, 12 Nov 2018 11:14:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181112013425.28588-2-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 12 Nov 2018 10:14:34 +0000 (UTC) Subject: Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Nov 2018 10:14:35 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/12/18 02:34, Hao Wu wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142 > > The fix is similar to commit ebb6c7633bca47fcd5b460a67e18e4a717ea91cc. > We found that a similar fix should be applied to the NVMe PEI driver as > well. Hence, this one is for the PEI counterpart driver. > > According to the the NVM Express spec Revision 1.1, for some commands > (like Get/Set Feature Command, Figure 89 & 90 of the spec), the Memory > Buffer maybe optional although the command opcode indicates there is a > data transfer between host & controller (Get/Set Feature Command, Figure > 38 of the spec). > > Hence, this commit refine the checks for the 'TransferLength' and > 'TransferBuffer' field of the > EDKII_PEI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET structure to address this > issue. I agree that this change qualifies as a bugfix for the hard feature freeze. From that perspective, without checking any technical details: Acked-by: Laszlo Ersek *However*, please clean up the description in the bugzilla (BZ#1142). In the bugzilla, both the title and the initial description say that the check is "unnecessar" / "not necessary". If the problem were only that the check was "superfluous", then this patch would *not* qualify as a bugfix. Because, there would be *no bug*. And a patch to remove a superfluous (and otherwise harmless) check would be called a "cleanup", or a "trivial optimization". Instead, the check is *wrong*. It breaks valid behavior. That's why there is a bug, and that's why the patch is a bugfix. Please be clear about this distinction, and update the BZ. Thanks Laszlo > > Cc: Andrew Fish > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Ruiyu Ni > Cc: Jiewen Yao > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu > --- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 33 +++++++++++--------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c > index 81ad01b7ee..ddcfe03998 100644 > --- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c > +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c > @@ -442,7 +442,8 @@ NvmePassThru ( > // specific addresses. > // > if ((Sq->Opc & (BIT0 | BIT1)) != 0) { > - if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) { > + if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) || > + ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) { > return EFI_INVALID_PARAMETER; > } > > @@ -468,21 +469,23 @@ NvmePassThru ( > MapOp = EdkiiIoMmuOperationBusMasterWrite; > } > > - MapLength = Packet->TransferLength; > - Status = IoMmuMap ( > - MapOp, > - Packet->TransferBuffer, > - &MapLength, > - &PhyAddr, > - &MapData > - ); > - if (EFI_ERROR (Status) || (MapLength != Packet->TransferLength)) { > - Status = EFI_OUT_OF_RESOURCES; > - DEBUG ((DEBUG_ERROR, "%a: Fail to map data buffer.\n", __FUNCTION__)); > - goto Exit; > - } > + if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) { > + MapLength = Packet->TransferLength; > + Status = IoMmuMap ( > + MapOp, > + Packet->TransferBuffer, > + &MapLength, > + &PhyAddr, > + &MapData > + ); > + if (EFI_ERROR (Status) || (MapLength != Packet->TransferLength)) { > + Status = EFI_OUT_OF_RESOURCES; > + DEBUG ((DEBUG_ERROR, "%a: Fail to map data buffer.\n", __FUNCTION__)); > + goto Exit; > + } > > - Sq->Prp[0] = PhyAddr; > + Sq->Prp[0] = PhyAddr; > + } > > if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) { > MapLength = Packet->MetadataLength; >