[PATCH v2 0/2] Avoid reading undefined contents in PrintLib

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v2 0/2] Avoid reading undefined contents in PrintLib
@ 2017-05-31  6:13 Hao Wu
  2017-05-31  6:13 ` [PATCH v2 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string Hao Wu
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Hao Wu @ 2017-05-31  6:13 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Liming Gao, Michael Kinney

V2 changes:
Refines the code logic to replace the 'if else' statments with the '? :'
operator.

V1 history:
This series add logic to avoid reading content beyond a ASCII format
string.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>

Hao Wu (2):
  MdePkg/BasePrintLib: Avoid reading content beyond the format string
  MdeModulePkg/PrintLib: Avoid reading content beyond the format string

 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c | 22 ++++++++++----------
 MdePkg/Library/BasePrintLib/PrintLibInternal.c            | 16 +++++++-------
 2 files changed, 19 insertions(+), 19 deletions(-)

-- 
2.12.0.windows.1



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

* [PATCH v2 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string
  2017-05-31  6:13 [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Hao Wu
@ 2017-05-31  6:13 ` Hao Wu
  2017-05-31  6:13 ` [PATCH v2 2/2] MdeModulePkg/PrintLib: " Hao Wu
  2017-05-31  7:05 ` [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Gao, Liming
  2 siblings, 0 replies; 4+ messages in thread
From: Hao Wu @ 2017-05-31  6:13 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Liming Gao, Michael Kinney

https://bugzilla.tianocore.org/show_bug.cgi?id=567

In function BasePrintLibSPrintMarker(), when processing ASCII format
strings, if the format string walker pointer 'Format' is pointing at the
end of the format string (i.e. '\0'), the following expression:
*(Format + 1)
will read an undefined value.

Though this value won't affect the functionality, since it will be masked
by variable 'FormatMask':
(*(Format + 1) << 8)) & FormatMask
(FormatMask is 0xff for ASCII format string)

This commit adds additional logic to avoid reading undefined content.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index 9b15a07ac0..cec5b3bc99 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -653,7 +653,7 @@ BasePrintLibSPrintMarker (
   //
   // Get the first character from the format string
   //
-  FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+  FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
 
   //
   // Loop until the end of the format string is reached or the output buffer is full
@@ -685,7 +685,7 @@ BasePrintLibSPrintMarker (
       //
       for (Done = FALSE; !Done; ) {
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         switch (FormatCharacter) {
         case '.': 
           Flags |= PRECISION; 
@@ -738,7 +738,7 @@ BasePrintLibSPrintMarker (
           for (Count = 0; ((FormatCharacter >= '0') &&  (FormatCharacter <= '9')); ){
             Count = (Count * 10) + FormatCharacter - '0';
             Format += BytesPerFormatCharacter;
-            FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+            FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
           }
           Format -= BytesPerFormatCharacter;
           if ((Flags & PRECISION) == 0) {
@@ -1017,7 +1017,7 @@ BasePrintLibSPrintMarker (
 
       case '\r':
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         if (FormatCharacter == '\n') {
           //
           // Translate '\r\n' to '\r\n'
@@ -1038,7 +1038,7 @@ BasePrintLibSPrintMarker (
         //
         ArgumentString = "\r\n";
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         if (FormatCharacter != '\r') {
           Format   -= BytesPerFormatCharacter;
         }
@@ -1057,7 +1057,7 @@ BasePrintLibSPrintMarker (
  
     case '\r':
       Format += BytesPerFormatCharacter;
-      FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+      FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
       if (FormatCharacter == '\n') {
         //
         // Translate '\r\n' to '\r\n'
@@ -1078,7 +1078,7 @@ BasePrintLibSPrintMarker (
       //
       ArgumentString = "\r\n";
       Format += BytesPerFormatCharacter;
-      FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+      FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
       if (FormatCharacter != '\r') {
         Format   -= BytesPerFormatCharacter;
       }
@@ -1206,7 +1206,7 @@ BasePrintLibSPrintMarker (
     //
     // Get the next character from the format string
     //
-    FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+    FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
   }
 
   if ((Flags & COUNT_ONLY_NO_PRINT) != 0) {
-- 
2.12.0.windows.1



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

* [PATCH v2 2/2] MdeModulePkg/PrintLib: Avoid reading content beyond the format string
  2017-05-31  6:13 [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Hao Wu
  2017-05-31  6:13 ` [PATCH v2 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string Hao Wu
@ 2017-05-31  6:13 ` Hao Wu
  2017-05-31  7:05 ` [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Gao, Liming
  2 siblings, 0 replies; 4+ messages in thread
From: Hao Wu @ 2017-05-31  6:13 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Liming Gao, Michael Kinney

https://bugzilla.tianocore.org/show_bug.cgi?id=567

In functions DxePrintLibPrint2ProtocolVaListToBaseList() and
InternalPrintLibSPrintMarker(), when processing ASCII format strings, if
the format string walker pointer 'Format' is pointing at the end of the
format string (i.e. '\0'), the following expression:
*(Format + 1)
will read an undefined value.

Though this value won't affect the functionality, since it will be masked
by variable 'FormatMask':
(*(Format + 1) << 8)) & FormatMask
(FormatMask is 0xff for ASCII format string)

This commit adds additional logic to avoid reading undefined content.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
index 9f702c4fef..b58db8e011 100644
--- a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
+++ b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
@@ -130,7 +130,7 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
   //
   // Get the first character from the format string
   //
-  FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+  FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
 
   while (FormatCharacter != 0) {
     if (FormatCharacter == '%') {
@@ -148,7 +148,7 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
         //
         // Get the next character from the format string
         //
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
 
         switch (FormatCharacter) {
         case '.': 
@@ -239,7 +239,7 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
     //
     // Get the next character from the format string
     //
-    FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+    FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
   }
   return TRUE;
 }
@@ -1596,7 +1596,7 @@ InternalPrintLibSPrintMarker (
   //
   // Get the first character from the format string
   //
-  FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+  FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
 
   //
   // Loop until the end of the format string is reached or the output buffer is full
@@ -1628,7 +1628,7 @@ InternalPrintLibSPrintMarker (
       //
       for (Done = FALSE; !Done; ) {
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         switch (FormatCharacter) {
         case '.': 
           Flags |= PRECISION; 
@@ -1681,7 +1681,7 @@ InternalPrintLibSPrintMarker (
           for (Count = 0; ((FormatCharacter >= '0') &&  (FormatCharacter <= '9')); ){
             Count = (Count * 10) + FormatCharacter - '0';
             Format += BytesPerFormatCharacter;
-            FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+            FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
           }
           Format -= BytesPerFormatCharacter;
           if ((Flags & PRECISION) == 0) {
@@ -1960,7 +1960,7 @@ InternalPrintLibSPrintMarker (
 
       case '\r':
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         if (FormatCharacter == '\n') {
           //
           // Translate '\r\n' to '\r\n'
@@ -1981,7 +1981,7 @@ InternalPrintLibSPrintMarker (
         //
         ArgumentString = "\r\n";
         Format += BytesPerFormatCharacter;
-        FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+        FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
         if (FormatCharacter != '\r') {
           Format   -= BytesPerFormatCharacter;
         }
@@ -2000,7 +2000,7 @@ InternalPrintLibSPrintMarker (
  
     case '\r':
       Format += BytesPerFormatCharacter;
-      FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+      FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
       if (FormatCharacter == '\n') {
         //
         // Translate '\r\n' to '\r\n'
@@ -2021,7 +2021,7 @@ InternalPrintLibSPrintMarker (
       //
       ArgumentString = "\r\n";
       Format += BytesPerFormatCharacter;
-      FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+      FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
       if (FormatCharacter != '\r') {
         Format   -= BytesPerFormatCharacter;
       }
@@ -2149,7 +2149,7 @@ InternalPrintLibSPrintMarker (
     //
     // Get the next character from the format string
     //
-    FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+    FormatCharacter = ((*Format & 0xff) | ((BytesPerFormatCharacter == 1) ? 0 : (*(Format + 1) << 8))) & FormatMask;
   }
 
   if ((Flags & COUNT_ONLY_NO_PRINT) != 0) {
-- 
2.12.0.windows.1



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

* Re: [PATCH v2 0/2] Avoid reading undefined contents in PrintLib
  2017-05-31  6:13 [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Hao Wu
  2017-05-31  6:13 ` [PATCH v2 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string Hao Wu
  2017-05-31  6:13 ` [PATCH v2 2/2] MdeModulePkg/PrintLib: " Hao Wu
@ 2017-05-31  7:05 ` Gao, Liming
  2 siblings, 0 replies; 4+ messages in thread
From: Gao, Liming @ 2017-05-31  7:05 UTC (permalink / raw)
  To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Kinney, Michael D

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, May 31, 2017 2:13 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH v2 0/2] Avoid reading undefined contents in PrintLib
> 
> V2 changes:
> Refines the code logic to replace the 'if else' statments with the '? :'
> operator.
> 
> V1 history:
> This series add logic to avoid reading content beyond a ASCII format
> string.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> 
> Hao Wu (2):
>   MdePkg/BasePrintLib: Avoid reading content beyond the format string
>   MdeModulePkg/PrintLib: Avoid reading content beyond the format string
> 
>  MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c | 22 ++++++++++----------
>  MdePkg/Library/BasePrintLib/PrintLibInternal.c            | 16 +++++++-------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> --
> 2.12.0.windows.1



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

end of thread, other threads:[~2017-05-31  7:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-31  6:13 [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Hao Wu
2017-05-31  6:13 ` [PATCH v2 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string Hao Wu
2017-05-31  6:13 ` [PATCH v2 2/2] MdeModulePkg/PrintLib: " Hao Wu
2017-05-31  7:05 ` [PATCH v2 0/2] Avoid reading undefined contents in PrintLib Gao, Liming

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