Re: [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected

["Zeng, Star" <star.zeng@intel.com>]

On 2018/10/15 14:38, Ruiyu Ni wrote:
> From: Hao Wu <hao.a.wu@intel.com>
> 
> This commit adds checks to make sure the UFS devices do not return more
> data than the driver expected.
> 
> 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>

Thanks for the patch.
Reviewed-by: Star Zeng <star.zeng@intel.com>

Star

> ---
>   MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c        | 19 ++++++++++++--
>   .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 30 +++++++++++++++++++---
>   2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> index 936f25da5e..9b87835ed8 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> @@ -857,6 +857,14 @@ UfsRwDeviceDesc (
>       SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
>   
>       if (Read) {
> +      //
> +      // Make sure the hardware device does not return more data than expected.
> +      //
> +      if (ReturnDataSize > Packet.InTransferLength) {
> +        Status = EFI_DEVICE_ERROR;
> +        goto Exit;
> +      }
> +
>         CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
>         Packet.InTransferLength = ReturnDataSize;
>       } else {
> @@ -1170,8 +1178,15 @@ UfsExecScsiCmds (
>     SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
>   
>     if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
> -    CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> -    Packet->SenseDataLength = (UINT8)SenseDataLen;
> +    //
> +    // Make sure the hardware device does not return more data than expected.
> +    //
> +    if (SenseDataLen <= Packet->SenseDataLength) {
> +      CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> +      Packet->SenseDataLength = (UINT8)SenseDataLen;
> +    } else {
> +      Packet->SenseDataLength = 0;
> +    }
>     }
>   
>     //
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 5756f637fd..0238264878 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -833,6 +833,7 @@ UfsStopExecCmd (
>     @param[in] QueryResp  Pointer to the query response.
>   
>     @retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or opcode is invalid.
> +  @retval EFI_DEVICE_ERROR      Data returned from device is invalid.
>     @retval EFI_SUCCESS           Data extracted.
>   
>   **/
> @@ -853,6 +854,13 @@ UfsGetReturnDataFromQueryResponse (
>       case UtpQueryFuncOpcodeRdDesc:
>         ReturnDataSize = QueryResp->Tsf.Length;
>         SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
> +      //
> +      // Make sure the hardware device does not return more data than expected.
> +      //
> +      if (ReturnDataSize > Packet->TransferLength) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +
>         CopyMem (Packet->DataBuffer, (QueryResp + 1), ReturnDataSize);
>         Packet->TransferLength = ReturnDataSize;
>         break;
> @@ -1469,8 +1477,15 @@ UfsExecScsiCmds (
>     SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
>   
>     if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
> -    CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> -    Packet->SenseDataLength = (UINT8)SenseDataLen;
> +    //
> +    // Make sure the hardware device does not return more data than expected.
> +    //
> +    if (SenseDataLen <= Packet->SenseDataLength) {
> +      CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> +      Packet->SenseDataLength = (UINT8)SenseDataLen;
> +    } else {
> +      Packet->SenseDataLength = 0;
> +    }
>     }
>   
>     //
> @@ -2226,8 +2241,15 @@ ProcessAsyncTaskList (
>           SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
>   
>           if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
> -          CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> -          Packet->SenseDataLength = (UINT8)SenseDataLen;
> +          //
> +          // Make sure the hardware device does not return more data than expected.
> +          //
> +          if (SenseDataLen <= Packet->SenseDataLength) {
> +            CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
> +            Packet->SenseDataLength = (UINT8)SenseDataLen;
> +          } else {
> +            Packet->SenseDataLength = 0;
> +          }
>           }
>   
>           //
>