Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing

["Laszlo Ersek" <lersek@redhat.com>]

On 04/06/21 21:52, Kun Qin wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3283
> 
> Current SMM Save State routine does not check the number of bytes to be
> read, when it comse to read IO_INFO, before casting the incoming buffer
> to EFI_SMM_SAVE_STATE_IO_INFO. This could potentially cause memory
> corruption due to extra bytes are written out of buffer boundary.
> 
> This change adds a width check before copying IoInfo into output buffer.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> 
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v2:
>     - Update return code description [Laszlo]
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 9 ++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 +-
>  2 files changed, 9 insertions(+), 2 deletions(-)

Thanks, looks OK. I'll let Ray or Eric merge the patch.

Laszlo

> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> index 661cc51f361a..fc418c2500a9 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> @@ -343,7 +343,7 @@ ReadSaveStateRegisterByIndex (
>  
>    @retval EFI_SUCCESS           The register was read from Save State.
>    @retval EFI_NOT_FOUND         The register is not defined for the Save State of Processor.
> -  @retval EFI_INVALID_PARAMETER  This or Buffer is NULL.
> +  @retval EFI_INVALID_PARAMETER Buffer is NULL, or Width does not meet requirement per Register type.
>  
>  **/
>  EFI_STATUS
> @@ -418,6 +418,13 @@ ReadSaveStateRegister (
>        return EFI_NOT_FOUND;
>      }
>  
> +    //
> +    // Make sure the incoming buffer is large enough to hold IoInfo before accessing
> +    //
> +    if (Width < sizeof (EFI_SMM_SAVE_STATE_IO_INFO)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
>      //
>      // Zero the IoInfo structure that will be returned in Buffer
>      //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index b8aa9e1769d3..2248a8c5ee66 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -337,7 +337,7 @@ This function supports reading a CPU Save State register in SMBase relocation ha
>  
>  @retval EFI_SUCCESS           The register was read from Save State.
>  @retval EFI_NOT_FOUND         The register is not defined for the Save State of Processor.
> -@retval EFI_INVALID_PARAMETER  This or Buffer is NULL.
> +@retval EFI_INVALID_PARAMETER Buffer is NULL, or Width does not meet requirement per Register type.
>  
>  **/
>  EFI_STATUS
>