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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 10/18/2018 2:41 PM, Hao Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142
> 
> 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 EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET
> structure to address this issue.
> 
> Cc: Liangcheng Tang <liangcheng.tang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@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/NvmExpressDxe/NvmExpressPassthru.c | 33 +++++++++++---------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 2468871322..bfcd349794 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -595,7 +595,8 @@ NvmExpressPassThru (
>     //
>     if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
>         !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD)))) {
> -    if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) {
> +    if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
> +        ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
>         return EFI_INVALID_PARAMETER;
>       }
>   
> @@ -605,21 +606,23 @@ NvmExpressPassThru (
>         Flag = EfiPciIoOperationBusMasterWrite;
>       }
>   
> -    MapLength = Packet->TransferLength;
> -    Status = PciIo->Map (
> -                      PciIo,
> -                      Flag,
> -                      Packet->TransferBuffer,
> -                      &MapLength,
> -                      &PhyAddr,
> -                      &MapData
> -                      );
> -    if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> +    if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) {
> +      MapLength = Packet->TransferLength;
> +      Status = PciIo->Map (
> +                        PciIo,
> +                        Flag,
> +                        Packet->TransferBuffer,
> +                        &MapLength,
> +                        &PhyAddr,
> +                        &MapData
> +                        );
> +      if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
>   
> -    Sq->Prp[0] = PhyAddr;
> -    Sq->Prp[1] = 0;
> +      Sq->Prp[0] = PhyAddr;
> +      Sq->Prp[1] = 0;
> +    }
>   
>       if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) {
>         MapLength = Packet->MetadataLength;
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray