Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nicholasbishop opened this issue Dec 17, 2022 · 2 comments · Fixed by #546
Closed

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

nicholasbishop opened this issue Dec 17, 2022 · 2 comments · Fixed by #546

Comments

@nicholasbishop
Copy link
Contributor

nicholasbishop commented Dec 17, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

#543 (comment) suggests importing fixes from EDK2, which sounds good to me.

nicholasbishop pushed a commit to nicholasbishop/shim that referenced this issue Jan 9, 2023
There is one long-standing problem in CRT realloc wrapper, which will
cause the obvious buffer overflow issue when re-allocating one bigger
memory block:
    void *realloc (void *ptr, size_t size)
    {
      //
      // BUG: hardcode OldSize == size! We have no any knowledge about
      // memory size of original pointer ptr.
      //
      return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
    }
This patch introduces one extra header to record the memory buffer size
information when allocating memory block from malloc routine, and re-wrap
the realloc() and free() routines to remove this BUG.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Validated-by: Jian J Wang <jian.j.wang@intel.com>

Cherry picked from https://github.com/tianocore/edk2.git, commit
cf8197a39d07179027455421a182598bd6989999. Changes:
* `SIGNATURE_32` -> `EFI_SIGNATURE_32`
* Added definition of `MIN`

Fixes rhboot#538

Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
vathpela pushed a commit that referenced this issue Jan 27, 2023
There is one long-standing problem in CRT realloc wrapper, which will
cause the obvious buffer overflow issue when re-allocating one bigger
memory block:
    void *realloc (void *ptr, size_t size)
    {
      //
      // BUG: hardcode OldSize == size! We have no any knowledge about
      // memory size of original pointer ptr.
      //
      return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
    }
This patch introduces one extra header to record the memory buffer size
information when allocating memory block from malloc routine, and re-wrap
the realloc() and free() routines to remove this BUG.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Validated-by: Jian J Wang <jian.j.wang@intel.com>

Cherry picked from https://github.com/tianocore/edk2.git, commit
cf8197a39d07179027455421a182598bd6989999. Changes:
* `SIGNATURE_32` -> `EFI_SIGNATURE_32`
* Added definition of `MIN`

Fixes #538

Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
brianredbeard pushed a commit to brianredbeard/redhat-efi-boot-shim that referenced this issue Feb 22, 2024
There is one long-standing problem in CRT realloc wrapper, which will
cause the obvious buffer overflow issue when re-allocating one bigger
memory block:
    void *realloc (void *ptr, size_t size)
    {
      //
      // BUG: hardcode OldSize == size! We have no any knowledge about
      // memory size of original pointer ptr.
      //
      return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
    }
This patch introduces one extra header to record the memory buffer size
information when allocating memory block from malloc routine, and re-wrap
the realloc() and free() routines to remove this BUG.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Validated-by: Jian J Wang <jian.j.wang@intel.com>

Cherry picked from https://github.com/tianocore/edk2.git, commit
cf8197a39d07179027455421a182598bd6989999. Changes:
* `SIGNATURE_32` -> `EFI_SIGNATURE_32`
* Added definition of `MIN`

Fixes rhboot#538

Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants