* [PATCH 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string
2017-05-23 1:19 [PATCH 0/2] Avoid reading undefined contents Hao Wu
@ 2017-05-23 1:19 ` Hao Wu
2017-05-23 1:19 ` [PATCH 2/2] MdeModulePkg/PrintLib: " Hao Wu
1 sibling, 0 replies; 3+ messages in thread
From: Hao Wu @ 2017-05-23 1:19 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Liming Gao, Michael Kinney
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 | 48 ++++++++++++++++----
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index 9b15a07ac0..d665b7b1d2 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -653,7 +653,11 @@ BasePrintLibSPrintMarker (
//
// Get the first character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
//
// Loop until the end of the format string is reached or the output buffer is full
@@ -685,7 +689,11 @@ BasePrintLibSPrintMarker (
//
for (Done = FALSE; !Done; ) {
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
switch (FormatCharacter) {
case '.':
Flags |= PRECISION;
@@ -738,7 +746,11 @@ BasePrintLibSPrintMarker (
for (Count = 0; ((FormatCharacter >= '0') && (FormatCharacter <= '9')); ){
Count = (Count * 10) + FormatCharacter - '0';
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
}
Format -= BytesPerFormatCharacter;
if ((Flags & PRECISION) == 0) {
@@ -1017,7 +1029,11 @@ BasePrintLibSPrintMarker (
case '\r':
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter == '\n') {
//
// Translate '\r\n' to '\r\n'
@@ -1038,7 +1054,11 @@ BasePrintLibSPrintMarker (
//
ArgumentString = "\r\n";
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter != '\r') {
Format -= BytesPerFormatCharacter;
}
@@ -1057,7 +1077,11 @@ BasePrintLibSPrintMarker (
case '\r':
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter == '\n') {
//
// Translate '\r\n' to '\r\n'
@@ -1078,7 +1102,11 @@ BasePrintLibSPrintMarker (
//
ArgumentString = "\r\n";
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter != '\r') {
Format -= BytesPerFormatCharacter;
}
@@ -1206,7 +1234,11 @@ BasePrintLibSPrintMarker (
//
// Get the next character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
}
if ((Flags & COUNT_ONLY_NO_PRINT) != 0) {
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] MdeModulePkg/PrintLib: Avoid reading content beyond the format string
2017-05-23 1:19 [PATCH 0/2] Avoid reading undefined contents Hao Wu
2017-05-23 1:19 ` [PATCH 1/2] MdePkg/BasePrintLib: Avoid reading content beyond the format string Hao Wu
@ 2017-05-23 1:19 ` Hao Wu
1 sibling, 0 replies; 3+ messages in thread
From: Hao Wu @ 2017-05-23 1:19 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Liming Gao, Michael Kinney
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 | 66 ++++++++++++++++----
1 file changed, 55 insertions(+), 11 deletions(-)
diff --git a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
index 9f702c4fef..342eee42fc 100644
--- a/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
+++ b/MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
@@ -130,7 +130,11 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
//
// Get the first character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
while (FormatCharacter != 0) {
if (FormatCharacter == '%') {
@@ -148,7 +152,11 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
//
// Get the next character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
switch (FormatCharacter) {
case '.':
@@ -239,7 +247,11 @@ DxePrintLibPrint2ProtocolVaListToBaseList (
//
// Get the next character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
}
return TRUE;
}
@@ -1596,7 +1608,11 @@ InternalPrintLibSPrintMarker (
//
// Get the first character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
//
// Loop until the end of the format string is reached or the output buffer is full
@@ -1628,7 +1644,11 @@ InternalPrintLibSPrintMarker (
//
for (Done = FALSE; !Done; ) {
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
switch (FormatCharacter) {
case '.':
Flags |= PRECISION;
@@ -1681,7 +1701,11 @@ InternalPrintLibSPrintMarker (
for (Count = 0; ((FormatCharacter >= '0') && (FormatCharacter <= '9')); ){
Count = (Count * 10) + FormatCharacter - '0';
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
}
Format -= BytesPerFormatCharacter;
if ((Flags & PRECISION) == 0) {
@@ -1960,7 +1984,11 @@ InternalPrintLibSPrintMarker (
case '\r':
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter == '\n') {
//
// Translate '\r\n' to '\r\n'
@@ -1981,7 +2009,11 @@ InternalPrintLibSPrintMarker (
//
ArgumentString = "\r\n";
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter != '\r') {
Format -= BytesPerFormatCharacter;
}
@@ -2000,7 +2032,11 @@ InternalPrintLibSPrintMarker (
case '\r':
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter == '\n') {
//
// Translate '\r\n' to '\r\n'
@@ -2021,7 +2057,11 @@ InternalPrintLibSPrintMarker (
//
ArgumentString = "\r\n";
Format += BytesPerFormatCharacter;
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
if (FormatCharacter != '\r') {
Format -= BytesPerFormatCharacter;
}
@@ -2149,7 +2189,11 @@ InternalPrintLibSPrintMarker (
//
// Get the next character from the format string
//
- FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ if (BytesPerFormatCharacter == 1) {
+ FormatCharacter = (*Format & 0xff) & FormatMask;
+ } else {
+ FormatCharacter = ((*Format & 0xff) | (*(Format + 1) << 8)) & FormatMask;
+ }
}
if ((Flags & COUNT_ONLY_NO_PRINT) != 0) {
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread