On 13/11/18 2:02, Wu, Hao A wrote:
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@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 <ler...@redhat.com>
I checked NVM Express spec Revision 1.3c.
Reviewed-by: Philippe Mathieu-Daudé <phi...@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 <af...@apple.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <liming....@intel.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Star Zeng <star.z...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
