rhboot / shim

UEFI shim loader
Other
857 stars 292 forks source link

`realloc` copies incorrect amount of memory, fails running under Mu firmware #538

Closed nicholasbishop closed 1 year ago

nicholasbishop commented 1 year ago

realloc is implemented here: https://github.com/rhboot/shim/blob/main/Cryptlib/SysCall/BaseMemAllocation.c#L29

It has a comment already pointing out the problem:

 // BUG: hardcode OldSize == size! We have no any knowledge about
 // memory size of original pointer ptr.

As currently implemented, it passes the desired size to ReallocatePool for both the old and new size. This happens to work fine usually, but I found when trying to test NX protection using https://github.com/microsoft/mu_tiano_platforms it reliably faults there.

I tried a very quick & hacky fix by statically keeping track of allocations in a fixed-size buffer so that realloc knows the allocation size, and that seemed to do the trick. As far as I can tell openssl requires a functioning realloc, so I think some variation of manually tracking allocations will be needed since UEFI doesn't provide such an interface. I don't have a PR for this yet, but figured I'd go ahead and file an issue for awareness.

dennis-tseng99 commented 1 year ago

I tried a very quick & hacky fix by statically keeping track of allocations in a fixed-size buffer

Yes. Because ReallocatePool() will copy your old buffer content to its new allocated buffer, the caller of realloc(void *ptr, size_t size) must prepare a fixed buffer by calling AllocatePool(size) with at least size bytes. Otherwise, some unknown garbage bytes will be copied to its new buffer. Many thanks.

nicholasbishop commented 1 year ago

https://github.com/rhboot/shim/pull/543#issuecomment-1374861905 suggests importing fixes from EDK2, which sounds good to me.