[edk2] [PATCH] IntelFrameworkPkg FrameworkUefiLib: Resolve issue brought by r17740

[Hao Wu]

BufferToReturn = AllocateCopyPool(SizeRequired, String);

The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.

The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
index 9a9503e..c02e653 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,16 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}

- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocateZeroPool(SizeRequired);

if (BufferToReturn == NULL) {
return NULL;
}

+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn), (CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);

ASSERT(StrSize(BufferToReturn)==SizeRequired);

[Jordan Justen]

I think maybe the subject could be improved. I prefer to describe the
problem instead.

IntelFrameworkPkg FrameworkUefiLib: Fix ASSERT in CatVSPrint

Then, I think you can mention the r17740 regression in the main body
of the commit message.

I also have a suggestion below.

Post by Hao Wu
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
index 9a9503e..c02e653 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,16 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocateZeroPool(SizeRequired);

What about something like this instead?

if (String != NULL) {
BufferToReturn = AllocateCopyPool(SizeRequired, String);
} else {
BufferToReturn = AllocatePool(SizeRequired);
BufferToReturn[0] = L'\0';
}

This prevents zero'ing the buffer and then copying over it, and if
String is NULL, it only null terminates the buffer rather than
zero'ing it entirely.

-Jordan

Post by Hao Wu
if (BufferToReturn == NULL) {
return NULL;
}
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn), (CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);
ASSERT(StrSize(BufferToReturn)==SizeRequired);
--
1.9.5.msysgit.0
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Wu, Hao A]

-----Original Message-----
From: Justen, Jordan L
Sent: Wednesday, July 08, 2015 11:14 AM
Subject: Re: [edk2] [PATCH] IntelFrameworkPkg FrameworkUefiLib: Resolve
issue brought by r17740
I think maybe the subject could be improved. I prefer to describe the
problem instead.
IntelFrameworkPkg FrameworkUefiLib: Fix ASSERT in CatVSPrint
Then, I think you can mention the r17740 regression in the main body
of the commit message.

OK, I will modify the subject.

I also have a suggestion below.

Post by Hao Wu
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c

b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c

Post by Hao Wu
index 9a9503e..c02e653 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,16 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocateZeroPool(SizeRequired);

What about something like this instead?
if (String != NULL) {
BufferToReturn = AllocateCopyPool(SizeRequired, String);

'SizeRequired' is longer than the size of 'String', so AllocateCopyPool
will read contents out of the scope of 'String'. It's a potential risk if
'String' is allocated at the memory boundary.

So I think a StrCpyS is still needed here.

} else {
BufferToReturn = AllocatePool(SizeRequired);
BufferToReturn[0] = L'\0';
}
This prevents zero'ing the buffer and then copying over it, and if
String is NULL, it only null terminates the buffer rather than
zero'ing it entirely.
-Jordan

Post by Hao Wu
if (BufferToReturn == NULL) {
return NULL;
}
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn),

(CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);

Post by Hao Wu
ASSERT(StrSize(BufferToReturn)==SizeRequired);
--
1.9.5.msysgit.0
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel