From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web08.7607.1666791706139495697 for ; Wed, 26 Oct 2022 06:41:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=AkDjotx8; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 0DDA0240107 for ; Wed, 26 Oct 2022 15:41:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1666791704; bh=cGzdhxxHW19WKNkVFVDm5jI0+Ulth2wUn68jfP56+uA=; h=From:Subject:Date:Cc:To:From; b=AkDjotx8DTLC0ZnlshSESOGN2Jv2ENCyBBUxptDKY0FK1Jf/PoTS6qfWL34czO09K ojZMB9IyTduQBNTJkNJ1WcRthwblcvz+r+zg0g2rJSptlcExAhRNjtYWv6NO27+mD8 5Vqxie6fuOxJF9wxTJNjIZZmDhu6NZMiLq4WydM/s2nDsrGoHoPfj4i7YVeKSbPWlh WN5JE3bVtcb4o2jCoy3ovFwHHtZcNRvoir/+Hm4Yf1lKMs+ZR6w3yvzt1ZcWE65JDk MkW+6b/APZ1tJT0xIayT4xEO+XmsHqaq4RPc4i8F2S6toWzCxoxLAx50XQgCV3cQhF NVO5I1Zdehofg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4My92Y4yw4z6tqF; Wed, 26 Oct 2022 15:41:41 +0200 (CEST) From: =?utf-8?Q?Marvin_H=C3=A4user?= Mime-Version: 1.0 (1.0) Subject: Re: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in SafeString Date: Wed, 26 Oct 2022 13:41:41 +0000 Message-Id: References: Cc: "Kinney, Michael D" , Pedro Falcato , devel@edk2.groups.io, Vitaly Cheptsov , "Gao, Liming" , "Liu, Zhiguang" In-Reply-To: To: "Yao, Jiewen" Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Comment inline. > On 26. Oct 2022, at 15:35, Yao, Jiewen wrote: >=20 > =EF=BB=BFThat is good catch. >=20 > Reviewed-by: Jiewen Yao >=20 >=20 >> -----Original Message----- >> From: Kinney, Michael D >> Sent: Wednesday, October 26, 2022 12:23 AM >> To: Pedro Falcato ; devel@edk2.groups.io >> Cc: Vitaly Cheptsov ; Marvin H=C3=A4user >> ; Gao, Liming ; Liu, >> Zhiguang ; Yao, Jiewen >> Subject: RE: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in >> SafeString >>=20 >> Adding Jiewen Yao. >>=20 >> Mike >>=20 >>> -----Original Message----- >>> From: Pedro Falcato >>> Sent: Monday, October 24, 2022 3:43 PM >>> To: devel@edk2.groups.io >>> Cc: Pedro Falcato ; Vitaly Cheptsov >> ; Marvin H=C3=A4user ; >>> Kinney, Michael D ; Gao, Liming >> ; Liu, Zhiguang >>> Subject: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in >> SafeString >>>=20 >>> OpenCore folks established an ASAN-equipped project to fuzz Ext4Dxe, >>> which was able to catch these (mostly harmless) issues. >>>=20 >>> Signed-off-by: Pedro Falcato >>> Cc: Vitaly Cheptsov >>> Cc: Marvin H=C3=A4user >>> Cc: Michael D Kinney >>> Cc: Liming Gao >>> Cc: Zhiguang Liu >>> --- >>> MdePkg/Library/BaseLib/SafeString.c | 24 ++++++++++++++++++++---- >>> 1 file changed, 20 insertions(+), 4 deletions(-) >>>=20 >>> 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 =3D FALSE; >>> ASSERT (((UINTN)String & BIT0) =3D=3D 0); >>>=20 >>> // >>> @@ -893,11 +896,12 @@ StrHexToUintnS ( >>> // Ignore leading Zeros after the spaces >>> // >>> while (*String =3D=3D L'0') { >>> + FoundLeadingZero =3D TRUE; I would just assign these outside the loop. Simpler, easier to read and unde= rstand, might emit better bins. Best regards, Marvin >>> String++; >>> } >>>=20 >>> if (CharToUpper (*String) =3D=3D L'X') { >>> - if (*(String - 1) !=3D L'0') { >>> + if (!FoundLeadingZero) { >>> *Data =3D 0; >>> return RETURN_SUCCESS; >>> } >>> @@ -992,6 +996,9 @@ StrHexToUint64S ( >>> OUT UINT64 *Data >>> ) >>> { >>> + BOOLEAN FoundLeadingZero; >>> + >>> + FoundLeadingZero =3D FALSE; >>> ASSERT (((UINTN)String & BIT0) =3D=3D 0); >>>=20 >>> // >>> @@ -1022,11 +1029,12 @@ StrHexToUint64S ( >>> // Ignore leading Zeros after the spaces >>> // >>> while (*String =3D=3D L'0') { >>> + FoundLeadingZero =3D TRUE; >>> String++; >>> } >>>=20 >>> if (CharToUpper (*String) =3D=3D L'X') { >>> - if (*(String - 1) !=3D L'0') { >>> + if (!FoundLeadingZero) { >>> *Data =3D 0; >>> return RETURN_SUCCESS; >>> } >>> @@ -2393,6 +2401,9 @@ AsciiStrHexToUintnS ( >>> OUT UINTN *Data >>> ) >>> { >>> + BOOLEAN FoundLeadingZero; >>> + >>> + FoundLeadingZero =3D FALSE; >>> // >>> // 1. Neither String nor Data shall be a null pointer. >>> // >>> @@ -2421,11 +2432,12 @@ AsciiStrHexToUintnS ( >>> // Ignore leading Zeros after the spaces >>> // >>> while (*String =3D=3D '0') { >>> + FoundLeadingZero =3D TRUE; >>> String++; >>> } >>>=20 >>> if (AsciiCharToUpper (*String) =3D=3D 'X') { >>> - if (*(String - 1) !=3D '0') { >>> + if (!FoundLeadingZero) { >>> *Data =3D 0; >>> return RETURN_SUCCESS; >>> } >>> @@ -2517,6 +2529,9 @@ AsciiStrHexToUint64S ( >>> OUT UINT64 *Data >>> ) >>> { >>> + BOOLEAN FoundLeadingZero; >>> + >>> + FoundLeadingZero =3D FALSE; >>> // >>> // 1. Neither String nor Data shall be a null pointer. >>> // >>> @@ -2545,11 +2560,12 @@ AsciiStrHexToUint64S ( >>> // Ignore leading Zeros after the spaces >>> // >>> while (*String =3D=3D '0') { >>> + FoundLeadingZero =3D TRUE; >>> String++; >>> } >>>=20 >>> if (AsciiCharToUpper (*String) =3D=3D 'X') { >>> - if (*(String - 1) !=3D '0') { >>> + if (!FoundLeadingZero) { >>> *Data =3D 0; >>> return RETURN_SUCCESS; >>> } >>> -- >>> 2.38.1 >=20