Code cleanup and improvements

* Move Root handle lookup into its own function call.
* Rename the number or entries from the hash list to something more explicit.
* Remove unneeded/unwanted code.
* Add file size validation in HashFile().
* Improve parameter validation.

.github/workflows/Linux.yml

@@ -24,19 +24,19 @@ jobs:
         include:
         - TARGET_TYPE: x64
           TARGET_ARCH: X64
-          TARGET_PKGS: python3-distutils nasm uuid-dev qemu-system-x86
+          TARGET_PKGS: gcc-multilib
         - TARGET_TYPE: ia32
           TARGET_ARCH: IA32
-          TARGET_PKGS: gcc-multilib python3-distutils nasm uuid-dev qemu-system-x86
+          TARGET_PKGS: gcc-multilib
         - TARGET_TYPE: aa64
           TARGET_ARCH: AARCH64
-          TARGET_PKGS: gcc-aarch64-linux-gnu python3-distutils uuid-dev qemu-system-arm
+          TARGET_PKGS: gcc-aarch64-linux-gnu
         - TARGET_TYPE: arm
           TARGET_ARCH: ARM
-          TARGET_PKGS: gcc-arm-linux-gnueabi python3-distutils uuid-dev qemu-system-arm
+          TARGET_PKGS: gcc-arm-linux-gnueabi
         - TARGET_TYPE: riscv64
           TARGET_ARCH: RISCV64
-          TARGET_PKGS: gcc-riscv64-linux-gnu python3-distutils uuid-dev qemu-system-misc
+          TARGET_PKGS: gcc-riscv64-linux-gnu
 
     steps:
     - name: Check out repository
@@ -58,7 +58,7 @@ jobs:
     - name: Set up Linux environment
       run: |
         sudo apt-get update
-        sudo apt-get -y --no-install-recommends install ${{ matrix.TARGET_PKGS }}
+        sudo apt-get -y --no-install-recommends install python3-distutils nasm uuid-dev ${{ matrix.TARGET_PKGS }}
 
     - name: Set up EDK2
       run: |

Md5SumPkg.dec

@@ -11,6 +11,3 @@
   PACKAGE_UNI_FILE               = Md5SumPkg.uni
   PACKAGE_GUID                   = B5FFEC2E-91BD-4DFB-B703-265DEEB856D8
   PACKAGE_VERSION                = 1.0
-
-[UserExtensions.TianoCore."ExtraFiles"]
-  Md5SumPkg-extra.uni

Md5SumPkg.dsc

@@ -1,7 +1,7 @@
 ## @file
 #  MD5Sum Bootloader
 #
-#  Copyright (c) 2024, Pete Batard <pete@akeo.ie>
+#  Copyright (c) 2023, Pete Batard <pete@akeo.ie>
 #
 #  SPDX-License-Identifier: GPL-2.0-or-later
 #

Md5SumPkg.inf

@@ -34,11 +34,11 @@
   BaseLib
   DebugLib
   MemoryAllocationLib
+  PcdLib
   UefiApplicationEntryPoint
   UefiBootServicesTableLib
   UefiLib
   UefiRuntimeServicesTableLib
-  PcdLib
 
 [Guids]
   gEfiFileInfoGuid
@@ -57,4 +57,4 @@
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang
 
 [BuildOptions]
-  RELEASE_*_*_CC_FLAGS    = -Os -DMDEPKG_NDEBUG -DNDEBUG
+  RELEASE_*_*_CC_FLAGS    = -O2 -DMDEPKG_NDEBUG -DNDEBUG

boot.c

@@ -28,8 +28,11 @@
  */
 BOOLEAN IsTestMode = FALSE;
 
-/* We'll use this string to erase a full line on the console */
-CHAR16* EmptyLine = NULL;
+/* We'll use this string to erase a line on the console */
+STATIC CHAR16 EmptyLine[STRING_MAX];
+
+/* Keep a copy of the main Image Handle */
+STATIC EFI_HANDLE MainImageHandle = NULL;
 
 /* Strings used to identify the plaform */
 #if defined(_M_X64) || defined(__x86_64__)
@@ -56,14 +59,14 @@ STATIC VOID DisplayBanner(
 	UINTN i, Len;
 	CHAR16* Line;
 
-	// Don't display the banner in test mode
-	if (IsTestMode)
-		return;
-
 	// The platform logo may still be displayed → remove it
 	gST->ConOut->ClearScreen(gST->ConOut);
 
-	V_ASSERT(Cols > 79);
+	// Make sure the console is large enough to accomodate the banner
+	// but not too insanely large...
+	if (Cols < 50 || Cols >= STRING_MAX)
+		return;
+
 	Line = AllocatePool((Cols + 1) * sizeof(CHAR16));
 	if (Line == NULL)
 		return;
@@ -104,7 +107,7 @@ STATIC VOID DisplayBanner(
 	Print(L"%c", BOXDRAW_UP_RIGHT);
 	for (i = 0; i < Cols - 2; i++)
 		Print(L"%c", BOXDRAW_HORIZONTAL);
-	Print(L"%c", BOXDRAW_UP_LEFT);
+	Print(L"%c\n", BOXDRAW_UP_LEFT);
 	DefText();
 
 	FreePool(Line);
@@ -117,35 +120,34 @@ STATIC VOID DisplayBanner(
   @param[in]  Status     The Status code from the failed operation on the entry.
   @param[in]  Path       A pointer to the CHAR16 string with the Path of the entry.
   @param[in]  NumFailed  The current number of failed entries.
-
 **/
 STATIC VOID PrintFailedEntry(
 	IN CONST EFI_STATUS Status,
 	IN CHAR16* Path,
 	IN CONST UINTN NumFailed
 )
 {
+	if (!EFI_ERROR(Status) || Path == NULL)
+		return;
+
 	// Truncate the path in case it's very long.
 	// TODO: Ideally we'd want long path reduction similar to what Windows does.
 	if (SafeStrLen(Path) > 80)
 		Path[80] = L'\0';
-	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 4 +
-		(NumFailed % FAILED_ENTRIES_MAX));
-	if (EmptyLine != NULL && !IsTestMode)
-		Print(EmptyLine);
-	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 4 +
+	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 2 +
 		(NumFailed % FAILED_ENTRIES_MAX));
+	if (!IsTestMode) {
+		gST->ConOut->OutputString(gST->ConOut, EmptyLine);
+		SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 2 +
+			(NumFailed % FAILED_ENTRIES_MAX));
+	}
 	PrintError(L"File '%s'", Path);
 }
 
 /**
   Exit-specific processing for test/debug
-
-  @param[in]  NumFailed  The total number of failed entries.
 **/
-STATIC VOID ExitCheck(
-	IN INTN NumFailed
-)
+STATIC VOID ExitCheck(VOID)
 {
 #if defined(EFI_DEBUG)
 	UINTN Index;
@@ -157,8 +159,6 @@ STATIC VOID ExitCheck(
 
 	// If running debug, wait for a user keystroke and shut down
 #if defined(EFI_DEBUG)
-	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 5 +
-		MIN(NumFailed, FAILED_ENTRIES_MAX));
 	SetText(TEXT_YELLOW);
 	Print(L"Press any key to exit.\n");
 	DefText();
@@ -168,6 +168,42 @@ STATIC VOID ExitCheck(
 #endif
 }
 
+/**
+  Obtain the root handle of the current volume.
+
+  @param[out] Root              A pointer to the root file handle.
+
+  @retval EFI_SUCCESS           The root file handle was successfully populated.
+  @retval EFI_INVALID_PARAMETER The pointer to the root file handle is invalid.
+**/
+STATIC EFI_STATUS GetRootHandle(
+	OUT EFI_FILE_HANDLE* Root
+)
+{
+	EFI_STATUS Status;
+	EFI_LOADED_IMAGE_PROTOCOL* LoadedImage;
+	EFI_SIMPLE_FILE_SYSTEM_PROTOCOL* Volume;
+
+	if (Root == NULL)
+		return EFI_INVALID_PARAMETER;
+
+	// Access the loaded image so we can open the current volume
+	Status = gBS->OpenProtocol(MainImageHandle, &gEfiLoadedImageProtocolGuid,
+		(VOID**)&LoadedImage, MainImageHandle,
+		NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (EFI_ERROR(Status))
+		return Status;
+
+	// Open the the root directory on the boot volume
+	Status = gBS->OpenProtocol(LoadedImage->DeviceHandle,
+		&gEfiSimpleFileSystemProtocolGuid, (VOID**)&Volume,
+		MainImageHandle, NULL, EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL);
+	if (EFI_ERROR(Status))
+		return Status;
+
+	return Volume->OpenVolume(Volume, Root);
+}
+
 /*
  * Application entry-point
  * NB: This must be set to 'efi_main' for gnu-efi crt0 compatibility
@@ -178,8 +214,6 @@ EFI_STATUS EFIAPI efi_main(
 )
 {
 	EFI_STATUS Status;
-	EFI_LOADED_IMAGE_PROTOCOL* LoadedImage;
-	EFI_SIMPLE_FILE_SYSTEM_PROTOCOL* Volume;
 	EFI_FILE_HANDLE Root;
 	EFI_INPUT_KEY Key;
 	HASH_LIST HashList = { 0 };
@@ -188,6 +222,9 @@ EFI_STATUS EFIAPI efi_main(
 	UINT8 ComputedHash[MD5_HASHSIZE], ExpectedHash[MD5_HASHSIZE];
 	UINTN i, Index, Cols, Rows, NumFailed = 0;
 
+	// Keep a global copy of the bootloader's image handle
+	MainImageHandle = BaseImageHandle;
+
 #if defined(_GNU_EFI)
 	InitializeLib(BaseImageHandle, SystemTable);
 #endif
@@ -202,72 +239,56 @@ EFI_STATUS EFIAPI efi_main(
 		&Cols, &Rows);
 	if (EFI_ERROR(Status)) {
 		// Couldn't get the console dimensions
-		Cols = 80;
-		Rows = 40;
+		Cols = 60;
+		Rows = 20;
 	}
+	if (Cols >= STRING_MAX)
+		Cols = STRING_MAX - 1;
 
-	// Display the top banner
-	DisplayBanner(Cols);
-
-	// Create the blank line we'll use to erase a line
-	EmptyLine = AllocateZeroPool((Cols + 1) * sizeof(CHAR16));
-	if (EmptyLine != NULL) {
-		for (i = 0; i < Cols; i++)
-			EmptyLine[i] = L' ';
-	}
+	// Populate a blank line we can use to erase a line
+	for (i = 0; i < Cols; i++)
+		EmptyLine[i] = L' ';
+	EmptyLine[i] = L'\0';
 
-	// Access the loaded image so we can open the current volume
-	Status = gBS->OpenProtocol(BaseImageHandle, &gEfiLoadedImageProtocolGuid,
-		(VOID**)&LoadedImage, BaseImageHandle,
-		NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
-	if (EFI_ERROR(Status)) {
-		PrintError(L"Unable to access boot image interface");
-		goto out;
+	// Display the static user-facing text if not in test mode
+	if (!IsTestMode) {
+		// Clear the input buffer
+		gST->ConIn->Reset(gST->ConIn, FALSE);
+		// Display the top banner
+		DisplayBanner(Cols);
+		SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y);
+		Print(L"Media verification:\n");
+		SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 1);
 	}
 
-	// Open the the root directory on the boot volume
-	Status = gBS->OpenProtocol(LoadedImage->DeviceHandle,
-		&gEfiSimpleFileSystemProtocolGuid, (VOID**)&Volume,
-		BaseImageHandle, NULL, EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL);
-	if (EFI_ERROR(Status)) {
-		PrintError(L"Unable to open boot volume");
-		goto out;
-	}
-	Root = NULL;
-	Status = Volume->OpenVolume(Volume, &Root);
+	Status = GetRootHandle(&Root);
 	if (EFI_ERROR(Status)) {
-		PrintError(L"Unable to open root directory");
+		PrintError(L"Could not open root directory\n");
 		goto out;
 	}
 
 	// Parse md5sum.txt to construct a hash list
 	Status = Parse(Root, HASH_FILE, &HashList);
 	if (EFI_ERROR(Status))
 		goto out;
+	PluralFiles = (HashList.NumEntries == 1) ? L"" : L"s";
 
 	if (IsTestMode) {
 		// Print any extra data we want to validate
 		Print(L"[TEST] TotalBytes = 0x%lX\n", HashList.TotalBytes);
-	} else {
-		// Position our output near the center of the screen
-		SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y);
-		Print(L"Media verification - Press any key to cancel\n");
-		// Clear the input buffer
-		gST->ConIn->Reset(gST->ConIn, FALSE);
 	}
 
 	// Now go through each entry we parsed
-	PluralFiles = (HashList.Size == 1) ? L"" : L"s";
-	for (Index = 0; Index < HashList.Size; Index++) {
+	for (Index = 0; Index < HashList.NumEntries; Index++) {
 		// Check for user cancellation
 		if (gST->ConIn->ReadKeyStroke(gST->ConIn, &Key) != EFI_NOT_READY)
 			break;
 
 		// Report progress
 		if (!IsTestMode) {
-			SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 2);
+			SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 1);
 			Print(L"%d/%d file%s processed%s\n", Index,
-				HashList.Size, PluralFiles, NumFailedString);
+				HashList.NumEntries, PluralFiles, NumFailedString);
 		}
 
 		// Convert the expected hexascii hash to a binary value we can use
@@ -311,14 +332,17 @@ EFI_STATUS EFIAPI efi_main(
 	}
 
 	// Final progress report
-	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 2);
+	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 1);
 	Print(L"%d/%d file%s processed%s\n", Index,
-		HashList.Size, PluralFiles, NumFailedString);
+		HashList.NumEntries, PluralFiles, NumFailedString);
+
+	// Position subsequent console messages after our report
+	SetTextPosition(TEXT_POSITION_X, TEXT_POSITION_Y + 3 +
+		MIN(NumFailed, FAILED_ENTRIES_MAX));
 
 out:
 	SafeFree(HashList.Buffer);
-	SafeFree(EmptyLine);
-	ExitCheck(NumFailed);
+	ExitCheck();
 
 	return Status;
 }

boot.h

@@ -64,7 +64,7 @@ extern BOOLEAN              IsTestMode;
 #define HASH_FILE           L"md5sum.txt"
 
 /* Used to center our output on screen */
-#define TEXT_POSITION_X     0
+#define TEXT_POSITION_X     2
 #define TEXT_POSITION_Y     5
 
 /* Number of failed entries we display beneath the validation line before looping back */
@@ -186,7 +186,7 @@ typedef struct {
 /* Hash list of <Size> Hash entries */
 typedef struct {
 	HASH_ENTRY* Entry;
-	UINTN       Size;
+	UINTN       NumEntries;
 	UINT8*      Buffer;
 	UINT64      TotalBytes;
 } HASH_LIST;
@@ -242,7 +242,7 @@ STATIC __inline UINTN _SafeStrLen(CONST CHAR16 * String, CONST CHAR8 * File, CON
   @retval TRUE   A test system was detected.
   @retval FALSE  A regular plaform is being used.
 **/
-extern BOOLEAN IsTestSystem(VOID);
+BOOLEAN IsTestSystem(VOID);
 
 /**
   Parse a hash sum list file and populate a HASH_LIST structure from it.
@@ -258,7 +258,7 @@ extern BOOLEAN IsTestSystem(VOID);
   @retval EFI_END_OF_FILE       The hash list file could not be read.
   @retval EFI_ABORTED           The hash list file contains invalid data.
 **/
-extern EFI_STATUS Parse(IN CONST EFI_FILE_HANDLE Root, IN CONST CHAR16* Path, OUT HASH_LIST* List);
+EFI_STATUS Parse(IN CONST EFI_FILE_HANDLE Root, IN CONST CHAR16* Path, OUT HASH_LIST* List);
 
 /**
   Compute the MD5 hash of a single file.
@@ -273,7 +273,7 @@ extern EFI_STATUS Parse(IN CONST EFI_FILE_HANDLE Root, IN CONST CHAR16* Path, OU
   @retval EFI_OUT_OF_RESOURCES  A memory allocation error occurred.
   @retval EFI_NOT_FOUND         The target file could not be found on the media.
 **/
-extern EFI_STATUS HashFile(IN CONST EFI_FILE_HANDLE Root, IN CONST CHAR16* Path, OUT UINT8* Hash);
+EFI_STATUS HashFile(IN CONST EFI_FILE_HANDLE Root, IN CONST CHAR16* Path, OUT UINT8* Hash);
 
 /**
   Convert a UTF-8 encoded string to a UCS-2 encoded string.