MdeModulePkg: Remove ASSERT which prevents VOLUME_CORRUPTED image status from being returned

[mikebeaton]

When DEBUG_RAISE is disabled then PeCoffInitializeContext within BasePeCoffLib2 will return VOLUME_CORRUPTED for certain image errors. These errors then hit the ASSERT removed by this PR. Without it these status codes pass back out of gBS->LoadImage in DEBUG builds, as desired.

[mikebeaton]

On reflection, there are several other recently added ASSERT (FALSE) statements in this file which should probably be changed into DEBUG_RAISE, for the same reason as the change of Pcd on DEBUG_RAISE which was just made: these are valid responses of the code to errors in images, not internal errors, so we only want them to ASSERT in specific debugging scenarios, not just generally (even in DEBUG builds).

[MikhailKrichanov]

Desired by whom? I hope in the near future uefi-format branch will be merged and the behavior will change to the following 7d97eab#diff-7716cd945ce4a34439a613ef76374c82b389ac12470368e183138d48df361e73R1246

[mikebeaton]

Desired by anyone who isn't debugging the internals of that particular module, but wants to be able to use a DEBUG or NOOPT build (e.g. so as to be able to step between external code and the library), only expecting the library code to stop and assert if it detects it's own unexpected internal state, not on valid responses to input (particularly, input - such as corrupt or invalid images - which the calling code might itself want to be testing it's own handling for - that's a specific case of what I take to be a general principle).

[mikebeaton]

The commit you've referenced replaces the ASSERT with cpu_dead_loop and has a comment stating that the code in question should be unreachable. It's not unreachable, the new PeCoffInit correctly returns error statuses on corrupt images, which then get back to here (and should be allowed to percolate back out to the code which called gBS->LoadImage). (Exactly what this PR is trying to fix.)

The separated 32 bit and 64 bit slices of 10.4 in here are examples of such images: https://github.com/acidanthera/OpenCorePkg/tree/master/Utilities/AppleEfiSignTool/Samples

[vit9696]

@MikhailKrichanov, the code in this pull request looks correct to me. Why should we freeze firmware upon trying to load a corrupted image from userspace? We should simply reject.

• "ASSERTs" should become CpuDeadLoop when they were used by EDK II maintainers as a way to stop booting.
• They should become DEBUG_RAISE or simply removed when they were used as a way to help debugging.

[vit9696]

Just to be clear:

• Images with violated requirements loaded from the firmware (i.e. BIOS) should indeed cause a dead loop, as it means we have corrupted firmware.
• Images that come from the user should simply gracefully abort, as the user may supply anything and we should just reject without killing everything.

[mikebeaton]

@MikhailKrichanov - Looking at the current uefi-format branch, I can see that the code should indeed exit cleanly - i.e. not hang - if the exit status of UefiImageInitializeContextPreHash is EFI_NOT_STARTED: https://github.com/acidanthera/audk/blob/uefi-format/MdeModulePkg/Core/Dxe/Image/Image.c#L1234-L1248 . But looking at the code for UefiImageInitializeContextPreHash
it appears to be written to return either EFI_SUCCESS or EFI_UNSUPPORTED: https://github.com/acidanthera/audk/blob/uefi-format/MdePkg/Library/BaseUefiImageLib/UefiImageLib.c#L114 . If I'm right, that may be a mistake?

If I'm right that that is the wrong status, then when fixed code here will already do what I'm aiming at, thank you.

[MikhailKrichanov]

@vit9696, the requirements are implemented in the latest fix.