* [PATCH v1 0/1] Add buffer size check before save state read
@ 2021-04-06 19:52 Kun Qin
2021-04-06 19:52 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing Kun Qin
0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2021-04-06 19:52 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
2021-04-06 19:52 [PATCH v1 0/1] Add buffer size check before save state read Kun Qin
@ 2021-04-06 19:52 ` Kun Qin
2021-04-07 16:08 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2021-04-06 19:52 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
2021-04-06 19:52 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing Kun Qin
@ 2021-04-07 16:08 ` Laszlo Ersek
2021-04-12 17:36 ` [edk2-devel] " Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-04-07 16:08 UTC (permalink / raw)
To: Kun Qin, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
2021-04-07 16:08 ` Laszlo Ersek
@ 2021-04-12 17:36 ` Laszlo Ersek
2021-04-12 17:43 ` Kun Qin
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-04-12 17:36 UTC (permalink / raw)
To: Kun Qin, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
2021-04-12 17:36 ` [edk2-devel] " Laszlo Ersek
@ 2021-04-12 17:43 ` Kun Qin
0 siblings, 0 replies; 6+ messages in thread
From: Kun Qin @ 2021-04-12 17:43 UTC (permalink / raw)
To: devel, lersek; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 0/1] Add buffer size check before save state read
@ 2021-03-26 23:41 Kun Qin
2021-03-26 23:41 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing Kun Qin
0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2021-03-26 23:41 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing
2021-03-26 23:41 [PATCH v1 0/1] Add buffer size check before save state read Kun Qin
@ 2021-03-26 23:41 ` Kun Qin
0 siblings, 0 replies; 6+ messages in thread
From: Kun Qin @ 2021-03-26 23:41 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-12 17:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-06 19:52 [PATCH v1 0/1] Add buffer size check before save state read Kun Qin
2021-04-06 19:52 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing Kun Qin
2021-04-07 16:08 ` Laszlo Ersek
2021-04-12 17:36 ` [edk2-devel] " Laszlo Ersek
2021-04-12 17:43 ` Kun Qin
-- strict thread matches above, loose matches on Subject: below --
2021-03-26 23:41 [PATCH v1 0/1] Add buffer size check before save state read Kun Qin
2021-03-26 23:41 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing Kun Qin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox