Re: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in SafeString

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

Adding Jiewen Yao.

Mike

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Monday, October 24, 2022 3:43 PM
> To: devel@edk2.groups.io
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; Vitaly Cheptsov <vit9696@protonmail.com>; Marvin Häuser <mhaeuser@posteo.de>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in SafeString
> 
> OpenCore folks established an ASAN-equipped project to fuzz Ext4Dxe,
> which was able to catch these (mostly harmless) issues.
> 
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdePkg/Library/BaseLib/SafeString.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
> index f338a32a3a41..77a2585ad56d 100644
> --- a/MdePkg/Library/BaseLib/SafeString.c
> +++ b/MdePkg/Library/BaseLib/SafeString.c
> @@ -863,6 +863,9 @@ StrHexToUintnS (
>    OUT       UINTN   *Data
>    )
>  {
> +  BOOLEAN  FoundLeadingZero;
> +
> +  FoundLeadingZero = FALSE;
>    ASSERT (((UINTN)String & BIT0) == 0);
> 
>    //
> @@ -893,11 +896,12 @@ StrHexToUintnS (
>    // Ignore leading Zeros after the spaces
>    //
>    while (*String == L'0') {
> +    FoundLeadingZero = TRUE;
>      String++;
>    }
> 
>    if (CharToUpper (*String) == L'X') {
> -    if (*(String - 1) != L'0') {
> +    if (!FoundLeadingZero) {
>        *Data = 0;
>        return RETURN_SUCCESS;
>      }
> @@ -992,6 +996,9 @@ StrHexToUint64S (
>    OUT       UINT64  *Data
>    )
>  {
> +  BOOLEAN  FoundLeadingZero;
> +
> +  FoundLeadingZero = FALSE;
>    ASSERT (((UINTN)String & BIT0) == 0);
> 
>    //
> @@ -1022,11 +1029,12 @@ StrHexToUint64S (
>    // Ignore leading Zeros after the spaces
>    //
>    while (*String == L'0') {
> +    FoundLeadingZero = TRUE;
>      String++;
>    }
> 
>    if (CharToUpper (*String) == L'X') {
> -    if (*(String - 1) != L'0') {
> +    if (!FoundLeadingZero) {
>        *Data = 0;
>        return RETURN_SUCCESS;
>      }
> @@ -2393,6 +2401,9 @@ AsciiStrHexToUintnS (
>    OUT       UINTN  *Data
>    )
>  {
> +  BOOLEAN  FoundLeadingZero;
> +
> +  FoundLeadingZero = FALSE;
>    //
>    // 1. Neither String nor Data shall be a null pointer.
>    //
> @@ -2421,11 +2432,12 @@ AsciiStrHexToUintnS (
>    // Ignore leading Zeros after the spaces
>    //
>    while (*String == '0') {
> +    FoundLeadingZero = TRUE;
>      String++;
>    }
> 
>    if (AsciiCharToUpper (*String) == 'X') {
> -    if (*(String - 1) != '0') {
> +    if (!FoundLeadingZero) {
>        *Data = 0;
>        return RETURN_SUCCESS;
>      }
> @@ -2517,6 +2529,9 @@ AsciiStrHexToUint64S (
>    OUT       UINT64  *Data
>    )
>  {
> +  BOOLEAN  FoundLeadingZero;
> +
> +  FoundLeadingZero = FALSE;
>    //
>    // 1. Neither String nor Data shall be a null pointer.
>    //
> @@ -2545,11 +2560,12 @@ AsciiStrHexToUint64S (
>    // Ignore leading Zeros after the spaces
>    //
>    while (*String == '0') {
> +    FoundLeadingZero = TRUE;
>      String++;
>    }
> 
>    if (AsciiCharToUpper (*String) == 'X') {
> -    if (*(String - 1) != '0') {
> +    if (!FoundLeadingZero) {
>        *Data = 0;
>        return RETURN_SUCCESS;
>      }
> --
> 2.38.1