fix: multiboot mod string in zero-terminated ASCII format #1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because GetFromConfigFile
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

For performance, the patch does not run "isascii" check.

Minor changes:
  - Fix typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h
  - Dump mod string for debug build

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Acked-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
Signed-off-by: Stanley Chang <stanley.chang@intel.com>

BootloaderCommonPkg/Include/Library/IasImageLib.h

@@ -129,6 +129,21 @@ IasGetFiles (
   OUT  IMAGE_DATA  *Img
   );
 
+/**
+  Free the allocated memory in an image data
+
+  This function free a memory allocated in IMAGE_DATA according to Allocation Type.
+
+  @param[in]  ImageData       An image data pointer which has allocated memory address,
+                              its size, and allocation type.
+
+**/
+VOID
+EFIAPI
+FreeImageData (
+  IN  IMAGE_DATA    *ImageData
+  );
+
 // Image type subfields (cf. boot subsustem HLD, appendix A for details).
 #define IAS_IMAGE_TYPE(it)      (((it) & 0xffff0000) >> 16)
 #define IAS_IMAGE_IS_SIGNED(it)  ((it) &      0x100)

BootloaderCommonPkg/Include/Library/MultibootLib.h

@@ -1,4 +1,7 @@
 /** @file
+  Copyright (c) 2018 - 2023, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Paten
+
   Copyright (c) 2005, 2006 The NetBSD Foundation, Inc.
   All rights reserved.
 
@@ -360,4 +363,22 @@ UpdateMultiboot2MemInfo (
   IN UINT32                   RsvdMemExtra
   );
 
+/**
+  Load Multiboot module string
+
+  @param[in,out] MultiBoot   Point to loaded Multiboot image structure
+  @param[in] ModuleIndex     Module index to load
+  @param[in] File            Source image file
+
+  @retval  EFI_SUCCESS       Load Multiboot module image successfully
+  @retval  Others            There is error when setup image
+**/
+EFI_STATUS
+EFIAPI
+LoadMultibootModString (
+  IN OUT MULTIBOOT_IMAGE     *MultiBoot,
+  IN     UINT16              ModuleIndex,
+  IN     IMAGE_DATA          *File
+  );
+
 #endif

BootloaderCommonPkg/Library/MultibootLib/Multiboot.c

@@ -1,7 +1,7 @@
 /** @file
   This file Multiboot specification (implementation).
 
-  Copyright (c) 2014 - 2022, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2023, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -14,6 +14,7 @@
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/BootloaderCommonLib.h>
+#include <Library/LinuxLib.h>
 #include <Guid/MemoryMapInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>
 #include "MultibootLibInternal.h"
@@ -122,7 +123,7 @@ GetMemoryMapInfo (
   @param[in]   MemoryMapInfo  Memmap buffer from boot loader
 **/
 VOID
-InitMultibotMmap (
+InitMultibootMmap (
   OUT     MULTIBOOT_MMAP     *MbMmap,
   IN      MEMORY_MAP_INFO    *MemoryMapInfo
   )
@@ -167,7 +168,7 @@ SetupMultibootInfo (
       DEBUG ((DEBUG_INFO, "Multiboot MMap allocation Error\n"));
       ASSERT (MbMmap);
     }
-    InitMultibotMmap (MbMmap, MemoryMapInfo);
+    InitMultibootMmap (MbMmap, MemoryMapInfo);
 
     MbInfo->MmapAddr   = (UINT32 *) MbMmap;
     MbInfo->MmapLength = MmapCount * sizeof (MULTIBOOT_MMAP);
@@ -231,6 +232,81 @@ SetupMultibootInfo (
   MultiBoot->BootState.Ebx = (UINT32)(UINTN)MbInfo;
 }
 
+/**
+  Load Multiboot module string
+
+  @param[in,out] MultiBoot   Point to loaded Multiboot image structure
+  @param[in] ModuleIndex     Module index to load
+  @param[in] File            Source image file
+
+  @retval  EFI_SUCCESS       Load Multiboot module image successfully
+  @retval  Others            There is error when setup image
+**/
+EFI_STATUS
+EFIAPI
+LoadMultibootModString (
+  IN OUT MULTIBOOT_IMAGE     *MultiBoot,
+  IN     UINT16              ModuleIndex,
+  IN     IMAGE_DATA          *File
+  )
+{
+  EFI_STATUS                 Status;
+  UINT32                     Index;
+  IMAGE_DATA                 *CmdFile;
+  UINT8                      *NewCmdBuffer;
+  UINT32                     NewSize;
+
+  if ((MultiBoot == NULL) || (File == NULL) || (ModuleIndex >= MultiBoot->MbModuleNumber)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  CmdFile = &MultiBoot->MbModuleData[ModuleIndex].CmdFile;
+  CopyMem (CmdFile, File, sizeof (IMAGE_DATA));
+
+  Status = EFI_SUCCESS;
+
+  if (CmdFile->Addr == NULL || CmdFile->Size == 0) {
+    goto done;
+  }
+
+  // multiboot spec requires a mod string be a zero-terminated ASCII string
+  // fast check: 1st char is zero or zero-terminated
+  if (((UINT8 *)CmdFile->Addr)[0] == '\0') {
+    goto done;
+  }
+
+  for (Index = (CmdFile->Size-1); Index > 0; Index--) {
+    if (((UINT8 *)CmdFile->Addr)[Index] == '\0') {
+      goto done;
+    }
+  }
+
+  // if the mod string is not zero-terminated, allocate a new buffer to append zero char
+  NewCmdBuffer = (UINT8 *) AllocatePages (EFI_SIZE_TO_PAGES (CMDLINE_LENGTH_MAX));
+  if (NewCmdBuffer == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto done;
+  }
+
+  // no ASCII check here for performance
+  NewSize = (CmdFile->Size > (CMDLINE_LENGTH_MAX -1)) ? CMDLINE_LENGTH_MAX: (CmdFile->Size + 1);
+  CopyMem (NewCmdBuffer, CmdFile->Addr, NewSize - 1);
+  NewCmdBuffer[NewSize - 1] = '\0';
+
+  // Free Allocated Memory earlier first
+  FreeImageData (CmdFile);
+  // Assign new one
+  CmdFile->Addr = NewCmdBuffer;
+  CmdFile->Size = NewSize;
+  CmdFile->AllocType = ImageAllocateTypePage;
+
+done:
+  if (Status == EFI_SUCCESS) {
+    MultiBoot->MbModule[ModuleIndex].String = (UINT8 *) MultiBoot->MbModuleData[ModuleIndex].CmdFile.Addr;
+  }
+
+  return Status;
+}
 
 /**
   Align multiboot modules if required by spec.
@@ -420,6 +496,7 @@ DumpMbInfo (
       DEBUG ((DEBUG_INFO, "- Mod[%d].Start:      %08x\n", Index, Mod[Index].Start));
       DEBUG ((DEBUG_INFO, "- Mod[%d].End:        %08x\n", Index, Mod[Index].End));
       DEBUG ((DEBUG_INFO, "- Mod[%d].String:     %08x\n", Index, (UINT32)(UINTN)Mod[Index].String));
+      DEBUG ((DEBUG_INFO, "  string = '%a'\n", Mod[Index].String));
     }
   }
 

PayloadPkg/OsLoader/LoadImage.c

@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -637,6 +637,7 @@ GetLoadedImageByType (
 
 **/
 VOID
+EFIAPI
 FreeImageData (
   IN  IMAGE_DATA    *ImageData
   )

PayloadPkg/OsLoader/OsLoader.c

@@ -269,10 +269,9 @@ UpdateLoadedImage (
         //
         // IMAGE_DATA memory will be freed later if fails
         //
-        CopyMem (&MultiBoot->MbModuleData[ModuleIndex].CmdFile, &File[Index], sizeof (IMAGE_DATA));
+        Status = LoadMultibootModString(MultiBoot, ModuleIndex, &File[Index]);
         CopyMem (&MultiBoot->MbModuleData[ModuleIndex].ImgFile, &File[Index + 1], sizeof (IMAGE_DATA));
 
-        MultiBoot->MbModule[ModuleIndex].String = (UINT8 *) MultiBoot->MbModuleData[ModuleIndex].CmdFile.Addr;
         MultiBoot->MbModule[ModuleIndex].Start  = (UINT32)(UINTN)MultiBoot->MbModuleData[ModuleIndex].ImgFile.Addr;
         MultiBoot->MbModule[ModuleIndex].End    = (UINT32)(UINTN)MultiBoot->MbModuleData[ModuleIndex].ImgFile.Addr
           + MultiBoot->MbModuleData[ModuleIndex].ImgFile.Size;

PayloadPkg/OsLoader/OsLoader.h

@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2017 - 2022, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -254,20 +254,6 @@ UnloadLoadedImage (
   IN  LOADED_IMAGE  *LoadedImage
   );
 
-/**
-  Free the allocated memory in an image data
-
-  This function free a memory allocated in IMAGE_DATA according to Allocation Type.
-
-  @param[in]  ImageData       An image data pointer which has allocated memory address,
-                              its size, and allocation type.
-
-**/
-VOID
-FreeImageData (
-  IN  IMAGE_DATA    *ImageData
-  );
-
 /**
   Load Image from boot media.