Comment inline.
> On 26. Oct 2022, at 15:35, Yao, Jiewen <jiewen....@intel.com> wrote:
>
> That is good catch.
>
> Reviewed-by: Jiewen Yao <jiewen....@intel.com>
>
>
>> -----Original Message-----
>> From: Kinney, Michael D <michael.d.kin...@intel.com>
>> Sent: Wednesday, October 26, 2022 12:23 AM
>> To: Pedro Falcato <pedro.falc...@gmail.com>; devel@edk2.groups.io
>> Cc: Vitaly Cheptsov <vit9...@protonmail.com>; Marvin Häuser
>> <mhaeu...@posteo.de>; Gao, Liming <gaolim...@byosoft.com.cn>; Liu,
>> Zhiguang <zhiguang....@intel.com>; Yao, Jiewen <jiewen....@intel.com>
>> Subject: RE: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in
>> SafeString
>>
>> Adding Jiewen Yao.
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Pedro Falcato <pedro.falc...@gmail.com>
>>> Sent: Monday, October 24, 2022 3:43 PM
>>> To: devel@edk2.groups.io
>>> Cc: Pedro Falcato <pedro.falc...@gmail.com>; Vitaly Cheptsov
>> <vit9...@protonmail.com>; Marvin Häuser <mhaeu...@posteo.de>;
>>> Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
>> <gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@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.falc...@gmail.com>
>>> Cc: Vitaly Cheptsov <vit9...@protonmail.com>
>>> Cc: Marvin Häuser <mhaeu...@posteo.de>
>>> Cc: Michael D Kinney <michael.d.kin...@intel.com>
>>> Cc: Liming Gao <gaolim...@byosoft.com.cn>
>>> Cc: Zhiguang Liu <zhiguang....@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;
I would just assign these outside the loop. Simpler, easier to read and
understand, might emit better bins.
Best regards,
Marvin
>>> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95635): https://edk2.groups.io/g/devel/message/95635
Mute This Topic: https://groups.io/mt/94546879/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
