[PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart

[PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart

[Hao Wu]

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

[PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru

[Hao Wu]

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

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru

[Laszlo Ersek]

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;
> 

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressPei: Refine data buffer & len check in PassThru

[Philippe Mathieu-Daudé]

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
> 

Re: [PATCH v1 0/1] [Nvme] Sync fix ebb6c76 from DXE to PEI counterpart

[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