[PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Vladimir Olovyannikov]

If the size of the supplied buffer in FileHandleReadLine(), module
UefiFileHandleLib.c, was not 0, but was not enough to fit in
the line, the size is increased, and then the Buffer of the new
size is zeroed. This size is always larger than the supplied buffer size,
causing supplied buffer overrun. Fix the issue by using the
supplied buffer size in ZeroMem().

Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 28e28e5f67d5..ab34e6ccd5f4 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -969,6 +969,7 @@ FileHandleReadLine(
   UINTN       CharSize;
   UINTN       CountSoFar;
   UINTN       CrCount;
+  UINTN       OldSize;
   UINT64      OriginalFilePosition;
 
   if (Handle == NULL
@@ -1039,10 +1040,11 @@ FileHandleReadLine(
   // if we ran out of space tell when...
   //
   if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
+    OldSize = *Size;
     *Size = (CountSoFar+1-CrCount)*sizeof(CHAR16);
     if (!Truncate) {
-      if (Buffer != NULL && *Size != 0) {
-        ZeroMem(Buffer, *Size);
+      if (Buffer != NULL && OldSize != 0) {
+        ZeroMem(Buffer, OldSize);
       }
       FileHandleSetPosition(Handle, OriginalFilePosition);
       return (EFI_BUFFER_TOO_SMALL);
-- 
2.26.2.266.ge870325ee8

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Zhiguang Liu]

Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir
> Olovyannikov via groups.io
> Sent: Thursday, July 2, 2020 10:31 AM
> To: devel@edk2.groups.io
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer
> overrun in FileHandleReadLine()
> 
> If the size of the supplied buffer in FileHandleReadLine(), module
> UefiFileHandleLib.c, was not 0, but was not enough to fit in
> the line, the size is increased, and then the Buffer of the new
> size is zeroed. This size is always larger than the supplied buffer size,
> causing supplied buffer overrun. Fix the issue by using the
> supplied buffer size in ZeroMem().
> 
> Signed-off-by: Vladimir Olovyannikov
> <vladimir.olovyannikov@broadcom.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 28e28e5f67d5..ab34e6ccd5f4 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -969,6 +969,7 @@ FileHandleReadLine(
>    UINTN       CharSize;
> 
>    UINTN       CountSoFar;
> 
>    UINTN       CrCount;
> 
> +  UINTN       OldSize;
> 
>    UINT64      OriginalFilePosition;
> 
> 
> 
>    if (Handle == NULL
> 
> @@ -1039,10 +1040,11 @@ FileHandleReadLine(
>    // if we ran out of space tell when...
> 
>    //
> 
>    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
> 
> +    OldSize = *Size;
> 
>      *Size = (CountSoFar+1-CrCount)*sizeof(CHAR16);
> 
>      if (!Truncate) {
> 
> -      if (Buffer != NULL && *Size != 0) {
> 
> -        ZeroMem(Buffer, *Size);
> 
> +      if (Buffer != NULL && OldSize != 0) {
> 
> +        ZeroMem(Buffer, OldSize);
> 
>        }
> 
>        FileHandleSetPosition(Handle, OriginalFilePosition);
> 
>        return (EFI_BUFFER_TOO_SMALL);
> 
> --
> 2.26.2.266.ge870325ee8
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#61938): https://edk2.groups.io/g/devel/message/61938
> Mute This Topic: https://groups.io/mt/75251007/1779286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [zhiguang.liu@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Laszlo Ersek]

On 07/03/20 04:30, Zhiguang Liu wrote:
> Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>

Merged as commit 4535fc312b76, via
<https://github.com/tianocore/edk2/pull/896>.

Thanks,
Laszlo

> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir
>> Olovyannikov via groups.io
>> Sent: Thursday, July 2, 2020 10:31 AM
>> To: devel@edk2.groups.io
>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer
>> overrun in FileHandleReadLine()
>>
>> If the size of the supplied buffer in FileHandleReadLine(), module
>> UefiFileHandleLib.c, was not 0, but was not enough to fit in
>> the line, the size is increased, and then the Buffer of the new
>> size is zeroed. This size is always larger than the supplied buffer size,
>> causing supplied buffer overrun. Fix the issue by using the
>> supplied buffer size in ZeroMem().
>>
>> Signed-off-by: Vladimir Olovyannikov
>> <vladimir.olovyannikov@broadcom.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> ---
>>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> index 28e28e5f67d5..ab34e6ccd5f4 100644
>> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> @@ -969,6 +969,7 @@ FileHandleReadLine(
>>    UINTN       CharSize;
>>
>>    UINTN       CountSoFar;
>>
>>    UINTN       CrCount;
>>
>> +  UINTN       OldSize;
>>
>>    UINT64      OriginalFilePosition;
>>
>>
>>
>>    if (Handle == NULL
>>
>> @@ -1039,10 +1040,11 @@ FileHandleReadLine(
>>    // if we ran out of space tell when...
>>
>>    //
>>
>>    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
>>
>> +    OldSize = *Size;
>>
>>      *Size = (CountSoFar+1-CrCount)*sizeof(CHAR16);
>>
>>      if (!Truncate) {
>>
>> -      if (Buffer != NULL && *Size != 0) {
>>
>> -        ZeroMem(Buffer, *Size);
>>
>> +      if (Buffer != NULL && OldSize != 0) {
>>
>> +        ZeroMem(Buffer, OldSize);
>>
>>        }
>>
>>        FileHandleSetPosition(Handle, OriginalFilePosition);
>>
>>        return (EFI_BUFFER_TOO_SMALL);
>>
>> --
>> 2.26.2.266.ge870325ee8

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Laszlo Ersek]

On 08/24/20 18:18, Laszlo Ersek wrote:
> On 07/03/20 04:30, Zhiguang Liu wrote:
>> Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
> 
> Merged as commit 4535fc312b76, via
> <https://github.com/tianocore/edk2/pull/896>.

The commit message does not mention a TianoCore BZ. If there *is* an
associated TianoCore BZ, please set it to RESOLVED|FIXED now, and also
mark the above commit hash in a comment on it.

Thanks
Laszlo

> 
> Thanks,
> Laszlo
> 
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir
>>> Olovyannikov via groups.io
>>> Sent: Thursday, July 2, 2020 10:31 AM
>>> To: devel@edk2.groups.io
>>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Kinney,
>>> Michael D <michael.d.kinney@intel.com>; Gao, Liming
>>> <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer
>>> overrun in FileHandleReadLine()
>>>
>>> If the size of the supplied buffer in FileHandleReadLine(), module
>>> UefiFileHandleLib.c, was not 0, but was not enough to fit in
>>> the line, the size is increased, and then the Buffer of the new
>>> size is zeroed. This size is always larger than the supplied buffer size,
>>> causing supplied buffer overrun. Fix the issue by using the
>>> supplied buffer size in ZeroMem().
>>>
>>> Signed-off-by: Vladimir Olovyannikov
>>> <vladimir.olovyannikov@broadcom.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>> ---
>>>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>>> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>>> index 28e28e5f67d5..ab34e6ccd5f4 100644
>>> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>>> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>>> @@ -969,6 +969,7 @@ FileHandleReadLine(
>>>    UINTN       CharSize;
>>>
>>>    UINTN       CountSoFar;
>>>
>>>    UINTN       CrCount;
>>>
>>> +  UINTN       OldSize;
>>>
>>>    UINT64      OriginalFilePosition;
>>>
>>>
>>>
>>>    if (Handle == NULL
>>>
>>> @@ -1039,10 +1040,11 @@ FileHandleReadLine(
>>>    // if we ran out of space tell when...
>>>
>>>    //
>>>
>>>    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
>>>
>>> +    OldSize = *Size;
>>>
>>>      *Size = (CountSoFar+1-CrCount)*sizeof(CHAR16);
>>>
>>>      if (!Truncate) {
>>>
>>> -      if (Buffer != NULL && *Size != 0) {
>>>
>>> -        ZeroMem(Buffer, *Size);
>>>
>>> +      if (Buffer != NULL && OldSize != 0) {
>>>
>>> +        ZeroMem(Buffer, OldSize);
>>>
>>>        }
>>>
>>>        FileHandleSetPosition(Handle, OriginalFilePosition);
>>>
>>>        return (EFI_BUFFER_TOO_SMALL);
>>>
>>> --
>>> 2.26.2.266.ge870325ee8
> 

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Vladimir Olovyannikov]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, August 24, 2020 9:52 AM
> To: devel@edk2.groups.io; zhiguang.liu@intel.com;
> vladimir.olovyannikov@broadcom.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix
> buffer overrun in FileHandleReadLine()
>
> On 08/24/20 18:18, Laszlo Ersek wrote:
> > On 07/03/20 04:30, Zhiguang Liu wrote:
> >> Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
> >
> > Merged as commit 4535fc312b76, via
> > <https://github.com/tianocore/edk2/pull/896>.
>
> The commit message does not mention a TianoCore BZ. If there *is* an
> associated TianoCore BZ, please set it to RESOLVED|FIXED now, and also
> mark the above commit hash in a comment on it.
>
> Thanks
> Laszlo
Thank you Laszlo.
I modified the BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2783 as you
suggested.

Thank you,
Vladimir
>
> >
> > Thanks,
> > Laszlo
> >
> >>
> >>> -----Original Message-----
> >>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>> Vladimir Olovyannikov via groups.io
> >>> Sent: Thursday, July 2, 2020 10:31 AM
> >>> To: devel@edk2.groups.io
> >>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
> >>> Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> >>> <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> >>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix
> >>> buffer overrun in FileHandleReadLine()
> >>>
> >>> If the size of the supplied buffer in FileHandleReadLine(), module
> >>> UefiFileHandleLib.c, was not 0, but was not enough to fit in the
> >>> line, the size is increased, and then the Buffer of the new size is
> >>> zeroed. This size is always larger than the supplied buffer size,
> >>> causing supplied buffer overrun. Fix the issue by using the supplied
> >>> buffer size in ZeroMem().
> >>>
> >>> Signed-off-by: Vladimir Olovyannikov
> >>> <vladimir.olovyannikov@broadcom.com>
> >>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> >>> ---
> >>>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 6 ++++--
> >>>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> >>> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> >>> index 28e28e5f67d5..ab34e6ccd5f4 100644
> >>> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> >>> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> >>> @@ -969,6 +969,7 @@ FileHandleReadLine(
> >>>    UINTN       CharSize;
> >>>
> >>>    UINTN       CountSoFar;
> >>>
> >>>    UINTN       CrCount;
> >>>
> >>> +  UINTN       OldSize;
> >>>
> >>>    UINT64      OriginalFilePosition;
> >>>
> >>>
> >>>
> >>>    if (Handle == NULL
> >>>
> >>> @@ -1039,10 +1040,11 @@ FileHandleReadLine(
> >>>    // if we ran out of space tell when...
> >>>
> >>>    //
> >>>
> >>>    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
> >>>
> >>> +    OldSize = *Size;
> >>>
> >>>      *Size = (CountSoFar+1-CrCount)*sizeof(CHAR16);
> >>>
> >>>      if (!Truncate) {
> >>>
> >>> -      if (Buffer != NULL && *Size != 0) {
> >>>
> >>> -        ZeroMem(Buffer, *Size);
> >>>
> >>> +      if (Buffer != NULL && OldSize != 0) {
> >>>
> >>> +        ZeroMem(Buffer, OldSize);
> >>>
> >>>        }
> >>>
> >>>        FileHandleSetPosition(Handle, OriginalFilePosition);
> >>>
> >>>        return (EFI_BUFFER_TOO_SMALL);
> >>>
> >>> --
> >>> 2.26.2.266.ge870325ee8
> >

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Laszlo Ersek]

On 08/25/20 06:20, Vladimir Olovyannikov wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Monday, August 24, 2020 9:52 AM
>> To: devel@edk2.groups.io; zhiguang.liu@intel.com;
>> vladimir.olovyannikov@broadcom.com
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix
>> buffer overrun in FileHandleReadLine()
>>
>> On 08/24/20 18:18, Laszlo Ersek wrote:
>>> On 07/03/20 04:30, Zhiguang Liu wrote:
>>>> Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
>>>
>>> Merged as commit 4535fc312b76, via
>>> <https://github.com/tianocore/edk2/pull/896>.
>>
>> The commit message does not mention a TianoCore BZ. If there *is* an
>> associated TianoCore BZ, please set it to RESOLVED|FIXED now, and also
>> mark the above commit hash in a comment on it.
>>
>> Thanks
>> Laszlo
> Thank you Laszlo.
> I modified the BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2783 as you
> suggested.

Thanks!

In the future, if a patch is being posted for a TianoCore BZ, then
please include a line in the commit message like this:

"""
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2783
"""

Because this lets us go from git history to BZ ticket.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Vladimir Olovyannikov]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, August 26, 2020 3:03 AM
> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
> devel@edk2.groups.io; zhiguang.liu@intel.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib: fix
> buffer overrun in FileHandleReadLine()
>
> On 08/25/20 06:20, Vladimir Olovyannikov wrote:
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Monday, August 24, 2020 9:52 AM
> >> To: devel@edk2.groups.io; zhiguang.liu@intel.com;
> >> vladimir.olovyannikov@broadcom.com
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg : UefiFileHandleLib:
> >> fix buffer overrun in FileHandleReadLine()
> >>
> >> On 08/24/20 18:18, Laszlo Ersek wrote:
> >>> On 07/03/20 04:30, Zhiguang Liu wrote:
> >>>> Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
> >>>
> >>> Merged as commit 4535fc312b76, via
> >>> <https://github.com/tianocore/edk2/pull/896>.
> >>
> >> The commit message does not mention a TianoCore BZ. If there *is* an
> >> associated TianoCore BZ, please set it to RESOLVED|FIXED now, and
> >> also mark the above commit hash in a comment on it.
> >>
> >> Thanks
> >> Laszlo
> > Thank you Laszlo.
> > I modified the BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2783
> > as you suggested.
>
> Thanks!
>
> In the future, if a patch is being posted for a TianoCore BZ, then please
> include a line in the commit message like this:
>
> """
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2783
> """
>
> Because this lets us go from git history to BZ ticket.
Sure, will do!
>
> Thanks!
> Laszlo