Re: [edk2-devel] [PATCHv2 1/2] MdePkg/UefiDevicePathLib: Fix AcpiEx print logic

["Michael D Kinney" <michael.d.kinney@intel.com>]

One comment below.

With that fix,

Reviewed-by: Michael D Kinney <Michael.d.kinney@intel.com>

Mike


> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Wednesday, September 27, 2023 8:51 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCHv2 1/2] MdePkg/UefiDevicePathLib: Fix AcpiEx print
> logic
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4555
> 
> 
> 
> Add logic that checks if the code doesn't overflow
> 
> ACPI_EXTENDED_HID_DEVICE_PATH node when searching for optional
> 
> strings. If the string is not provided in the device path node
> 
> default value of "\0" is used.
> 
> 
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> 
> 
> 
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> 
> ---
> 
>  .../UefiDevicePathLib/DevicePathToText.c      | 69 +++++++++++-------
> -
> 
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 
> 
> 
> diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> 
> index dd90dfa58e..bd8d1de201 100644
> 
> --- a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> 
> +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> 
> @@ -418,23 +418,38 @@ DevPathToTextAcpiEx (
> 
>    )
> 
>  {
> 
>    ACPI_EXTENDED_HID_DEVICE_PATH  *AcpiEx;
> 
> -  CHAR8                          *HIDStr;
> 
> -  CHAR8                          *UIDStr;
> 
> -  CHAR8                          *CIDStr;
> 
>    CHAR16                         HIDText[11];
> 
>    CHAR16                         CIDText[11];
> 
> -
> 
> -  AcpiEx = DevPath;
> 
> -  HIDStr = (CHAR8 *)(((UINT8 *)AcpiEx) + sizeof
> (ACPI_EXTENDED_HID_DEVICE_PATH));
> 
> -  UIDStr = HIDStr + AsciiStrLen (HIDStr) + 1;
> 
> -  CIDStr = UIDStr + AsciiStrLen (UIDStr) + 1;
> 
> +  UINTN                          CurrentLength;
> 
> +  CHAR8                          *CurrentPos;
> 
> +  UINTN                          NextStringOffset;
> 
> +  CHAR8                          *Strings[3];
> 
> +  CONST UINT8                    HidStrIndex = 0;
> 
> +  CONST UINT8                    UidStrIndex = 1;
> 
> +  CONST UINT8                    CidStrIndex = 2;

Locals should not be assigned to valued in declaration.  Please move value assignments down.

> 
> +  UINT8                          StrIndex;
> 
> +
> 
> +  AcpiEx               = DevPath;
> 
> +  Strings[HidStrIndex] = NULL;
> 
> +  Strings[UidStrIndex] = NULL;
> 
> +  Strings[CidStrIndex] = NULL;
> 
> +  CurrentLength        = sizeof (ACPI_EXTENDED_HID_DEVICE_PATH);
> 
> +  CurrentPos           = (CHAR8 *)(((UINT8 *)AcpiEx) + sizeof
> (ACPI_EXTENDED_HID_DEVICE_PATH));
> 
> +  StrIndex             = 0;
> 
> +  while (CurrentLength < AcpiEx->Header.Length[0] && StrIndex <
> ARRAY_SIZE (Strings)) {
> 
> +    Strings[StrIndex] = CurrentPos;
> 
> +    NextStringOffset  = AsciiStrLen (CurrentPos) + 1;
> 
> +    CurrentLength    += NextStringOffset;
> 
> +    CurrentPos       += NextStringOffset;
> 
> +    StrIndex++;
> 
> +  }
> 
> 
> 
>    if (DisplayOnly) {
> 
>      if ((EISA_ID_TO_NUM (AcpiEx->HID) == 0x0A03) ||
> 
>          ((EISA_ID_TO_NUM (AcpiEx->CID) == 0x0A03) && (EISA_ID_TO_NUM
> (AcpiEx->HID) != 0x0A08)))
> 
>      {
> 
> -      if (AcpiEx->UID == 0) {
> 
> -        UefiDevicePathLibCatPrint (Str, L"PciRoot(%a)", UIDStr);
> 
> +      if (Strings[UidStrIndex] != NULL) {
> 
> +        UefiDevicePathLibCatPrint (Str, L"PciRoot(%a)",
> Strings[UidStrIndex]);
> 
>        } else {
> 
>          UefiDevicePathLibCatPrint (Str, L"PciRoot(0x%x)", AcpiEx-
> >UID);
> 
>        }
> 
> @@ -443,8 +458,8 @@ DevPathToTextAcpiEx (
> 
>      }
> 
> 
> 
>      if ((EISA_ID_TO_NUM (AcpiEx->HID) == 0x0A08) || (EISA_ID_TO_NUM
> (AcpiEx->CID) == 0x0A08)) {
> 
> -      if (AcpiEx->UID == 0) {
> 
> -        UefiDevicePathLibCatPrint (Str, L"PcieRoot(%a)", UIDStr);
> 
> +      if (Strings[UidStrIndex] != NULL) {
> 
> +        UefiDevicePathLibCatPrint (Str, L"PcieRoot(%a)",
> Strings[UidStrIndex]);
> 
>        } else {
> 
>          UefiDevicePathLibCatPrint (Str, L"PcieRoot(0x%x)", AcpiEx-
> >UID);
> 
>        }
> 
> @@ -475,7 +490,10 @@ DevPathToTextAcpiEx (
> 
>      (AcpiEx->CID >> 16) & 0xFFFF
> 
>      );
> 
> 
> 
> -  if ((*HIDStr == '\0') && (*CIDStr == '\0') && (*UIDStr != '\0')) {
> 
> +  if (((Strings[HidStrIndex] != NULL) && (*Strings[HidStrIndex] ==
> '\0')) &&
> 
> +      ((Strings[CidStrIndex] != NULL) && (*Strings[CidStrIndex] ==
> '\0')) &&
> 
> +      ((Strings[UidStrIndex] != NULL) && (*Strings[UidStrIndex] !=
> '\0')))
> 
> +  {
> 
>      //
> 
>      // use AcpiExp()
> 
>      //
> 
> @@ -484,7 +502,7 @@ DevPathToTextAcpiEx (
> 
>          Str,
> 
>          L"AcpiExp(%s,0,%a)",
> 
>          HIDText,
> 
> -        UIDStr
> 
> +        Strings[UidStrIndex]
> 
>          );
> 
>      } else {
> 
>        UefiDevicePathLibCatPrint (
> 
> @@ -492,28 +510,25 @@ DevPathToTextAcpiEx (
> 
>          L"AcpiExp(%s,%s,%a)",
> 
>          HIDText,
> 
>          CIDText,
> 
> -        UIDStr
> 
> +        Strings[UidStrIndex]
> 
>          );
> 
>      }
> 
>    } else {
> 
>      if (DisplayOnly) {
> 
> -      //
> 
> -      // display only
> 
> -      //
> 
> -      if (AcpiEx->HID == 0) {
> 
> -        UefiDevicePathLibCatPrint (Str, L"AcpiEx(%a,", HIDStr);
> 
> +      if (Strings[HidStrIndex] != NULL) {
> 
> +        UefiDevicePathLibCatPrint (Str, L"AcpiEx(%a,",
> Strings[HidStrIndex]);
> 
>        } else {
> 
>          UefiDevicePathLibCatPrint (Str, L"AcpiEx(%s,", HIDText);
> 
>        }
> 
> 
> 
> -      if (AcpiEx->CID == 0) {
> 
> -        UefiDevicePathLibCatPrint (Str, L"%a,", CIDStr);
> 
> +      if (Strings[CidStrIndex] != NULL) {
> 
> +        UefiDevicePathLibCatPrint (Str, L"%a,",
> Strings[CidStrIndex]);
> 
>        } else {
> 
>          UefiDevicePathLibCatPrint (Str, L"%s,", CIDText);
> 
>        }
> 
> 
> 
> -      if (AcpiEx->UID == 0) {
> 
> -        UefiDevicePathLibCatPrint (Str, L"%a)", UIDStr);
> 
> +      if (Strings[UidStrIndex] != NULL) {
> 
> +        UefiDevicePathLibCatPrint (Str, L"%a)",
> Strings[UidStrIndex]);
> 
>        } else {
> 
>          UefiDevicePathLibCatPrint (Str, L"0x%x)", AcpiEx->UID);
> 
>        }
> 
> @@ -524,9 +539,9 @@ DevPathToTextAcpiEx (
> 
>          HIDText,
> 
>          CIDText,
> 
>          AcpiEx->UID,
> 
> -        HIDStr,
> 
> -        CIDStr,
> 
> -        UIDStr
> 
> +        Strings[HidStrIndex] != NULL ? Strings[HidStrIndex] : '\0',
> 
> +        Strings[CidStrIndex] != NULL ? Strings[CidStrIndex] : '\0',
> 
> +        Strings[UidStrIndex] != NULL ? Strings[UidStrIndex] : '\0'
> 
>          );
> 
>      }
> 
>    }
> 
> --
> 
> 2.39.2
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109595): https://edk2.groups.io/g/devel/message/109595
Mute This Topic: https://groups.io/mt/101619978/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-