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

[PATCH v1 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>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 28e28e5f67d5..4bc9fabb6c74 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -1039,10 +1039,13 @@ FileHandleReadLine(
   // if we ran out of space tell when...
   //
   if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
+    UINTN OldSize;
+
+    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 v1 1/1] MdePkg : UefiFileHandleLib: fix buffer overrun in FileHandleReadLine()

[Zhiguang Liu]

Hi Vladimir
Thanks for catching this bug. And I agree with you about your code change.
One little problem is that we always define the variable in the beginning of the function.
Please fix the little issue and I will give my RB.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir
> Olovyannikov via groups.io
> Sent: Friday, June 5, 2020 8:18 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>
> Subject: [edk2-devel] [PATCH v1 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>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 28e28e5f67d5..4bc9fabb6c74 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -1039,10 +1039,13 @@ FileHandleReadLine(
>    // if we ran out of space tell when...
> 
>    //
> 
>    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
> 
> +    UINTN OldSize;
> 
> +
> 
> +    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 (#60736): https://edk2.groups.io/g/devel/message/60736
> Mute This Topic: https://groups.io/mt/74683735/1779286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [zhiguang.liu@intel.com]
> -=-=-=-=-=-=

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

[Vladimir Olovyannikov]

Hi Zhiguang,

Thank you for reviewing my commit.
I thought, the variable was used only once in the if {} statement, so had
it declared only there.
I will declare it in the beginning of the function if it is a standard.
Will submit patch v2.

Thank you,
Vladimir
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Saturday, June 27, 2020 11:31 PM
> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg : UefiFileHandleLib: fix
> buffer overrun in FileHandleReadLine()
>
> Hi Vladimir
> Thanks for catching this bug. And I agree with you about your code
change.
> One little problem is that we always define the variable in the
beginning of
> the function.
> Please fix the little issue and I will give my RB.
>
> Thanks
> Zhiguang
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Vladimir Olovyannikov via groups.io
> > Sent: Friday, June 5, 2020 8:18 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>
> > Subject: [edk2-devel] [PATCH v1 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>
> > ---
> >  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> > b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> > index 28e28e5f67d5..4bc9fabb6c74 100644
> > --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> > +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> > @@ -1039,10 +1039,13 @@ FileHandleReadLine(
> >    // if we ran out of space tell when...
> >
> >    //
> >
> >    if ((CountSoFar+1-CrCount)*sizeof(CHAR16) > *Size){
> >
> > +    UINTN OldSize;
> >
> > +
> >
> > +    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 (#60736):
> > https://edk2.groups.io/g/devel/message/60736
> > Mute This Topic: https://groups.io/mt/74683735/1779286
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [zhiguang.liu@intel.com]
> > -=-=-=-=-=-=