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

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Saturday, March 27, 2021 7:42 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
> 
> 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>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> index 661cc51f361a..ec760e4c37ca 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> @@ -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
>      //
> --
> 2.31.0.windows.1
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

On 03/27/21 00:41, 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>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> index 661cc51f361a..ec760e4c37ca 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
> @@ -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
>      //
> 

Please update the description of the EFI_INVALID_PARAMETER return code
in the function's leading comment as well.

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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

[Kun Qin]

Hi Laszlo,

Thanks for the feedback. I will update the description in v2.

Regards,
Kun

On 04/06/2021 05:09, Laszlo Ersek wrote:
> On 03/27/21 00:41, 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>
>> ---
>>   UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
>> index 661cc51f361a..ec760e4c37ca 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
>> @@ -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
>>       //
>>
> 
> Please update the description of the EFI_INVALID_PARAMETER return code
> in the function's leading comment as well.
> 
> With that:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 

[PATCH v1 0/1] Add buffer size check before save state read

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3283

This change is validated on two different physical platforms.

v2 mainly focus on feedback for v1 patche, including:
a. Adding "Reviewed-by" tags;
b. Updated return code description for modified function;

Patch v2 branch: https://github.com/kuqin12/edk2/tree/svst_width_v2

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>

Kun Qin (1):
  UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing

 UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 9 ++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.31.0.windows.1

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

[Kun Qin]

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(-)

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
-- 
2.31.0.windows.1

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

[Laszlo Ersek]

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
> 

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

[Laszlo Ersek]

On 04/07/21 18:08, Laszlo Ersek wrote:
> 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.

:/

Merged as commit a7d8e28b29f2, via
<https://github.com/tianocore/edk2/pull/1554>.

Laszlo

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

[Kun Qin]

Hi Laszlo,

Thanks for the help.

Regards,
Kun

On 04/12/2021 10:36, Laszlo Ersek wrote:
> On 04/07/21 18:08, Laszlo Ersek wrote:
>> 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.
> 
> :/
> 
> Merged as commit a7d8e28b29f2, via
> <https://github.com/tianocore/edk2/pull/1554>.
> 
> Laszlo
>