* [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart @ 2018-11-12 1:34 Hao Wu 2018-11-12 1:34 ` [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru Hao Wu 2018-11-12 6:13 ` [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Zeng, Star 0 siblings, 2 replies; 5+ messages in thread From: Hao Wu @ 2018-11-12 1:34 UTC (permalink / raw) To: edk2-devel Cc: Hao Wu, Andrew Fish, Laszlo Ersek, Leif Lindholm, Michael D Kinney, Liming Gao, Ruiyu Ni, Jiewen Yao, Star Zeng This patch is a bug fix that targets for the edk2-stable201811 tag. This patch syncs the below fix: SHA-1: ebb6c7633bca47fcd5b460a67e18e4a717ea91cc * MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru to the NvmExpressPei counter part. More details of the fix background can be referred from the log message of the patch. Cc: Andrew Fish <afish@apple.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Jiewen Yao <Jiewen.yao@intel.com> Cc: Star Zeng <star.zeng@intel.com> Hao Wu (1): MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 33 +++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) -- 2.12.0.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru 2018-11-12 1:34 [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Hao Wu @ 2018-11-12 1:34 ` Hao Wu 2018-11-12 10:14 ` Laszlo Ersek 2018-11-12 6:13 ` [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Zeng, Star 1 sibling, 1 reply; 5+ messages in thread From: Hao Wu @ 2018-11-12 1:34 UTC (permalink / raw) To: edk2-devel Cc: Hao Wu, Andrew Fish, Laszlo Ersek, Leif Lindholm, Michael D Kinney, Liming Gao, Ruiyu Ni, Jiewen Yao, Star Zeng 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. Cc: Andrew Fish <afish@apple.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Jiewen Yao <Jiewen.yao@intel.com> Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu <hao.a.wu@intel.com> --- 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; -- 2.12.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru 2018-11-12 1:34 ` [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru Hao Wu @ 2018-11-12 10:14 ` Laszlo Ersek [not found] ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C84C502@SHSMSX104.ccr.corp.intel.com> 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2018-11-12 10:14 UTC (permalink / raw) To: Hao Wu, edk2-devel Cc: Andrew Fish, Leif Lindholm, Michael D Kinney, Liming Gao, Ruiyu Ni, Jiewen Yao, Star Zeng 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 <lersek@redhat.com> *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 <afish@apple.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Jiewen Yao <Jiewen.yao@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu <hao.a.wu@intel.com> > --- > 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; > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <B80AF82E9BFB8E4FBD8C89DA810C6A093C84C502@SHSMSX104.ccr.corp.intel.com>]
* Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru [not found] ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C84C502@SHSMSX104.ccr.corp.intel.com> @ 2018-11-13 12:03 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2018-11-13 12:03 UTC (permalink / raw) To: Wu, Hao A, Laszlo Ersek, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Kinney, Michael D, Yao, Jiewen, Zeng, Star, Gao, Liming 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 <lersek@redhat.com> I checked NVM Express spec Revision 1.3c. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> *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 <afish@apple.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Cc: Leif Lindholm <leif.lindholm@linaro.org> >>> Cc: Michael D Kinney <michael.d.kinney@intel.com> >>> Cc: Liming Gao <liming.gao@intel.com> >>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>> Cc: Jiewen Yao <Jiewen.yao@intel.com> >>> Cc: Star Zeng <star.zeng@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Hao Wu <hao.a.wu@intel.com> >>> --- >>> 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart 2018-11-12 1:34 [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Hao Wu 2018-11-12 1:34 ` [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru Hao Wu @ 2018-11-12 6:13 ` Zeng, Star 1 sibling, 0 replies; 5+ messages in thread From: Zeng, Star @ 2018-11-12 6:13 UTC (permalink / raw) To: Wu, Hao A, edk2-devel@lists.01.org Cc: Andrew Fish, Laszlo Ersek, Leif Lindholm, Kinney, Michael D, Gao, Liming, Ni, Ruiyu, Yao, Jiewen, Zeng, Star Reviewed-by: Star Zeng <star.zeng@intel.com> -----Original Message----- From: Wu, Hao A Sent: Monday, November 12, 2018 9:34 AM To: edk2-devel@lists.01.org Cc: Wu, Hao A <hao.a.wu@intel.com>; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart This patch is a bug fix that targets for the edk2-stable201811 tag. This patch syncs the below fix: SHA-1: ebb6c7633bca47fcd5b460a67e18e4a717ea91cc * MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru to the NvmExpressPei counter part. More details of the fix background can be referred from the log message of the patch. Cc: Andrew Fish <afish@apple.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Jiewen Yao <Jiewen.yao@intel.com> Cc: Star Zeng <star.zeng@intel.com> Hao Wu (1): MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 33 +++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) -- 2.12.0.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-13 12:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-12 1:34 [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Hao Wu 2018-11-12 1:34 ` [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru Hao Wu 2018-11-12 10:14 ` Laszlo Ersek [not found] ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C84C502@SHSMSX104.ccr.corp.intel.com> 2018-11-13 12:03 ` Philippe Mathieu-Daudé 2018-11-12 6:13 ` [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox