[edk2] [PATCH 0/2] Fix some bugs of replacing unsafe string functions

[Heyi Guo]

I found EFI Shell triggers ASSERT on QEMU AARCH64 when I run "cd"
command and then "ls" command, with EDK2 DEBUG version.

These are 2 bugs I found which were introduced by unsafe string
functions replacement.

We may need more review on these commits, especially for using
Str***S to replace Strn***, because they functions differently when
length overflows.

Thanks.

Heyi Guo (2):
MdePkg: Fix bug in CatVSPrint introduced by r17742
ShellPkg: Fix bug introduced by r17730

MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
ShellPkg/Application/Shell/ShellProtocol.c | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)

[Heyi Guo]

SVN r17742 uses AllocateCopyPool to replace AllocateZeroPool, however
String can be NULL and this will trigger assert in AllocateCopyPool.
Error Can be replayed when we use "cd <dir>" command under Shell.

Just use a more conservative way to replace unsafe StrCpy.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/UefiLib/UefiLibPrint.c b/MdePkg/Library/UefiLib/UefiLibPrint.c
index cc41eb0..604c25b 100644
--- a/MdePkg/Library/UefiLib/UefiLibPrint.c
+++ b/MdePkg/Library/UefiLib/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);

[Ard Biesheuvel]

Post by Heyi Guo
SVN r17742 uses AllocateCopyPool to replace AllocateZeroPool, however
String can be NULL and this will trigger assert in AllocateCopyPool.
Error Can be replayed when we use "cd <dir>" command under Shell.
Just use a more conservative way to replace unsafe StrCpy.
Contributed-under: TianoCore Contribution Agreement 1.0
---
MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/UefiLib/UefiLibPrint.c b/MdePkg/Library/UefiLib/UefiLibPrint.c
index cc41eb0..604c25b 100644
--- a/MdePkg/Library/UefiLib/UefiLibPrint.c
+++ b/MdePkg/Library/UefiLib/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);
+ }
+

How about:

if (String != NULL) {
BufferToReturn = AllocateCopyPool(SizeRequired, String);
} else {
BufferToReturn = AllocateZeroPool(SizeRequired);
}

instead?

[Heyi Guo]

BufferToReturn = AllocateCopyPool(SizeRequired, String);

It will touch the address out of the scope of String. Though it is only
read operation, I think we'd better not touch it, once the String were
allocated at the boundary of memory region. Also the patch reverts part
of the changes in r17742 and only replace StrCpy with StrCpyS.

Post by Heyi Guo

Post by Heyi Guo
SVN r17742 uses AllocateCopyPool to replace AllocateZeroPool, however
String can be NULL and this will trigger assert in AllocateCopyPool.
Error Can be replayed when we use "cd <dir>" command under Shell.
Just use a more conservative way to replace unsafe StrCpy.
Contributed-under: TianoCore Contribution Agreement 1.0
---
MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/UefiLib/UefiLibPrint.c b/MdePkg/Library/UefiLib/UefiLibPrint.c
index cc41eb0..604c25b 100644
--- a/MdePkg/Library/UefiLib/UefiLibPrint.c
+++ b/MdePkg/Library/UefiLib/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);
+ }
+

if (String != NULL) {
BufferToReturn = AllocateCopyPool(SizeRequired, String);
} else {
BufferToReturn = AllocateZeroPool(SizeRequired);
}
instead?

[Gao, Liming]

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

-----Original Message-----
From: Heyi Guo [mailto:***@linaro.org]
Sent: Friday, July 03, 2015 5:35 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [PATCH 1/2] MdePkg: Fix bug in CatVSPrint introduced by r17742

SVN r17742 uses AllocateCopyPool to replace AllocateZeroPool, however String can be NULL and this will trigger assert in AllocateCopyPool.
Error Can be replayed when we use "cd <dir>" command under Shell.

Just use a more conservative way to replace unsafe StrCpy.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/UefiLib/UefiLibPrint.c b/MdePkg/Library/UefiLib/UefiLibPrint.c
index cc41eb0..604c25b 100644
--- a/MdePkg/Library/UefiLib/UefiLibPrint.c
+++ b/MdePkg/Library/UefiLib/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);
--
2.1.4


------------------------------------------------------------------------------
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
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Heyi Guo]

CurrentFilePattern is only part of FilePattern and will be less than
or equal to FilePattern. If we use StrCpyS to replace StrnCpy, it
will cause assert when FilePattern is longer.

The bug can be replayed when we cd to one directory and run ls
command.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
ShellPkg/Application/Shell/ShellProtocol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
index 249e1e1..6a24852 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -2220,7 +2220,7 @@ ShellSearchHandle(

CurrentFilePattern = AllocateZeroPool((NextFilePatternStart-FilePattern+1)*sizeof(CHAR16));
ASSERT(CurrentFilePattern != NULL);
- StrCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1, FilePattern);
+ StrnCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1, FilePattern, NextFilePatternStart-FilePattern);

if (CurrentFilePattern[0] == CHAR_NULL
&&NextFilePatternStart[0] == CHAR_NULL

[Ard Biesheuvel]

Post by Heyi Guo
CurrentFilePattern is only part of FilePattern and will be less than
or equal to FilePattern. If we use StrCpyS to replace StrnCpy, it
will cause assert when FilePattern is longer.
The bug can be replayed when we cd to one directory and run ls
command.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ShellPkg/Application/Shell/ShellProtocol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
index 249e1e1..6a24852 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -2220,7 +2220,7 @@ ShellSearchHandle(
CurrentFilePattern = AllocateZeroPool((NextFilePatternStart-FilePattern+1)*sizeof(CHAR16));
ASSERT(CurrentFilePattern != NULL);
- StrCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1, FilePattern);
+ StrnCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1, FilePattern, NextFilePatternStart-FilePattern);
if (CurrentFilePattern[0] == CHAR_NULL
&&NextFilePatternStart[0] == CHAR_NULL
--
2.1.4

[Qiu, Shumin]

Reviewed-by: Qiu Shumin <***@intel.com>

-----Original Message-----
From: Heyi Guo [mailto:***@linaro.org]
Sent: Friday, July 03, 2015 5:35 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [PATCH 2/2] ShellPkg: Fix bug introduced by r17730

CurrentFilePattern is only part of FilePattern and will be less than or equal to FilePattern. If we use StrCpyS to replace StrnCpy, it will cause assert when FilePattern is longer.

The bug can be replayed when we cd to one directory and run ls command.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
ShellPkg/Application/Shell/ShellProtocol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
index 249e1e1..6a24852 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -2220,7 +2220,7 @@ ShellSearchHandle(

CurrentFilePattern = AllocateZeroPool((NextFilePatternStart-FilePattern+1)*sizeof(CHAR16));
ASSERT(CurrentFilePattern != NULL);
- StrCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1, FilePattern);
+ StrnCpyS(CurrentFilePattern, NextFilePatternStart-FilePattern+1,
+ FilePattern, NextFilePatternStart-FilePattern);

if (CurrentFilePattern[0] == CHAR_NULL
&&NextFilePatternStart[0] == CHAR_NULL
--
2.1.4


------------------------------------------------------------------------------
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
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Qiu, Shumin]

Reviewed-by: Qiu Shumin <***@intel.com>

-----Original Message-----
From: Heyi Guo [mailto:***@linaro.org]
Sent: Friday, July 03, 2015 5:35 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [PATCH 1/2] MdePkg: Fix bug in CatVSPrint introduced by r17742

SVN r17742 uses AllocateCopyPool to replace AllocateZeroPool, however String can be NULL and this will trigger assert in AllocateCopyPool.
Error Can be replayed when we use "cd <dir>" command under Shell.

Just use a more conservative way to replace unsafe StrCpy.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
MdePkg/Library/UefiLib/UefiLibPrint.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/UefiLib/UefiLibPrint.c b/MdePkg/Library/UefiLib/UefiLibPrint.c
index cc41eb0..604c25b 100644
--- a/MdePkg/Library/UefiLib/UefiLibPrint.c
+++ b/MdePkg/Library/UefiLib/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);
--
2.1.4


------------------------------------------------------------------------------
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
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel