From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.85.221.66; helo=mail-wr1-f66.google.com; envelope-from=philmd@redhat.com; receiver=edk2-devel@lists.01.org Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) (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 AAB302118F762 for ; Tue, 13 Nov 2018 04:03:39 -0800 (PST) Received: by mail-wr1-f66.google.com with SMTP id p4so3835619wrt.7 for ; Tue, 13 Nov 2018 04:03:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=XsGPy90BqlzSI15ZuhbDKKynzh8iSbldVYWWmrY/P28=; b=R23mgJ6oCN0/i3iQBu/0TVXX6xMtMZrM+GAUBgFJvddOs/v2nKI3E7w6CwRF5dLxoh MHiThWykRvqVkaseowCm97JRgAnXuIEOOgqt7moASIxT6MbhpUQoFdb7WkYv/a+/O7ah hy1UqfX4DUfWYwvugJA+QqDXpAlgsX9EAWlJSaxFmZNBJnZZNxRmhZsWVp2xzINqaaQL 7rOOWX7nt1sbPWKh2J+1hW1CZraYYAars1qLq6wA/igNpeJDT4ZSU4/ZrH5LAnwdsvQa fg5fkcOtTucNPkDtcnBORT32G6KPxYrVT4v/iH8wxxR9ychmXPRomdpC5qv4OgMreaYT fAZw== X-Gm-Message-State: AGRZ1gLn0uRdPY3SmJxCF5hnKbrugUEg/fpIb/qFFBe5vG8E0OgKGC57 sQ92iCfaklUrkrRL26HCX/zWNg== X-Google-Smtp-Source: AJdET5fTnuHAMHaytN24d/OvYd4DCRIFOu0LMNQqZGA98XAMz4Hi34KI1AK/fD1/n2kHURgBbGDjCg== X-Received: by 2002:adf:fe44:: with SMTP id m4-v6mr4802915wrs.309.1542110617425; Tue, 13 Nov 2018 04:03:37 -0800 (PST) Received: from [192.168.1.36] (2.red-83-59-163.dynamicip.rima-tde.net. [83.59.163.2]) by smtp.gmail.com with ESMTPSA id s16sm2751647wrt.77.2018.11.13.04.03.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 13 Nov 2018 04:03:36 -0800 (PST) To: "Wu, Hao A" , Laszlo Ersek , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , "Kinney, Michael D" , "Yao, Jiewen" , "Zeng, Star" , "Gao, Liming" References: <20181112013425.28588-1-hao.a.wu@intel.com> <20181112013425.28588-2-hao.a.wu@intel.com> <783ee24f-2925-e480-8b8a-0e0f2b829c2a@redhat.com> From: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Message-ID: <46e725b7-0de5-ffba-9fcd-bce2f89e0f67@redhat.com> Date: Tue, 13 Nov 2018 13:03:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: 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: Tue, 13 Nov 2018 12:03:41 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 13/11/18 2:02, Wu, Hao A wrote: >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo >> Ersek >> Sent: Monday, November 12, 2018 6:15 PM >> To: Wu, Hao A; edk2-devel@lists.01.org >> Cc: Ni, Ruiyu; Gao, Liming; Yao, Jiewen; Kinney, Michael D; Zeng, Star >> Subject: Re: [edk2] [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data >> buffer & len check in PassThru >> >> 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 I checked NVM Express spec Revision 1.3c. Reviewed-by: Philippe Mathieu-Daudé >> >> *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, > > I have updated the description of BZ 1142 to better reflect the issue. > > Best Regards, > Hao Wu > >> >> 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; >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >