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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>