From b6b6c9c688b1f72a81b2e1efbd2d18b000c14669 Mon Sep 17 00:00:00 2001 From: kenlautner <85201046+kenlautner@users.noreply.github.com> Date: Fri, 14 Jul 2023 08:32:35 -0700 Subject: [PATCH] Fix CodeQL errors (#273) ## Description Fixed some CodeQL failures we're seeing in a variety of packages. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Tested through CodeQL checks. ## Integration Instructions N/A --- HidPkg/HidKeyboardDxe/HidKeyboard.c | 11 ++- .../MsWheaReport/MsWheaEarlyStorageMgr.c | 14 +++- .../MsBootOptionsLib/MsBootOptionsLib.c | 72 +++++++++++++++---- 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/HidPkg/HidKeyboardDxe/HidKeyboard.c b/HidPkg/HidKeyboardDxe/HidKeyboard.c index 190f8dfc..eee3bc6e 100644 --- a/HidPkg/HidKeyboardDxe/HidKeyboard.c +++ b/HidPkg/HidKeyboardDxe/HidKeyboard.c @@ -666,7 +666,16 @@ SetKeyboardLayoutEvent ( // TableEntry = GetKeyDescriptor (HidKeyboardDevice, 0x58); KeyDescriptor = GetKeyDescriptor (HidKeyboardDevice, 0x28); - CopyMem (TableEntry, KeyDescriptor, sizeof (EFI_KEY_DESCRIPTOR)); + + // MU_CHANGE [BEGIN] - CodeQL change + if ((TableEntry == NULL) || (KeyDescriptor == NULL)) { + ASSERT (TableEntry != NULL); + ASSERT (KeyDescriptor != NULL); + } else { + CopyMem (TableEntry, KeyDescriptor, sizeof (EFI_KEY_DESCRIPTOR)); + } + + // MU_CHANGE [END] - CodeQL change FreePool (KeyboardLayout); } diff --git a/MsWheaPkg/MsWheaReport/MsWheaEarlyStorageMgr.c b/MsWheaPkg/MsWheaReport/MsWheaEarlyStorageMgr.c index f6022862..4b948702 100644 --- a/MsWheaPkg/MsWheaReport/MsWheaEarlyStorageMgr.c +++ b/MsWheaPkg/MsWheaReport/MsWheaEarlyStorageMgr.c @@ -224,12 +224,24 @@ MsWheaESContentChangeChecksumHelper ( { UINT16 Sum16; MS_WHEA_EARLY_STORAGE_HEADER Header; + // MU_CHANGE [BEGIN] - CodeQL change + EFI_STATUS Status; + + // MU_CHANGE [END] - CodeQL change DEBUG ((DEBUG_INFO, "Calculate sum content helper...\n")); MsWheaESReadHeader (&Header); Header.ActiveRange += (UINT32)Length; - MsWheaESCalculateChecksum16 (&Header, &Sum16); + Status = MsWheaESCalculateChecksum16 (&Header, &Sum16); + // MU_CHANGE [BEGIN] - CodeQL change + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "WHEA Early Storage Checksum calculation failed - %r...", Status)); + ASSERT_EFI_ERROR (Status); + return; + } + + // MU_CHANGE [END] - CodeQL change Header.Checksum = Sum16; MsWheaESWriteHeader (&Header); } diff --git a/PcBdsPkg/Library/MsBootOptionsLib/MsBootOptionsLib.c b/PcBdsPkg/Library/MsBootOptionsLib/MsBootOptionsLib.c index 05b9dd5d..2c8ee559 100644 --- a/PcBdsPkg/Library/MsBootOptionsLib/MsBootOptionsLib.c +++ b/PcBdsPkg/Library/MsBootOptionsLib/MsBootOptionsLib.c @@ -118,7 +118,13 @@ BuildFwLoadOption ( DevicePathFromHandle (LoadedImage->DeviceHandle), (EFI_DEVICE_PATH_PROTOCOL *)&FileNode ); - ASSERT (DevicePath != NULL); + // MU_CHANGE [BEGIN] - CodeQL change + if (DevicePath == NULL) { + ASSERT (DevicePath != NULL); + return EFI_NOT_FOUND; + } + + // MU_CHANGE [END] - CodeQL change Status = EfiBootManagerInitializeLoadOption ( BootOption, @@ -329,6 +335,10 @@ CreateFvBootOption ( DevicePathFromHandle (LoadedImage->DeviceHandle), (EFI_DEVICE_PATH_PROTOCOL *)&FileNode ); + if (DevicePath == NULL) { + ASSERT (DevicePath != NULL); + return EFI_OUT_OF_RESOURCES; + } } else { if (IsZeroGuid (PcdGetPtr (PcdShellFvGuid))) { // Search all FV's for Shell. @@ -350,6 +360,10 @@ CreateFvBootOption ( (EFI_DEVICE_PATH_PROTOCOL *)DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&FileNode ); + if (DevicePath == NULL) { + ASSERT (DevicePath != NULL); + return EFI_OUT_OF_RESOURCES; + } } Status = EfiBootManagerInitializeLoadOption ( @@ -373,14 +387,17 @@ CreateFvBootOption ( /** * Register a boot option * - * @param FileGuid - * @param Description - * @param Position - * @param Attributes - * @param OptionalData - * @param OptionalDataSize + * @param FileGuid The guid associated with the boot option. + * @param Description Description of the boot option. + * @param Position The position of the load option to put in the ****Order variable. + * @param Attributes The attributes of the boot option. + * @param OptionalData Optional data of the boot option. + * @param OptionalDataSize Size of the optional data of the boot option. * - * @return UINTN + * @return UINTN If a value is returned that is smaller + * than MAX_UINTN we registered successfully. + * @return MAX_UINTN We were unable to get Load Options and failed + * to register the boot option. */ static UINTN @@ -405,6 +422,11 @@ RegisterFvBootOption ( if (!EFI_ERROR (Status)) { BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOptionTypeBoot); + if (BootOptions == NULL) { + ASSERT (BootOptions != NULL); + return MAX_UINTN; + } + OptionIndex = EfiBootManagerFindLoadOption (&NewOption, BootOptions, BootOptionCount); if (OptionIndex == -1) { NewOption.Attributes ^= LOAD_OPTION_ACTIVE; @@ -428,6 +450,12 @@ RegisterFvBootOption ( // ensure the boot option for INTERNAL SHELL is deleted. if (0 == StrCmp (INTERNAL_UEFI_SHELL_NAME, Description)) { BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOptionTypeBoot); + + if (BootOptions == NULL) { + ASSERT (BootOptions != NULL); + return MAX_UINTN; + } + for (i = 0; i < BootOptionCount; i++) { if (0 == StrCmp (INTERNAL_UEFI_SHELL_NAME, BootOptions[i].Description)) { EfiBootManagerDeleteLoadOptionVariable (BootOptions[i].OptionNumber, LoadOptionTypeBoot); @@ -456,11 +484,31 @@ MsBootOptionsLibRegisterDefaultBootOptions ( ) { DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); + UINTN BootOption; - RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_SDD_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_SDD_BOOT_PARM, sizeof (MS_SDD_BOOT_PARM)); - RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_USB_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_USB_BOOT_PARM, sizeof (MS_USB_BOOT_PARM)); - RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_PXE_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_PXE_BOOT_PARM, sizeof (MS_PXE_BOOT_PARM)); - RegisterFvBootOption (PcdGetPtr (PcdShellFile), INTERNAL_UEFI_SHELL_NAME, (UINTN)-1, LOAD_OPTION_ACTIVE, NULL, 0); + BootOption = RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_SDD_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_SDD_BOOT_PARM, sizeof (MS_SDD_BOOT_PARM)); + if (BootOption == MAX_UINTN) { + DEBUG ((DEBUG_ERROR, "Failed to register Boot Option. Description: %s\n", MS_SDD_BOOT)); + ASSERT (BootOption != MAX_UINTN); + } + + BootOption = RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_USB_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_USB_BOOT_PARM, sizeof (MS_USB_BOOT_PARM)); + if (BootOption == MAX_UINTN) { + DEBUG ((DEBUG_ERROR, "Failed to register Boot Option. Description: %s\n", MS_USB_BOOT)); + ASSERT (BootOption != MAX_UINTN); + } + + BootOption = RegisterFvBootOption (&gMsBootPolicyFileGuid, MS_PXE_BOOT, (UINTN)-1, LOAD_OPTION_ACTIVE, (UINT8 *)MS_PXE_BOOT_PARM, sizeof (MS_PXE_BOOT_PARM)); + if (BootOption == MAX_UINTN) { + DEBUG ((DEBUG_ERROR, "Failed to register Boot Option. Description: %s\n", MS_PXE_BOOT)); + ASSERT (BootOption != MAX_UINTN); + } + + BootOption = RegisterFvBootOption (PcdGetPtr (PcdShellFile), INTERNAL_UEFI_SHELL_NAME, (UINTN)-1, LOAD_OPTION_ACTIVE, NULL, 0); + if (BootOption == MAX_UINTN) { + DEBUG ((DEBUG_ERROR, "Failed to register Boot Option. Description: %s\n", INTERNAL_UEFI_SHELL_NAME)); + ASSERT (BootOption != MAX_UINTN); + } } /**