It would be very helpful to have, at the very least, logs here to indicate these calls can fail, as it has a great chance to prevent booting if the calls do fail. For example, shim was failing to boot and causing a synchronous exception on an ARM platform because the shim Ubuntu was using had the Memory Attributes Protocol commit, but not commit https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6, so shim was setting NX for the entire image of fbaa64.efi, then all calls setting individual code sections to executable were silently failing due to the size not being 4k aligned. fbaa64.efi would then attempt to execute code marked NX and crash the system.
Failures to set or clear memory attributes can be fatal to a system, so the expectation would at least be for shim to log these errors. Ideally, it could also attempt recovery from failures (although it may not be able to). Or change its paradigm to not set the entire image region as NX first, but instead go section by section updating.
An example of how the logging could look in pe.c:
if (uefi_set_attrs) {
efi_status = proto->SetMemoryAttributes(proto, physaddr, size, uefi_set_attrs);
if (EFI_ERROR(efi_status) {
dprint("Failed to set memory attributes on physaddr: 0x%llx size: 0x%llx, status: 0x%lx", physaddr, size, status);
}
if (!EFI_ERROR(efi_status) && uefi_clear_attrs) {
efi_status = proto->ClearMemoryAttributes(proto, physaddr, size, uefi_clear_attrs);
if (EFI_ERROR(efi_status) {
dprint("Failed to clear memory attributes on physaddr: 0x%llx size: 0x%llx, status: 0x%lx", physaddr, size, status);
}
}
}
When shim make SetMemoryAttributes and ClearMemoryAttributes calls, it does not check the return status of these calls (other than to not clear memory if set failed) nor does it log any information about it: https://github.com/rhboot/shim/blob/main/pe.c#L993-L997 and https://github.com/rhboot/shim/blob/main/pe.c#LL1195C2-L1195C17.
It would be very helpful to have, at the very least, logs here to indicate these calls can fail, as it has a great chance to prevent booting if the calls do fail. For example, shim was failing to boot and causing a synchronous exception on an ARM platform because the shim Ubuntu was using had the Memory Attributes Protocol commit, but not commit https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6, so shim was setting NX for the entire image of fbaa64.efi, then all calls setting individual code sections to executable were silently failing due to the size not being 4k aligned. fbaa64.efi would then attempt to execute code marked NX and crash the system.
Failures to set or clear memory attributes can be fatal to a system, so the expectation would at least be for shim to log these errors. Ideally, it could also attempt recovery from failures (although it may not be able to). Or change its paradigm to not set the entire image region as NX first, but instead go section by section updating.
An example of how the logging could look in pe.c: