[PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions

[PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions

[Ard Biesheuvel]

This series prepares EmbeddedPkg for building with the preprocess symbol
DISABLE_NEW_DEPRECATED_INTERFACES defined, by adding missing components
to EmbeddedPkg (#3), fixing broken code or code that relies on deprecated
functionality (#1 - #2, #4 - #8), and finally adds
-DDISABLE_NEW_DEPRECATED_INTERFACES to the CC flags for all build types,
toolchains and architectures.

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

Branch can be found here (including ArmVirtPkg and ArmPkg)
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/armpkg-no-deprecated-interfaces

Ard Biesheuvel (9):
  EmbeddedPkg/AndroidFastbootTransportTcpDxe: remove broken hostname
    handling
  EmbeddedPkg: remove unused PrePiHobListPointerLib
  EmbeddedPkg: add missing modules
  EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size
  EmbeddedPkg/AndroidFastboot: eliminate deprecated string function
    calls
  EmbeddedPkg/Ebl: eliminate deprecated string function calls
  EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls
  EmbeddedPkg/MmcDxe: eliminate deprecated string function calls
  EmbeddedPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES

 EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c                       |  3 +-
 EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c                   |  4 +-
 EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c      | 23 -------
 EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf |  3 -
 EmbeddedPkg/Ebl/Command.c                                                      |  2 +-
 EmbeddedPkg/Ebl/Dir.c                                                          |  4 +-
 EmbeddedPkg/Ebl/EfiDevice.c                                                    | 11 ++--
 EmbeddedPkg/Ebl/Main.c                                                         |  8 +--
 EmbeddedPkg/Ebl/Variable.c                                                     | 17 +++--
 EmbeddedPkg/EmbeddedPkg.dec                                                    |  1 -
 EmbeddedPkg/EmbeddedPkg.dsc                                                    | 13 +++-
 EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c                                    | 42 ++++++------
 EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c                              |  2 +-
 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c               | 69 --------------------
 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf          | 38 -----------
 EmbeddedPkg/Universal/MmcDxe/Diagnostics.c                                     |  2 +-
 Omap35xxPkg/Omap35xxPkg.dsc                                                    |  1 -
 17 files changed, 64 insertions(+), 179 deletions(-)
 delete mode 100644 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c
 delete mode 100644 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf

-- 
2.7.4

[PATCH 1/9] EmbeddedPkg/AndroidFastbootTransportTcpDxe: remove broken hostname handling

[Ard Biesheuvel]

The fastboot TCP connection setup routine retrieves a hostname from a
UEFI variable 'hostname' that is scoped under a GUID gEfiHostnameVariableGuid
whose definition is missing from the code. Since the hostname is only printed
and then discarded, let's just drop the whole thing.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c      | 23 --------------------
 EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf |  3 ---
 2 files changed, 26 deletions(-)

diff --git a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
index 8ec78aeb534e..7c008ac9722e 100644
--- a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
+++ b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
@@ -27,8 +27,6 @@
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
 
-#include <Guid/Hostname.h>
-
 #define IP4_ADDR_TO_STRING(IpAddr, IpAddrString) UnicodeSPrint (       \
                                                    IpAddrString,       \
                                                    16 * 2,             \
@@ -307,9 +305,6 @@ TcpFastbootTransportStart (
   EFI_HANDLE                   *HandleBuffer;
   EFI_IP4_MODE_DATA             Ip4ModeData;
   UINTN                         NumHandles;
-  UINTN                         HostnameSize = 256;
-  CHAR8                         Hostname[256];
-  CHAR16                        HostnameUnicode[256] = L"<no hostname>";
   CHAR16                        IpAddrString[16];
   UINTN                         Index;
 
@@ -442,28 +437,10 @@ TcpFastbootTransportStart (
   //
   IP4_ADDR_TO_STRING (Ip4ModeData.ConfigData.StationAddress, IpAddrString);
 
-  // Look up hostname
-  Status = gRT->GetVariable (
-                  L"Hostname",
-                  &gEfiHostnameVariableGuid,
-                  NULL,
-                  &HostnameSize,
-                  &Hostname
-                  );
-  if (!EFI_ERROR (Status) && HostnameSize != 0) {
-    AsciiStrToUnicodeStr (Hostname, HostnameUnicode);
-  }
-
-  // Hostname variable is not null-terminated.
-  Hostname[HostnameSize] = L'\0';
-
   mTextOut->OutputString (mTextOut, L"TCP Fastboot transport configured.");
   mTextOut->OutputString (mTextOut, L"\r\nIP address: ");
   mTextOut->OutputString (mTextOut ,IpAddrString);
   mTextOut->OutputString (mTextOut, L"\r\n");
-  mTextOut->OutputString (mTextOut, L"\r\nhostname: ");
-  mTextOut->OutputString (mTextOut, HostnameUnicode);
-  mTextOut->OutputString (mTextOut, L"\r\n");
 
   //
   // Start listening for a connection
diff --git a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
index 4d777934e5a4..89ff556b40ca 100644
--- a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
+++ b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
@@ -47,8 +47,5 @@ [Packages]
   MdeModulePkg/MdeModulePkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
 
-[Guids]
-  gEfiHostnameVariableGuid
-
 [FixedPcd]
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootTcpPort
-- 
2.7.4

[PATCH 2/9] EmbeddedPkg: remove unused PrePiHobListPointerLib

[Ard Biesheuvel]

Remove this unused version: all existing platforms use the one under
ArmPlatformPkg instead.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dec                                           |  1 -
 EmbeddedPkg/EmbeddedPkg.dsc                                           |  1 -
 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c      | 69 --------------------
 EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf | 38 -----------
 Omap35xxPkg/Omap35xxPkg.dsc                                           |  1 -
 5 files changed, 110 deletions(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 775d863c5f64..2c2cf41103c2 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -97,7 +97,6 @@ [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdEmbeddedMemVariableStoreSize|0x10000|UINT32|0x00000009
   gEmbeddedTokenSpaceGuid.PcdEmbeddedPrompt|"Ebl"|VOID*|0x00000034
 
-  gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|131072|UINT32|0x00000040
   gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0|UINT32|0x0000000b
   gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|131072|UINT32|0x0000000c
 
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index cd1f6df06b24..5224bcefa9c7 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -184,7 +184,6 @@ [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdEmbeddedDefaultTextColor|0x07
   gEmbeddedTokenSpaceGuid.PcdEmbeddedMemVariableStoreSize|0x10000
 
-  gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|0
   gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0
   gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|0
 
diff --git a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c b/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c
deleted file mode 100644
index 9065b1132fda..000000000000
--- a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c
+++ /dev/null
@@ -1,69 +0,0 @@
-/** @file
-*
-*  Copyright (c) 2011, ARM Limited. All rights reserved.
-*
-*  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
-*  http://opensource.org/licenses/bsd-license.php
-*
-*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#include <PiPei.h>
-#include <Library/PrePiHobListPointerLib.h>
-#include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
-
-//
-// Have to use build system to set the original value in case we are running
-// from FLASH and globals don't work. So if you do a GetHobList() and gHobList
-// and gHobList is NULL the PCD default values are used.
-//
-VOID *gHobList = NULL;
-
-
-/**
-  Returns the pointer to the HOB list.
-
-  This function returns the pointer to first HOB in the list.
-
-  @return The pointer to the HOB list.
-
-**/
-VOID *
-EFIAPI
-PrePeiGetHobList (
-  VOID
-  )
-{
-  if (gHobList == NULL) {
-    return (VOID *)*(UINTN*)PcdGet32 (PcdPrePiHobBase);
-  } else {
-    return gHobList;
-  }
-}
-
-
-
-/**
-  Updates the pointer to the HOB list.
-
-  @param  HobList       Hob list pointer to store
-
-**/
-EFI_STATUS
-EFIAPI
-PrePeiSetHobList (
-  IN  VOID      *HobList
-  )
-{
-  gHobList = HobList;
-
-  //
-  // If this code is running from ROM this could fail
-  //
-  return (gHobList == HobList) ? EFI_SUCCESS: EFI_UNSUPPORTED;
-}
diff --git a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf b/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
deleted file mode 100644
index bfd3c209a13b..000000000000
--- a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
+++ /dev/null
@@ -1,38 +0,0 @@
-#/** @file
-#
-#  Copyright (c) 2011, ARM Limited. All rights reserved.
-#
-#  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
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#**/
-
-[Defines]
-  INF_VERSION                    = 0x00010005
-  BASE_NAME                      = PrePiHobListPointerLib
-  FILE_GUID                      = 7a6a7fc0-5ee0-11e0-b47f-0002a5d5c51b
-  MODULE_TYPE                    = BASE
-  VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = PrePiHobListPointerLib
-
-#
-#  VALID_ARCHITECTURES           = IA32 X64 IPF EBC (EBC is for build only)
-#
-
-[Sources]
-  PrePiHobListPointer.c
-
-[Packages]
-  MdePkg/MdePkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
-
-[LibraryClasses]
-  #DebugLib
-
-[FixedPcd.common]
-  gEmbeddedTokenSpaceGuid.PcdPrePiHobBase
diff --git a/Omap35xxPkg/Omap35xxPkg.dsc b/Omap35xxPkg/Omap35xxPkg.dsc
index 11259191e1aa..2b339253ff9c 100644
--- a/Omap35xxPkg/Omap35xxPkg.dsc
+++ b/Omap35xxPkg/Omap35xxPkg.dsc
@@ -145,7 +145,6 @@ [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdPrePiBfvSize|0
   gEmbeddedTokenSpaceGuid.PcdFlashFvMainBase|0
   gEmbeddedTokenSpaceGuid.PcdFlashFvMainSize|0
-  gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|0x80001000
   gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0x87FE0000 # stack at top of memory
   gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|0x20000  # 128K stack
   gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x80000000
-- 
2.7.4

[PATCH 3/9] EmbeddedPkg: add missing modules

[Ard Biesheuvel]

Add modules that live under EmbeddedPkg but were missing from the
[Components] section of the package .dsc file

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dsc | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index 5224bcefa9c7..cc95564f31af 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -280,5 +280,14 @@ [Components.common]
   EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf
   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
 
+  EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
+  EmbeddedPkg/Library/DebugAgentTimerLibNull/DebugAgentTimerLibNull.inf
+  EmbeddedPkg/Library/DxeHobPeCoffLib/DxeHobPeCoffLib.inf
+  EmbeddedPkg/Library/EblNetworkLib/EblNetworkLib.inf
+  EmbeddedPkg/Library/FdtLib/FdtLib.inf
+  EmbeddedPkg/Library/GdbDebugAgent/GdbDebugAgent.inf
+  EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
+  EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
+
 [Components.IA32, Components.X64, Components.IPF, Components.ARM]
   EmbeddedPkg/GdbStub/GdbStub.inf
-- 
2.7.4

[PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size

[Ard Biesheuvel]

The result of PcdGet64() can only be cast to VOID* on 64-bit platforms,
so add an intermediate UINTN cast to make this code build again on 32 bit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
index 8226c80ab466..a43ff95d03a3 100644
--- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
+++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
@@ -657,7 +657,7 @@ InitializeDebugAgent (
   *(UINTN *) (((UINT8 *)VectorBase) + Offset) = (UINTN)AsmCommonExceptionEntry;
 
   // Flush Caches since we updated executable stuff
-  InvalidateInstructionCacheRange ((VOID *)PcdGet64(PcdCpuVectorBaseAddress), Length);
+  InvalidateInstructionCacheRange ((VOID *)(UINTN)PcdGet64(PcdCpuVectorBaseAddress), Length);
 
   // setup a timer so gdb can break in via ctrl-c
   DebugAgentTimerIntialize ();
-- 
2.7.4

[PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls

[Ard Biesheuvel]

Get rid of calls to unsafe string functions. These are deprecated and may
be removed in the future.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c     | 3 ++-
 EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
index bbca90fc08a2..f3e770bcc980 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
@@ -84,7 +84,8 @@ ParseAndroidBootImg (
                  + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
   }
 
-  AsciiStrnCpy (KernelArgs, Header->KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
+  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
+    BOOTIMG_KERNEL_ARGS_SIZE);
 
   return EFI_SUCCESS;
 }
diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
index 9ddc34f57cf4..960218b25241 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
@@ -127,7 +127,7 @@ HandleDownload (
   if (mDataBuffer == NULL) {
     SEND_LITERAL ("FAILNot enough memory");
   } else {
-    AsciiStrnCpy (Response + 4, NumBytesString, 8);
+    AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8);
     mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);
 
     mState = ExpectDataState;
@@ -257,7 +257,7 @@ AcceptCmd (
   }
 
   // Commands aren't null-terminated. Let's get a null-terminated version.
-  AsciiStrnCpy (Command, Data, Size);
+  AsciiStrnCpyS (Command, sizeof Command, Data, Size);
   Command[Size] = '\0';
 
   // Parse command
-- 
2.7.4

[PATCH 6/9] EmbeddedPkg/Ebl: eliminate deprecated string function calls

[Ard Biesheuvel]

Get rid of calls to unsafe string functions. These are deprecated and may
be removed in the future.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Ebl/Command.c   |  2 +-
 EmbeddedPkg/Ebl/Dir.c       |  4 ++--
 EmbeddedPkg/Ebl/EfiDevice.c | 11 ++++++-----
 EmbeddedPkg/Ebl/Main.c      |  8 ++++----
 EmbeddedPkg/Ebl/Variable.c  | 17 +++++++++++------
 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/EmbeddedPkg/Ebl/Command.c b/EmbeddedPkg/Ebl/Command.c
index e75c6a2e5c32..4bc1f4df0ca0 100644
--- a/EmbeddedPkg/Ebl/Command.c
+++ b/EmbeddedPkg/Ebl/Command.c
@@ -614,7 +614,7 @@ OutputData (
   UINTN Spaces   = 0;
   CHAR8 Blanks[80];
 
-  AsciiStrCpy (Blanks, mBlanks);
+  AsciiStrCpyS (Blanks, sizeof Blanks, mBlanks);
   for (EndAddress = Address + Length; Address < EndAddress; Offset += Line) {
     AsciiPrint ("%08x: ", Offset);
     for (Line = 0; (Line < 0x10) && (Address < EndAddress);) {
diff --git a/EmbeddedPkg/Ebl/Dir.c b/EmbeddedPkg/Ebl/Dir.c
index 36095b633019..8dd9d48ff6ac 100644
--- a/EmbeddedPkg/Ebl/Dir.c
+++ b/EmbeddedPkg/Ebl/Dir.c
@@ -116,7 +116,7 @@ EblDirCmd (
     UnicodeFileName[0] = '\0';
     MatchSubString = &UnicodeFileName[0];
     if (Argc > 2) {
-      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
+      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
       if (UnicodeFileName[0] == '*') {
         // Handle *Name substring matching
         MatchSubString = &UnicodeFileName[1];
@@ -231,7 +231,7 @@ EblDirCmd (
     MatchSubString = NULL;
     UnicodeFileName[0] = '\0';
     if (Argc > 2) {
-      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
+      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
       if (UnicodeFileName[0] == '*') {
         MatchSubString = &UnicodeFileName[1];
       }
diff --git a/EmbeddedPkg/Ebl/EfiDevice.c b/EmbeddedPkg/Ebl/EfiDevice.c
index ec9c331b7004..f6969e7b2b05 100644
--- a/EmbeddedPkg/Ebl/EfiDevice.c
+++ b/EmbeddedPkg/Ebl/EfiDevice.c
@@ -343,7 +343,7 @@ EblStartCmd (
 
       ImageInfo->LoadOptionsSize = (UINT32)AsciiStrSize (Argv[2]);
       ImageInfo->LoadOptions     = AllocatePool (ImageInfo->LoadOptionsSize);
-      AsciiStrCpy (ImageInfo->LoadOptions, Argv[2]);
+      AsciiStrCpyS (ImageInfo->LoadOptions, ImageInfo->LoadOptionsSize, Argv[2]);
     }
 
     // Transfer control to the EFI image we loaded with LoadImage()
@@ -741,7 +741,7 @@ EblFileCopyCmd (
   UINTN         Size;
   UINTN         Offset;
   UINTN         Chunk        = FILE_COPY_CHUNK;
-  UINTN         FileNameLen;
+  UINTN         FileNameLen, DestFileNameLen;
   CHAR8*        DestFileName;
   CHAR8*        SrcFileName;
   CHAR8*        SrcPtr;
@@ -786,9 +786,10 @@ EblFileCopyCmd (
     }
 
     // Construct the destination filepath
-    DestFileName = (CHAR8*)AllocatePool (FileNameLen + AsciiStrLen (SrcFileName) + 1);
-    AsciiStrCpy (DestFileName, Argv[2]);
-    AsciiStrCat (DestFileName, SrcFileName);
+    DestFileNameLen = FileNameLen + AsciiStrLen (SrcFileName) + 1;
+    DestFileName = (CHAR8*)AllocatePool (DestFileNameLen);
+    AsciiStrCpyS (DestFileName, DestFileNameLen, Argv[2]);
+    AsciiStrCatS (DestFileName, DestFileNameLen, SrcFileName);
   }
 
   Source = EfiOpen(Argv[1], EFI_FILE_MODE_READ, 0);
diff --git a/EmbeddedPkg/Ebl/Main.c b/EmbeddedPkg/Ebl/Main.c
index 18b2878f69a1..4f04ae4afd33 100644
--- a/EmbeddedPkg/Ebl/Main.c
+++ b/EmbeddedPkg/Ebl/Main.c
@@ -88,7 +88,7 @@ SetCmdHistory (
     }
 
     // Copy the new command line into the ring buffer
-    AsciiStrnCpy(&mCmdHistory[mCmdHistoryStart][0], Cmd, MAX_CMD_LINE);
+    AsciiStrnCpyS (&mCmdHistory[mCmdHistoryStart][0], MAX_CMD_LINE, Cmd, MAX_CMD_LINE);
   }
 
   // Reset the command history for the next up arrow press
@@ -432,7 +432,7 @@ GetCmd (
       }
       AsciiPrint (History);
       Index = AsciiStrLen (History);
-      AsciiStrnCpy (Cmd, History, CmdMaxSize);
+      AsciiStrnCpyS (Cmd, CmdMaxSize, History, CmdMaxSize);
     } else {
       Cmd[Index++] = Char;
       if (FixedPcdGetBool(PcdEmbeddedShellCharacterEcho) == TRUE) {
@@ -644,14 +644,14 @@ EdkBootLoaderEntry (
 
     Status = gRT->GetVariable(CommandLineVariableName, &VendorGuid, NULL, &CommandLineVariableSize, CommandLineVariable);
     if (!EFI_ERROR(Status)) {
-      UnicodeStrToAsciiStr(CommandLineVariable, CmdLine);
+      UnicodeStrToAsciiStrS (CommandLineVariable, CmdLine, CommandLineVariableSize);
     }
 
     FreePool(CommandLineVariable);
   }
 
   if (EFI_ERROR(Status)) {
-    AsciiStrCpy (CmdLine, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
+    AsciiStrCpyS (CmdLine, MAX_CMD_LINE, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
   }
 
   for (;;) {
diff --git a/EmbeddedPkg/Ebl/Variable.c b/EmbeddedPkg/Ebl/Variable.c
index f440c48f16dd..b94531300d07 100644
--- a/EmbeddedPkg/Ebl/Variable.c
+++ b/EmbeddedPkg/Ebl/Variable.c
@@ -29,6 +29,7 @@ EblGetCmd (
   VOID*       Value;
   CHAR8*      AsciiVariableName = NULL;
   CHAR16*     VariableName;
+  UINTN       VariableNameLen;
   UINT32      Index;
 
   if (Argc == 1) {
@@ -48,8 +49,9 @@ EblGetCmd (
     AsciiPrint("Variable name is missing.\n");
     return Status;
   } else {
-    VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
-    AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
+    VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
+    VariableName = AllocatePool(VariableNameLen);
+    AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
   }
 
   // Try to get the variable size.
@@ -93,6 +95,7 @@ EblSetCmd (
   CHAR8*        AsciiValue;
   UINT32        AsciiValueLength;
   CHAR16*       VariableName;
+  UINTN         VariableNameLen;
   UINT32        Index;
   UINT32        EscapedQuotes = 0;
   BOOLEAN       Volatile = FALSE;
@@ -125,8 +128,9 @@ EblSetCmd (
     //
 
     // Convert VariableName into Unicode
-    VariableName = AllocatePool((AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16));
-    AsciiStrToUnicodeStr (AsciiVariableSetting,VariableName);
+    VariableNameLen = (AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16);
+    VariableName = AllocatePool(VariableNameLen);
+    AsciiStrToUnicodeStrS (AsciiVariableSetting, VariableName, VariableNameLen);
 
     Status = gRT->SetVariable (
                           VariableName,
@@ -170,8 +174,9 @@ EblSetCmd (
   }
 
   // Convert VariableName into Unicode
-  VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
-  AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
+  VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
+  VariableName = AllocatePool(VariableNameLen);
+  AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
 
   Status = gRT->SetVariable (
                       VariableName,
-- 
2.7.4

[PATCH 7/9] EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls

[Ard Biesheuvel]

Get rid of calls to unsafe string functions. These are deprecated and may
be removed in the future.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++---------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
index 4d58c830861c..d3b65aa5a3e0 100644
--- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
+++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
@@ -384,9 +384,9 @@ EblFileDevicePath (
 
 
   if ( *FileName != 0 ) {
-    AsciiStrToUnicodeStr (FileName, UnicodeFileName);
+    AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME);
   } else {
-    AsciiStrToUnicodeStr ("\\", UnicodeFileName);
+    AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME);
   }
 
   Size = StrSize (UnicodeFileName);
@@ -589,7 +589,7 @@ EblFvFileDevicePath (
           &AuthenticationStatus
           );
         if (!EFI_ERROR (Status)) {
-          UnicodeStrToAsciiStr (Section, AsciiSection);
+          UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME);
           if (AsciiStriCmp (FileName, AsciiSection) == 0) {
             FreePool (Section);
             break;
@@ -674,6 +674,7 @@ EfiOpen (
   CHAR8                     *CwdPlusPathName;
   UINTN                     Index;
   EFI_SECTION_TYPE          ModifiedSectionType;
+  UINTN                     AsciiLength;
 
   EblUpdateDeviceLists ();
 
@@ -706,7 +707,8 @@ EfiOpen (
       }
 
       // We could add a current working directory concept
-      CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName));
+      AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName);
+      CwdPlusPathName = AllocatePool (AsciiLength);
       if (CwdPlusPathName == NULL) {
         return NULL;
       }
@@ -723,14 +725,14 @@ EfiOpen (
           }
         }
       } else {
-        AsciiStrCpy (CwdPlusPathName, gCwd);
+        AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd);
         StrLen = AsciiStrLen (gCwd);
         if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) {
-          AsciiStrCat (CwdPlusPathName, "\\");
+          AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\");
         }
       }
 
-      AsciiStrCat (CwdPlusPathName, PathName);
+      AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName);
       if (AsciiStrStr (CwdPlusPathName, ":") == NULL) {
         // Extra error check to make sure we don't recurse and blow stack
         return NULL;
@@ -745,7 +747,7 @@ EfiOpen (
   }
 
   File->DeviceName = AllocatePool (StrLen);
-  AsciiStrCpy (File->DeviceName, PathName);
+  AsciiStrCpyS (File->DeviceName, StrLen, PathName);
   File->DeviceName[FileStart - 1] = '\0';
   File->FileName = &File->DeviceName[FileStart];
   if (File->FileName[0] == '\0') {
@@ -1611,7 +1613,7 @@ ExpandPath (
 {
   CHAR8   *NewPath;
   CHAR8   *Work, *Start, *End;
-  UINTN   StrLen;
+  UINTN   StrLen, AllocLen;
   INTN    i;
 
   if (Cwd == NULL || Path == NULL) {
@@ -1625,11 +1627,12 @@ ExpandPath (
   }
 
   StrLen = AsciiStrSize (Path);
-  NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1);
+  AllocLen = AsciiStrSize (Cwd) + StrLen + 1;
+  NewPath = AllocatePool (AllocLen);
   if (NewPath == NULL) {
     return NULL;
   }
-  AsciiStrCpy (NewPath, Cwd);
+  AsciiStrCpyS (NewPath, AllocLen, Cwd);
 
   End = Path + StrLen;
   for (Start = Path ;;) {
@@ -1640,7 +1643,7 @@ ExpandPath (
     }
 
     // append path prior to ..
-    AsciiStrnCat (NewPath, Start, Work - Start);
+    AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start);
     StrLen = AsciiStrLen (NewPath);
     for (i = StrLen; i >= 0; i--) {
       if (NewPath[i] == ':') {
@@ -1663,7 +1666,7 @@ ExpandPath (
   }
 
   // Handle the path that remains after the ..
-  AsciiStrnCat (NewPath, Start, End - Start);
+  AsciiStrnCatS (NewPath, AllocLen, Start, End - Start);
 
   return NewPath;
 }
@@ -1686,7 +1689,7 @@ EfiSetCwd (
   )
 {
   EFI_OPEN_FILE *File;
-  UINTN         Len;
+  UINTN         Len, AllocLen;
   CHAR8         *Path;
 
   if (Cwd == NULL) {
@@ -1729,17 +1732,18 @@ EfiSetCwd (
 
   // Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be
   // relative to the current gCwd or not.
-  gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10);
+  AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10;
+  gCwd = AllocatePool (AllocLen);
   if (gCwd == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  AsciiStrCpy (gCwd, File->DeviceName);
+  AsciiStrCpyS (gCwd, AllocLen, File->DeviceName);
   if (File->FileName == NULL) {
-    AsciiStrCat (gCwd, ":\\");
+    AsciiStrCatS (gCwd, AllocLen, ":\\");
   } else {
-    AsciiStrCat (gCwd, ":");
-    AsciiStrCat (gCwd, File->FileName);
+    AsciiStrCatS (gCwd, AllocLen, ":");
+    AsciiStrCatS (gCwd, AllocLen, File->FileName);
   }
 
 
-- 
2.7.4

[PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls

[Ard Biesheuvel]

Get rid of calls to unsafe string functions. These are deprecated and may
be removed in the future.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/Diagnostics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
index 783e548d2aed..b489393e27b0 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
+++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
@@ -44,7 +44,7 @@ DiagnosticLog (
   UINTN len = StrLen (Str);
   if (len <= mLogRemainChar) {
     mLogRemainChar -= len;
-    StrCpy (mLogBuffer, Str);
+    StrCpyS (mLogBuffer, mLogRemainChar, Str);
     mLogBuffer += len;
     return len;
   } else {
-- 
2.7.4

[PATCH 9/9] EmbeddedPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES

[Ard Biesheuvel]

Define DISABLE_NEW_DEPRECATED_INTERFACES on the compiler command line by
default, to prevent deprecated interfaces from being used in core EDK2
code.

Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dsc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index cc95564f31af..eb7af800f0b2 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -4,6 +4,7 @@
 #
 # Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
 # Copyright (c) 2012-2015, ARM Ltd. All rights reserved.<BR>
+# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
 #
 #    This program and the accompanying materials
 #    are licensed and made available under the terms and conditions of the BSD License
@@ -227,7 +228,7 @@ [PcdsFixedAtBuild.IPF]
 
 [BuildOptions]
   RVCT:*_*_ARM_PLATFORM_FLAGS == --cpu=7-A.security
-
+  *_*_*_CC_FLAGS  = -DDISABLE_NEW_DEPRECATED_INTERFACES
 
 ################################################################################
 #
-- 
2.7.4

Re: [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size

[Laszlo Ersek]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> The result of PcdGet64() can only be cast to VOID* on 64-bit platforms,
> so add an intermediate UINTN cast to make this code build again on 32 bit.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> index 8226c80ab466..a43ff95d03a3 100644
> --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> @@ -657,7 +657,7 @@ InitializeDebugAgent (
>    *(UINTN *) (((UINT8 *)VectorBase) + Offset) = (UINTN)AsmCommonExceptionEntry;
>  
>    // Flush Caches since we updated executable stuff
> -  InvalidateInstructionCacheRange ((VOID *)PcdGet64(PcdCpuVectorBaseAddress), Length);
> +  InvalidateInstructionCacheRange ((VOID *)(UINTN)PcdGet64(PcdCpuVectorBaseAddress), Length);
>  
>    // setup a timer so gdb can break in via ctrl-c
>    DebugAgentTimerIntialize ();
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls

[Laszlo Ersek]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> Get rid of calls to unsafe string functions. These are deprecated and may
> be removed in the future.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c     | 3 ++-
>  EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> index bbca90fc08a2..f3e770bcc980 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> @@ -84,7 +84,8 @@ ParseAndroidBootImg (
>                   + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>    }
>  
> -  AsciiStrnCpy (KernelArgs, Header->KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
> +  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> +    BOOTIMG_KERNEL_ARGS_SIZE);
>  
>    return EFI_SUCCESS;
>  }

This loses the zero padding, but I guess that's okay. Is fine otherwise.

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> index 9ddc34f57cf4..960218b25241 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
> @@ -127,7 +127,7 @@ HandleDownload (
>    if (mDataBuffer == NULL) {
>      SEND_LITERAL ("FAILNot enough memory");
>    } else {
> -    AsciiStrnCpy (Response + 4, NumBytesString, 8);
> +    AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8);
>      mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);
>  
>      mState = ExpectDataState;

I don't think this is right. Here we're trying to copy no more than 8
characters from NumBytesString into Response, after the "DATA" prefix.
mNumDataBytes is the decimal value of NumBytesString, and it's unrelated
to this formatting.

What we could do is

  AsciiStrnCpyS (Response + 4, sizeof Response - 4, NumBytesString, 8)

in order to remain "surgical". However, that's not right again, because
AsciiStrnCpyS() *always* NUL-terminates, and here we only have

  CHAR8       Response[12] = "DATA";

i.e., no room for values above 0x0FFF_FFFF. Another issue is that the
zero padding of AsciiStrnCpy() would be lost, and we actually send the
zero padding to the wire.

So, the real fix is, in my opinion:

* resize Response to 4 + 8 + 1 == 13 bytes,

* format it like this:
  ZeroMem (Response, sizeof Response);
  AsciiSPrint (Response, sizeof Response, "DATA%x",
    (UINT32)mNumDataBytes);

* send it like this:
  mTransport->Send (sizeof Response - 1, Response,
    &mFatalSendErrorEvent);


> @@ -257,7 +257,7 @@ AcceptCmd (
>    }
>  
>    // Commands aren't null-terminated. Let's get a null-terminated version.
> -  AsciiStrnCpy (Command, Data, Size);
> +  AsciiStrnCpyS (Command, sizeof Command, Data, Size);
>    Command[Size] = '\0';
>  
>    // Parse command
> 

This looks good, but the explicit NUL-termination can be dropped, as
AsciiStrnCpyS() enforces that internally.

Thanks
Laszlo

Re: [PATCH 6/9] EmbeddedPkg/Ebl: eliminate deprecated string function calls

[Laszlo Ersek]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> Get rid of calls to unsafe string functions. These are deprecated and may
> be removed in the future.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Ebl/Command.c   |  2 +-
>  EmbeddedPkg/Ebl/Dir.c       |  4 ++--
>  EmbeddedPkg/Ebl/EfiDevice.c | 11 ++++++-----
>  EmbeddedPkg/Ebl/Main.c      |  8 ++++----
>  EmbeddedPkg/Ebl/Variable.c  | 17 +++++++++++------
>  5 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/EmbeddedPkg/Ebl/Command.c b/EmbeddedPkg/Ebl/Command.c
> index e75c6a2e5c32..4bc1f4df0ca0 100644
> --- a/EmbeddedPkg/Ebl/Command.c
> +++ b/EmbeddedPkg/Ebl/Command.c
> @@ -614,7 +614,7 @@ OutputData (
>    UINTN Spaces   = 0;
>    CHAR8 Blanks[80];
>  
> -  AsciiStrCpy (Blanks, mBlanks);
> +  AsciiStrCpyS (Blanks, sizeof Blanks, mBlanks);

The original code is so poor that it makes me weep. (Not knowing who
wrote this, and I don't want to look it up.) The mBlanks array has 43
(forty three) space characters in it.

Please drop the mBlanks array, and use a SetMem() call here.

Or... meh, I don't care. It's fine as it is.

>    for (EndAddress = Address + Length; Address < EndAddress; Offset += Line) {
>      AsciiPrint ("%08x: ", Offset);
>      for (Line = 0; (Line < 0x10) && (Address < EndAddress);) {
> diff --git a/EmbeddedPkg/Ebl/Dir.c b/EmbeddedPkg/Ebl/Dir.c
> index 36095b633019..8dd9d48ff6ac 100644
> --- a/EmbeddedPkg/Ebl/Dir.c
> +++ b/EmbeddedPkg/Ebl/Dir.c
> @@ -116,7 +116,7 @@ EblDirCmd (
>      UnicodeFileName[0] = '\0';
>      MatchSubString = &UnicodeFileName[0];
>      if (Argc > 2) {
> -      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
> +      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
>        if (UnicodeFileName[0] == '*') {
>          // Handle *Name substring matching
>          MatchSubString = &UnicodeFileName[1];

Looks okay.

> @@ -231,7 +231,7 @@ EblDirCmd (
>      MatchSubString = NULL;
>      UnicodeFileName[0] = '\0';
>      if (Argc > 2) {
> -      AsciiStrToUnicodeStr (Argv[2], UnicodeFileName);
> +      AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE);
>        if (UnicodeFileName[0] == '*') {
>          MatchSubString = &UnicodeFileName[1];
>        }

Ditto.

> diff --git a/EmbeddedPkg/Ebl/EfiDevice.c b/EmbeddedPkg/Ebl/EfiDevice.c
> index ec9c331b7004..f6969e7b2b05 100644
> --- a/EmbeddedPkg/Ebl/EfiDevice.c
> +++ b/EmbeddedPkg/Ebl/EfiDevice.c
> @@ -343,7 +343,7 @@ EblStartCmd (
>  
>        ImageInfo->LoadOptionsSize = (UINT32)AsciiStrSize (Argv[2]);
>        ImageInfo->LoadOptions     = AllocatePool (ImageInfo->LoadOptionsSize);
> -      AsciiStrCpy (ImageInfo->LoadOptions, Argv[2]);
> +      AsciiStrCpyS (ImageInfo->LoadOptions, ImageInfo->LoadOptionsSize, Argv[2]);
>      }
>  
>      // Transfer control to the EFI image we loaded with LoadImage()

AllocateCopyPool() would be much better, but this will do.

> @@ -741,7 +741,7 @@ EblFileCopyCmd (
>    UINTN         Size;
>    UINTN         Offset;
>    UINTN         Chunk        = FILE_COPY_CHUNK;
> -  UINTN         FileNameLen;
> +  UINTN         FileNameLen, DestFileNameLen;
>    CHAR8*        DestFileName;
>    CHAR8*        SrcFileName;
>    CHAR8*        SrcPtr;
> @@ -786,9 +786,10 @@ EblFileCopyCmd (
>      }
>  
>      // Construct the destination filepath
> -    DestFileName = (CHAR8*)AllocatePool (FileNameLen + AsciiStrLen (SrcFileName) + 1);
> -    AsciiStrCpy (DestFileName, Argv[2]);
> -    AsciiStrCat (DestFileName, SrcFileName);
> +    DestFileNameLen = FileNameLen + AsciiStrLen (SrcFileName) + 1;
> +    DestFileName = (CHAR8*)AllocatePool (DestFileNameLen);
> +    AsciiStrCpyS (DestFileName, DestFileNameLen, Argv[2]);
> +    AsciiStrCatS (DestFileName, DestFileNameLen, SrcFileName);
>    }
>  
>    Source = EfiOpen(Argv[1], EFI_FILE_MODE_READ, 0);

AsciiSPrint would be (and would have been, originally) superior, but
this will do.

> diff --git a/EmbeddedPkg/Ebl/Main.c b/EmbeddedPkg/Ebl/Main.c
> index 18b2878f69a1..4f04ae4afd33 100644
> --- a/EmbeddedPkg/Ebl/Main.c
> +++ b/EmbeddedPkg/Ebl/Main.c
> @@ -88,7 +88,7 @@ SetCmdHistory (
>      }
>  
>      // Copy the new command line into the ring buffer
> -    AsciiStrnCpy(&mCmdHistory[mCmdHistoryStart][0], Cmd, MAX_CMD_LINE);
> +    AsciiStrnCpyS (&mCmdHistory[mCmdHistoryStart][0], MAX_CMD_LINE, Cmd, MAX_CMD_LINE);
>    }
>  
>    // Reset the command history for the next up arrow press
> @@ -432,7 +432,7 @@ GetCmd (
>        }
>        AsciiPrint (History);
>        Index = AsciiStrLen (History);
> -      AsciiStrnCpy (Cmd, History, CmdMaxSize);
> +      AsciiStrnCpyS (Cmd, CmdMaxSize, History, CmdMaxSize);
>      } else {
>        Cmd[Index++] = Char;
>        if (FixedPcdGetBool(PcdEmbeddedShellCharacterEcho) == TRUE) {
> @@ -644,14 +644,14 @@ EdkBootLoaderEntry (
>  
>      Status = gRT->GetVariable(CommandLineVariableName, &VendorGuid, NULL, &CommandLineVariableSize, CommandLineVariable);
>      if (!EFI_ERROR(Status)) {
> -      UnicodeStrToAsciiStr(CommandLineVariable, CmdLine);
> +      UnicodeStrToAsciiStrS (CommandLineVariable, CmdLine, CommandLineVariableSize);
>      }

This is wrong, the last argument should be MAX_CMD_LINE.

CommandLineVariableSize is the size (incl. the terminating NUL) of the
UCS2 string read from the variable. The last argument of
UnicodeStrToAsciiStrS() is DestMax, "The maximum number of Destination
scii char, including terminating null char.". That is, MAX_CMD_LINE.

>  
>      FreePool(CommandLineVariable);
>    }
>  
>    if (EFI_ERROR(Status)) {
> -    AsciiStrCpy (CmdLine, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
> +    AsciiStrCpyS (CmdLine, MAX_CMD_LINE, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
>    }
>  
>    for (;;) {

This looks good.

> diff --git a/EmbeddedPkg/Ebl/Variable.c b/EmbeddedPkg/Ebl/Variable.c
> index f440c48f16dd..b94531300d07 100644
> --- a/EmbeddedPkg/Ebl/Variable.c
> +++ b/EmbeddedPkg/Ebl/Variable.c
> @@ -29,6 +29,7 @@ EblGetCmd (
>    VOID*       Value;
>    CHAR8*      AsciiVariableName = NULL;
>    CHAR16*     VariableName;
> +  UINTN       VariableNameLen;
>    UINT32      Index;
>  
>    if (Argc == 1) {
> @@ -48,8 +49,9 @@ EblGetCmd (
>      AsciiPrint("Variable name is missing.\n");
>      return Status;
>    } else {
> -    VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
> -    AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
> +    VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
> +    VariableName = AllocatePool(VariableNameLen);
> +    AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
>    }
>  
>    // Try to get the variable size.

This is wrong. VariableNameLen is the size in bytes of a CHAR16 array
(incl. the terminating L'\0'). The last argument of
AsciiStrToUnicodeStrS(), is DestMax: "The maximum number of Destination
Unicode char, including terminating null char."

> @@ -93,6 +95,7 @@ EblSetCmd (
>    CHAR8*        AsciiValue;
>    UINT32        AsciiValueLength;
>    CHAR16*       VariableName;
> +  UINTN         VariableNameLen;
>    UINT32        Index;
>    UINT32        EscapedQuotes = 0;
>    BOOLEAN       Volatile = FALSE;
> @@ -125,8 +128,9 @@ EblSetCmd (
>      //
>  
>      // Convert VariableName into Unicode
> -    VariableName = AllocatePool((AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16));
> -    AsciiStrToUnicodeStr (AsciiVariableSetting,VariableName);
> +    VariableNameLen = (AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16);
> +    VariableName = AllocatePool(VariableNameLen);
> +    AsciiStrToUnicodeStrS (AsciiVariableSetting, VariableName, VariableNameLen);
>  
>      Status = gRT->SetVariable (
>                            VariableName,

Same issue here.

> @@ -170,8 +174,9 @@ EblSetCmd (
>    }
>  
>    // Convert VariableName into Unicode
> -  VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16));
> -  AsciiStrToUnicodeStr (AsciiVariableName,VariableName);
> +  VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16);
> +  VariableName = AllocatePool(VariableNameLen);
> +  AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen);
>  
>    Status = gRT->SetVariable (
>                        VariableName,
> 

And here.

Thanks
Laszlo

Re: [PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls

[Laszlo Ersek]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> Get rid of calls to unsafe string functions. These are deprecated and may
> be removed in the future.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Universal/MmcDxe/Diagnostics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> index 783e548d2aed..b489393e27b0 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> @@ -44,7 +44,7 @@ DiagnosticLog (
>    UINTN len = StrLen (Str);
>    if (len <= mLogRemainChar) {
>      mLogRemainChar -= len;
> -    StrCpy (mLogBuffer, Str);
> +    StrCpyS (mLogBuffer, mLogRemainChar, Str);
>      mLogBuffer += len;
>      return len;
>    } else {
> 

I think this is incorrect: "mLogRemainChar" is decreased before printing.

Also, the original controlling expression is broken: it will permit the
body with (len == mLogRemainChar). The effect thereof, for the original
code, is a buffer overflow, the terminating L'\0' will be written
outside of the buffer. (See the allocation in DiagnosticInitLog().)

This buffer overflow is actually fixed by the patch (by throwing away
the last character and storing an L'\0' instead). However, that in turn
invalidates the returned value (because not all "len" characters have
been written).

So, please move the decrement after the StrCpyS(), and replace the <=
with <.

Thanks
Laszlo

Re: [PATCH 7/9] EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls

[Laszlo Ersek]

On 10/24/16 19:41, Ard Biesheuvel wrote:
> Get rid of calls to unsafe string functions. These are deprecated and may
> be removed in the future.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++---------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> index 4d58c830861c..d3b65aa5a3e0 100644
> --- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> +++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> @@ -384,9 +384,9 @@ EblFileDevicePath (
>  
>  
>    if ( *FileName != 0 ) {
> -    AsciiStrToUnicodeStr (FileName, UnicodeFileName);
> +    AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME);

Seems okay.

>    } else {
> -    AsciiStrToUnicodeStr ("\\", UnicodeFileName);
> +    AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME);
>    }

Ditto.

>  
>    Size = StrSize (UnicodeFileName);
> @@ -589,7 +589,7 @@ EblFvFileDevicePath (
>            &AuthenticationStatus
>            );
>          if (!EFI_ERROR (Status)) {
> -          UnicodeStrToAsciiStr (Section, AsciiSection);
> +          UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME);
>            if (AsciiStriCmp (FileName, AsciiSection) == 0) {
>              FreePool (Section);
>              break;

Seems fine.

> @@ -674,6 +674,7 @@ EfiOpen (
>    CHAR8                     *CwdPlusPathName;
>    UINTN                     Index;
>    EFI_SECTION_TYPE          ModifiedSectionType;
> +  UINTN                     AsciiLength;
>  
>    EblUpdateDeviceLists ();
>  
> @@ -706,7 +707,8 @@ EfiOpen (
>        }
>  
>        // We could add a current working directory concept
> -      CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName));
> +      AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName);
> +      CwdPlusPathName = AllocatePool (AsciiLength);
>        if (CwdPlusPathName == NULL) {
>          return NULL;
>        }
> @@ -723,14 +725,14 @@ EfiOpen (
>            }
>          }
>        } else {
> -        AsciiStrCpy (CwdPlusPathName, gCwd);
> +        AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd);

okay

>          StrLen = AsciiStrLen (gCwd);
>          if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) {
> -          AsciiStrCat (CwdPlusPathName, "\\");
> +          AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\");
>          }
>        }

okay

>  
> -      AsciiStrCat (CwdPlusPathName, PathName);
> +      AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName);
>        if (AsciiStrStr (CwdPlusPathName, ":") == NULL) {
>          // Extra error check to make sure we don't recurse and blow stack
>          return NULL;

Also fine.

(I strongly dislike the trickery in the original allocation, that is,
summing two full sizes (each including the terminating NUL separately),
and then letting one of those NULs account for the pathname separator
'\\' that gets inserted in the middle. It happens to be correct, but
seriously...)

> @@ -745,7 +747,7 @@ EfiOpen (
>    }
>  
>    File->DeviceName = AllocatePool (StrLen);

StrLen was initially assigned like this:

  StrLen = AsciiStrSize (PathName);

(Q: what would you call a variable storing the retval of a function
named XxxxSize()? A: XxxxLen, obviously!

Never mind that there is a function named XxxxLen() as well; we'll store
the retval of *that* in XxxxSize. For clarity.)

... Just to be clear, this is criticism for the original code.

> -  AsciiStrCpy (File->DeviceName, PathName);
> +  AsciiStrCpyS (File->DeviceName, StrLen, PathName);

okay.

>    File->DeviceName[FileStart - 1] = '\0';
>    File->FileName = &File->DeviceName[FileStart];
>    if (File->FileName[0] == '\0') {
> @@ -1611,7 +1613,7 @@ ExpandPath (
>  {
>    CHAR8   *NewPath;
>    CHAR8   *Work, *Start, *End;
> -  UINTN   StrLen;
> +  UINTN   StrLen, AllocLen;
>    INTN    i;
>  
>    if (Cwd == NULL || Path == NULL) {
> @@ -1625,11 +1627,12 @@ ExpandPath (
>    }
>  
>    StrLen = AsciiStrSize (Path);
> -  NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1);
> +  AllocLen = AsciiStrSize (Cwd) + StrLen + 1;
> +  NewPath = AllocatePool (AllocLen);
>    if (NewPath == NULL) {
>      return NULL;
>    }
> -  AsciiStrCpy (NewPath, Cwd);
> +  AsciiStrCpyS (NewPath, AllocLen, Cwd);
>  
>    End = Path + StrLen;
>    for (Start = Path ;;) {
> @@ -1640,7 +1643,7 @@ ExpandPath (
>      }
>  
>      // append path prior to ..
> -    AsciiStrnCat (NewPath, Start, Work - Start);
> +    AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start);
>      StrLen = AsciiStrLen (NewPath);
>      for (i = StrLen; i >= 0; i--) {
>        if (NewPath[i] == ':') {
> @@ -1663,7 +1666,7 @@ ExpandPath (
>    }
>  
>    // Handle the path that remains after the ..
> -  AsciiStrnCat (NewPath, Start, End - Start);
> +  AsciiStrnCatS (NewPath, AllocLen, Start, End - Start);
>  
>    return NewPath;
>  }

looks good

> @@ -1686,7 +1689,7 @@ EfiSetCwd (
>    )
>  {
>    EFI_OPEN_FILE *File;
> -  UINTN         Len;
> +  UINTN         Len, AllocLen;
>    CHAR8         *Path;
>  
>    if (Cwd == NULL) {
> @@ -1729,17 +1732,18 @@ EfiSetCwd (
>  
>    // Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be
>    // relative to the current gCwd or not.
> -  gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10);
> +  AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10;
> +  gCwd = AllocatePool (AllocLen);
>    if (gCwd == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  AsciiStrCpy (gCwd, File->DeviceName);
> +  AsciiStrCpyS (gCwd, AllocLen, File->DeviceName);
>    if (File->FileName == NULL) {
> -    AsciiStrCat (gCwd, ":\\");
> +    AsciiStrCatS (gCwd, AllocLen, ":\\");
>    } else {
> -    AsciiStrCat (gCwd, ":");
> -    AsciiStrCat (gCwd, File->FileName);
> +    AsciiStrCatS (gCwd, AllocLen, ":");
> +    AsciiStrCatS (gCwd, AllocLen, File->FileName);
>    }
>  
>  
> 

Looks correct.

(The constant 10 in the addition, for the allocation, represents how
carefully the original code was written.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I need some brain bleach now.