1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow

Patrick Rudolph posted 1 patch 3 months, 3 weeks ago
Failed in applying to current master (apply log)
UefiPayloadPkg/Library/CbParseLib/CbParseLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[edk2-devel] [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow
Posted by Patrick Rudolph 3 months, 3 weeks ago 
The IMD entry uses the 32bit start field as relative offset
to root. On Ia32X64 this works fine as UINTN is also 32 bit and
negative relative offsets are properly calculated due to an
integer overflow.

On X64 this doesn't work as UINTN is 64 bit and the offset
is no longer subtracted, but it's added to the root. Fix that
by sign extending the start field to 64 bit.

Test: Booting UefiPayloadPkg still works on Ia32X64 and now also
      works on X64.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 UefiPayloadPkg/Library/CbParseLib/CbParseLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
index 8a353f77f6..9e149532a7 100644
--- a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
+++ b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
@@ -282,7 +282,7 @@ FindCbMemTable (
   for (Idx = 0; Idx < Root->num_entries; Idx++) {
     if (Entries[Idx].id == TableId) {
       if (IsImdEntry) {
-        *MemTable = (VOID *)((UINTN)Entries[Idx].start + (UINTN)Root);
+        *MemTable = (VOID *)((INTN)(INT32)Entries[Idx].start + (UINTN)Root);
       } else {
         *MemTable = (VOID *)(UINTN)Entries[Idx].start;
       }
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113361): https://edk2.groups.io/g/devel/message/113361
Mute This Topic: https://groups.io/mt/103593206/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow
Posted by Sheng Lean Tan 3 months, 2 weeks ago 
Hi Gua or Sean,
Would you mind to help review this?
Thanks!

Best Regards,
*Lean Sheng Tan*



9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany
Email: sheng.tan@9elements.com
Phone: *+49 234 68 94 188 <+492346894188>*
Mobile: *+49 176 76 113842 <+4917676113842>*

Registered office: Bochum
Commercial register: Amtsgericht Bochum, HRB 17519
Management: Sebastian German, Eray Bazaar

Data protection information according to Art. 13 GDPR
<https://9elements.com/privacy>


On Mon, 8 Jan 2024 at 08:00, Patrick Rudolph <patrick.rudolph@9elements.com>
wrote:

> The IMD entry uses the 32bit start field as relative offset
> to root. On Ia32X64 this works fine as UINTN is also 32 bit and
> negative relative offsets are properly calculated due to an
> integer overflow.
>
> On X64 this doesn't work as UINTN is 64 bit and the offset
> is no longer subtracted, but it's added to the root. Fix that
> by sign extending the start field to 64 bit.
>
> Test: Booting UefiPayloadPkg still works on Ia32X64 and now also
>       works on X64.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  UefiPayloadPkg/Library/CbParseLib/CbParseLib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> index 8a353f77f6..9e149532a7 100644
> --- a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> +++ b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> @@ -282,7 +282,7 @@ FindCbMemTable (
>    for (Idx = 0; Idx < Root->num_entries; Idx++) {
>      if (Entries[Idx].id == TableId) {
>        if (IsImdEntry) {
> -        *MemTable = (VOID *)((UINTN)Entries[Idx].start + (UINTN)Root);
> +        *MemTable = (VOID *)((INTN)(INT32)Entries[Idx].start +
> (UINTN)Root);
>        } else {
>          *MemTable = (VOID *)(UINTN)Entries[Idx].start;
>        }
> --
> 2.43.0
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113713): https://edk2.groups.io/g/devel/message/113713
Mute This Topic: https://groups.io/mt/103593206/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow
Posted by Guo, Gua 3 months, 2 weeks ago 
Reviewed-by: Gua Guo <gua.guo@intel.com>
________________________________
From: Lean Sheng Tan <sheng.tan@9elements.com>
Sent: Friday, January 12, 2024 7:33:00 PM
To: Rudolph, Patrick <patrick.rudolph@9elements.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Rhodes, Sean <sean@starlabs.systems>; Guo, Gua <gua.guo@intel.com>; Lu, James <james.lu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong, Guo <guo.dong@intel.com>
Subject: Re: [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow

Hi Gua or Sean,
Would you mind to help review this?
Thanks!

Best Regards,
Lean Sheng Tan

[http://static.9elements.com/logo-signature.png]

9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany
Email: sheng.tan@9elements.com<mailto:sheng.tan@9elements.com>
Phone: +49 234 68 94 188<tel:+492346894188>
Mobile: +49 176 76 113842<tel:+4917676113842>

Registered office: Bochum
Commercial register: Amtsgericht Bochum, HRB 17519
Management: Sebastian German, Eray Bazaar

Data protection information according to Art. 13 GDPR<https://9elements.com/privacy>


On Mon, 8 Jan 2024 at 08:00, Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>> wrote:
The IMD entry uses the 32bit start field as relative offset
to root. On Ia32X64 this works fine as UINTN is also 32 bit and
negative relative offsets are properly calculated due to an
integer overflow.

On X64 this doesn't work as UINTN is 64 bit and the offset
is no longer subtracted, but it's added to the root. Fix that
by sign extending the start field to 64 bit.

Test: Booting UefiPayloadPkg still works on Ia32X64 and now also
      works on X64.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>
---
 UefiPayloadPkg/Library/CbParseLib/CbParseLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
index 8a353f77f6..9e149532a7 100644
--- a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
+++ b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
@@ -282,7 +282,7 @@ FindCbMemTable (
   for (Idx = 0; Idx < Root->num_entries; Idx++) {
     if (Entries[Idx].id == TableId) {
       if (IsImdEntry) {
-        *MemTable = (VOID *)((UINTN)Entries[Idx].start + (UINTN)Root);
+        *MemTable = (VOID *)((INTN)(INT32)Entries[Idx].start + (UINTN)Root);
       } else {
         *MemTable = (VOID *)(UINTN)Entries[Idx].start;
       }
--
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113720): https://edk2.groups.io/g/devel/message/113720
Mute This Topic: https://groups.io/mt/103593206/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow
Posted by Sean Rhodes 3 months, 2 weeks ago 
Reviewed-by: Sean Rhodes <sean@starlabs.systems>

On Fri, 12 Jan 2024 at 11:43, Guo, Gua <gua.guo@intel.com> wrote:

> Reviewed-by: Gua Guo <gua.guo@intel.com>
> ------------------------------
> *From:* Lean Sheng Tan <sheng.tan@9elements.com>
> *Sent:* Friday, January 12, 2024 7:33:00 PM
> *To:* Rudolph, Patrick <patrick.rudolph@9elements.com>
> *Cc:* devel@edk2.groups.io <devel@edk2.groups.io>; Rhodes, Sean
> <sean@starlabs.systems>; Guo, Gua <gua.guo@intel.com>; Lu, James <
> james.lu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong, Guo <
> guo.dong@intel.com>
> *Subject:* Re: [PATCH] UefiPayloadPkg: CbParseLib: Fix integer overflow
>
> Hi Gua or Sean,
> Would you mind to help review this?
> Thanks!
>
> Best Regards,
> *Lean Sheng Tan*
>
>
>
> 9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany
> Email: sheng.tan@9elements.com
> Phone: *+49 234 68 94 188 <+492346894188>*
> Mobile: *+49 176 76 113842 <+4917676113842>*
>
> Registered office: Bochum
> Commercial register: Amtsgericht Bochum, HRB 17519
> Management: Sebastian German, Eray Bazaar
>
> Data protection information according to Art. 13 GDPR
> <https://9elements.com/privacy>
>
>
> On Mon, 8 Jan 2024 at 08:00, Patrick Rudolph <
> patrick.rudolph@9elements.com> wrote:
>
> The IMD entry uses the 32bit start field as relative offset
> to root. On Ia32X64 this works fine as UINTN is also 32 bit and
> negative relative offsets are properly calculated due to an
> integer overflow.
>
> On X64 this doesn't work as UINTN is 64 bit and the offset
> is no longer subtracted, but it's added to the root. Fix that
> by sign extending the start field to 64 bit.
>
> Test: Booting UefiPayloadPkg still works on Ia32X64 and now also
>       works on X64.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  UefiPayloadPkg/Library/CbParseLib/CbParseLib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> index 8a353f77f6..9e149532a7 100644
> --- a/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> +++ b/UefiPayloadPkg/Library/CbParseLib/CbParseLib.c
> @@ -282,7 +282,7 @@ FindCbMemTable (
>    for (Idx = 0; Idx < Root->num_entries; Idx++) {
>      if (Entries[Idx].id == TableId) {
>        if (IsImdEntry) {
> -        *MemTable = (VOID *)((UINTN)Entries[Idx].start + (UINTN)Root);
> +        *MemTable = (VOID *)((INTN)(INT32)Entries[Idx].start +
> (UINTN)Root);
>        } else {
>          *MemTable = (VOID *)(UINTN)Entries[Idx].start;
>        }
> --
> 2.43.0
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113812): https://edk2.groups.io/g/devel/message/113812
Mute This Topic: https://groups.io/mt/103593206/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.