public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] MdePkg/SafeString: Fix potential out-of-bound memory access
@ 2018-02-05  5:26 Ruiyu Ni
  2018-02-05  8:42 ` Yao, Jiewen
  0 siblings, 1 reply; 2+ messages in thread
From: Ruiyu Ni @ 2018-02-05  5:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Jiewen Yao, Liming Gao, Jian J Wang

Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS calls
StrnLenS () to get the length of source string but supplies the
destination buffer size as max size.
It's a bug that may cause out-of-bound memory access.
For example:
  StrnCpyS (Dest[10], 10, "hello", 6)
  -> StrnLenS ("hello", 10) //< cause out-of bound memory access

In a pool guard enabled environment, when using shell to edit an
existing file which contains empty line, the page fault is met.

The patch fixes the four library functions to avoid such
out-of-bound memory access.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
---
 MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
index 68c33e9b7b..29310889ca 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -1,7 +1,7 @@
 /** @file
   Safe String functions.
 
-  Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at
@@ -342,7 +342,7 @@ StrnCpyS (
   //
   // 4. If Length is not less than DestMax, then DestMax shall be greater than StrnLenS(Source, DestMax).
   //
-  SourceLen = StrnLenS (Source, DestMax);
+  SourceLen = StrnLenS (Source, MIN (DestMax, Length));
   if (Length >= DestMax) {
     SAFE_STRING_CONSTRAINT_CHECK ((DestMax > SourceLen), RETURN_BUFFER_TOO_SMALL);
   }
@@ -361,7 +361,7 @@ StrnCpyS (
   // pointed to by Destination. If no null character was copied from Source, then Destination[Length] is set to a null
   // character.
   //
-  while ((*Source != 0) && (SourceLen > 0)) {
+  while ((SourceLen > 0) && (*Source != 0)) {
     *(Destination++) = *(Source++);
     SourceLen--;
   }
@@ -551,7 +551,7 @@ StrnCatS (
   //
   // 5. If Length is not less than CopyLen, then CopyLen shall be greater than StrnLenS(Source, CopyLen).
   //
-  SourceLen = StrnLenS (Source, CopyLen);
+  SourceLen = StrnLenS (Source, MIN (CopyLen, Length));
   if (Length >= CopyLen) {
     SAFE_STRING_CONSTRAINT_CHECK ((CopyLen > SourceLen), RETURN_BUFFER_TOO_SMALL);
   }
@@ -572,7 +572,7 @@ StrnCatS (
   // a null character.
   //
   Destination = Destination + DestLen;
-  while ((*Source != 0) && (SourceLen > 0)) {
+  while ((SourceLen > 0) && (*Source != 0)) {
     *(Destination++) = *(Source++);
     SourceLen--;
   }
@@ -1916,7 +1916,7 @@ AsciiStrnCpyS (
   //
   // 4. If Length is not less than DestMax, then DestMax shall be greater than AsciiStrnLenS(Source, DestMax).
   //
-  SourceLen = AsciiStrnLenS (Source, DestMax);
+  SourceLen = AsciiStrnLenS (Source, MIN (DestMax, Length));
   if (Length >= DestMax) {
     SAFE_STRING_CONSTRAINT_CHECK ((DestMax > SourceLen), RETURN_BUFFER_TOO_SMALL);
   }
@@ -1935,7 +1935,7 @@ AsciiStrnCpyS (
   // pointed to by Destination. If no null character was copied from Source, then Destination[Length] is set to a null
   // character.
   //
-  while ((*Source != 0) && (SourceLen > 0)) {
+  while ((SourceLen > 0) && (*Source != 0)) {
     *(Destination++) = *(Source++);
     SourceLen--;
   }
@@ -2115,7 +2115,7 @@ AsciiStrnCatS (
   //
   // 5. If Length is not less than CopyLen, then CopyLen shall be greater than AsciiStrnLenS(Source, CopyLen).
   //
-  SourceLen = AsciiStrnLenS (Source, CopyLen);
+  SourceLen = AsciiStrnLenS (Source, MIN (CopyLen, Length));
   if (Length >= CopyLen) {
     SAFE_STRING_CONSTRAINT_CHECK ((CopyLen > SourceLen), RETURN_BUFFER_TOO_SMALL);
   }
@@ -2136,7 +2136,7 @@ AsciiStrnCatS (
   // a null character.
   //
   Destination = Destination + DestLen;
-  while ((*Source != 0) && (SourceLen > 0)) {
+  while ((SourceLen > 0) && (*Source != 0)) {
     *(Destination++) = *(Source++);
     SourceLen--;
   }
-- 
2.16.1.windows.1



^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] MdePkg/SafeString: Fix potential out-of-bound memory access
  2018-02-05  5:26 [PATCH v2] MdePkg/SafeString: Fix potential out-of-bound memory access Ruiyu Ni
@ 2018-02-05  8:42 ` Yao, Jiewen
  0 siblings, 0 replies; 2+ messages in thread
From: Yao, Jiewen @ 2018-02-05  8:42 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Wang, Jian J

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, February 5, 2018 1:26 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>
> Subject: [PATCH v2] MdePkg/SafeString: Fix potential out-of-bound memory
> access
> 
> Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS calls
> StrnLenS () to get the length of source string but supplies the
> destination buffer size as max size.
> It's a bug that may cause out-of-bound memory access.
> For example:
>   StrnCpyS (Dest[10], 10, "hello", 6)
>   -> StrnLenS ("hello", 10) //< cause out-of bound memory access
> 
> In a pool guard enabled environment, when using shell to edit an
> existing file which contains empty line, the page fault is met.
> 
> The patch fixes the four library functions to avoid such
> out-of-bound memory access.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/SafeString.c
> b/MdePkg/Library/BaseLib/SafeString.c
> index 68c33e9b7b..29310889ca 100644
> --- a/MdePkg/Library/BaseLib/SafeString.c
> +++ b/MdePkg/Library/BaseLib/SafeString.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Safe String functions.
> 
> -  Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -342,7 +342,7 @@ StrnCpyS (
>    //
>    // 4. If Length is not less than DestMax, then DestMax shall be greater than
> StrnLenS(Source, DestMax).
>    //
> -  SourceLen = StrnLenS (Source, DestMax);
> +  SourceLen = StrnLenS (Source, MIN (DestMax, Length));
>    if (Length >= DestMax) {
>      SAFE_STRING_CONSTRAINT_CHECK ((DestMax > SourceLen),
> RETURN_BUFFER_TOO_SMALL);
>    }
> @@ -361,7 +361,7 @@ StrnCpyS (
>    // pointed to by Destination. If no null character was copied from Source,
> then Destination[Length] is set to a null
>    // character.
>    //
> -  while ((*Source != 0) && (SourceLen > 0)) {
> +  while ((SourceLen > 0) && (*Source != 0)) {
>      *(Destination++) = *(Source++);
>      SourceLen--;
>    }
> @@ -551,7 +551,7 @@ StrnCatS (
>    //
>    // 5. If Length is not less than CopyLen, then CopyLen shall be greater than
> StrnLenS(Source, CopyLen).
>    //
> -  SourceLen = StrnLenS (Source, CopyLen);
> +  SourceLen = StrnLenS (Source, MIN (CopyLen, Length));
>    if (Length >= CopyLen) {
>      SAFE_STRING_CONSTRAINT_CHECK ((CopyLen > SourceLen),
> RETURN_BUFFER_TOO_SMALL);
>    }
> @@ -572,7 +572,7 @@ StrnCatS (
>    // a null character.
>    //
>    Destination = Destination + DestLen;
> -  while ((*Source != 0) && (SourceLen > 0)) {
> +  while ((SourceLen > 0) && (*Source != 0)) {
>      *(Destination++) = *(Source++);
>      SourceLen--;
>    }
> @@ -1916,7 +1916,7 @@ AsciiStrnCpyS (
>    //
>    // 4. If Length is not less than DestMax, then DestMax shall be greater than
> AsciiStrnLenS(Source, DestMax).
>    //
> -  SourceLen = AsciiStrnLenS (Source, DestMax);
> +  SourceLen = AsciiStrnLenS (Source, MIN (DestMax, Length));
>    if (Length >= DestMax) {
>      SAFE_STRING_CONSTRAINT_CHECK ((DestMax > SourceLen),
> RETURN_BUFFER_TOO_SMALL);
>    }
> @@ -1935,7 +1935,7 @@ AsciiStrnCpyS (
>    // pointed to by Destination. If no null character was copied from Source,
> then Destination[Length] is set to a null
>    // character.
>    //
> -  while ((*Source != 0) && (SourceLen > 0)) {
> +  while ((SourceLen > 0) && (*Source != 0)) {
>      *(Destination++) = *(Source++);
>      SourceLen--;
>    }
> @@ -2115,7 +2115,7 @@ AsciiStrnCatS (
>    //
>    // 5. If Length is not less than CopyLen, then CopyLen shall be greater than
> AsciiStrnLenS(Source, CopyLen).
>    //
> -  SourceLen = AsciiStrnLenS (Source, CopyLen);
> +  SourceLen = AsciiStrnLenS (Source, MIN (CopyLen, Length));
>    if (Length >= CopyLen) {
>      SAFE_STRING_CONSTRAINT_CHECK ((CopyLen > SourceLen),
> RETURN_BUFFER_TOO_SMALL);
>    }
> @@ -2136,7 +2136,7 @@ AsciiStrnCatS (
>    // a null character.
>    //
>    Destination = Destination + DestLen;
> -  while ((*Source != 0) && (SourceLen > 0)) {
> +  while ((SourceLen > 0) && (*Source != 0)) {
>      *(Destination++) = *(Source++);
>      SourceLen--;
>    }
> --
> 2.16.1.windows.1



^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-02-05  8:37 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-05  5:26 [PATCH v2] MdePkg/SafeString: Fix potential out-of-bound memory access Ruiyu Ni
2018-02-05  8:42 ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox