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

[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 added buffer width check before copying IO information into
incoming buffer. This bug could potentially corrupt memory due to out of
buffer boundary memory access.

Patch v1 branch: https://github.com/kuqin12/edk2/tree/svst_width_v1

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 | 7 +++++++
 1 file changed, 7 insertions(+)

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

[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
>