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

["Pedro Falcato" <pedro.falcato@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5890 bytes --]

Can someone push this? Is there a blocker here?

On Wed, Oct 26, 2022 at 4:54 PM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> Acked-by: Michael D Kinney <michael.d.kinney@intel.com>
>
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Wednesday, October 26, 2022 6:35 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <
> pedro.falcato@gmail.com>; devel@edk2.groups.io
> > Cc: Vitaly Cheptsov <vit9696@protonmail.com>; Marvin Häuser <
> mhaeuser@posteo.de>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> > Zhiguang <zhiguang.liu@intel.com>
> > Subject: RE: [PATCH v2 1/1] MdePkg/BaseLib: Fix out-of-bounds reads in
> SafeString
> >
> > That is good catch.
> >
> > Reviewed-by: Jiewen Yao <Jiewen.yao@Intel.com>
> >
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Sent: Wednesday, October 26, 2022 12:23 AM
> > > To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io
> > > Cc: Vitaly Cheptsov <vit9696@protonmail.com>; Marvin Häuser
> > > <mhaeuser@posteo.de>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> > > Zhiguang <zhiguang.liu@intel.com>; Yao, Jiewen <jiewen.yao@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.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
>
>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 10161 bytes --]