[WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! #4711

Roeegg2 · 2024-11-11T14:55:51Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the rizin book with the relevant information (if needed)

Detailed description

Up until now PE relocations were implemented as imports, which is incorrect. The PE format uses base relocations for image files (ie executable files) and classic COFF relocations for object files.

Implemented so far:

PE base relocation parsing + assigning
PE base relocation tests

Still need to implement:

~~relocation patching~~ - Rizin always loads the binary in it's expected base address, so no patching is needed (I added a detailed comment about this in relocs function in bin_pe.inc)
~~make MDMP use the new base relocation~~ MDMP doesn't have relocations, only imports.
~~add COFF relocations for object files~~ technically PE can have COFF relocations since it's based off of COFF, but COFF relocations are only used for for object files, and there is no reason to use PE for object files, so it's never actually used.

Notes:

There are no names printed with base relocations, since they don't have a symbol/name field associated with them. Simply a type and offset to apply the relocation in.
...

Test plan

All tests green, except the MDMP & and the ones dependent of relocation patching.

...

Closing issues

...

* Add missing `PE_IMAGE_FILE_MACHINE` types * Add PE base relocation types Everything is based off of Microsoft's PE format documentation [found here](https://learn.microsoft.com/en-us/windows/win32/debug/pe-format)

Roeegg2 · 2024-11-11T14:56:06Z

Some things I wasn't sure about:

The comment in has_canary seems to be from r2, and I don't see what leaks might be caused from this...
Should I add licensing to the new created files?
I deliberately didn't add COFF base relocations yet, since we could use the COFF format code for this. There is actually quite a handful of code we could use from COFF plugin instead of completely re implementing it for PE. It might be better to add that to a separate issue & PR since this seems like an issue on its own. This might take a lot of work though since it would require changing a lot of the internal structures used by PE plugin, as well as re implementing handling of many things in COFF to make it compatible with PE as well.

librz/bin/format/pe/pe_relocs.c

librz/bin/format/pe/pe64_relocs.c

librz/bin/format/pe/pe_relocs.c

librz/bin/p/bin_pe.inc

XVilka · 2024-11-11T15:29:24Z

librz/bin/p/bin_pe.inc

@@ -554,30 +638,16 @@ err:
 }

 static int has_canary(RzBinFile *bf) {
-	// XXX: We only need imports here but this causes leaks, we need to wait for the below. This is a horrible solution!
 	// TODO: use O(1) when imports sdbized


Remove this comment too

XVilka · 2024-11-11T15:31:56Z

I deliberately didn't add COFF base relocations yet, since we could use the COFF format code for this. There is actually quite a handful of code we could use from COFF plugin instead of completely re implementing it for PE. It might be better to add that to a separate issue & PR since this seems like an issue on its own. This might take a lot of work though since it would require changing a lot of the internal structures used by PE plugin, as well as re implementing handling of many things in COFF to make it compatible with PE as well.

For now, I think some code duplication is fine. Sharing the code between two plugins is non-trivial at this point.

XVilka · 2024-11-12T02:40:51Z

librz/bin/format/pe/pe_relocs.c

@@ -0,0 +1,74 @@
+// SPDX-FileCopyrightText: 2024 Roee Toledano <roeetoledano10@gmail.com>
+// SPDX-License-Identifier: LGPL-3.0-only
+#include "pe.h"


Please leave 1 empty line between SPDX headers and first #include

XVilka · 2024-11-12T02:41:03Z

librz/bin/format/pe/pe64_relocs.c

@@ -0,0 +1,4 @@
+// SPDX-FileCopyrightText: 2024 Roee Toledano <roeetoledano10@gmail.com>
+// SPDX-License-Identifier: LGPL-3.0-only
+#define RZ_BIN_PE64 1


librz/bin/format/pe/pe.h

wargio

This needs to be tested via distro- branch before merge. @XVilka

wargio · 2024-11-13T05:51:21Z

librz/bin/format/pe/pe_relocs.c

+		rz_buf_read(b, ent_buf, sizeof(ent_buf));
+		reloc = rz_read_ble16(ent_buf, big_endian);


maybe use the rz_buf_read_ble16_offset which is a special api which updates automatically the offset/address parameter and returns true on successful read.

also be sure to check the boolean return value to fail gracefully.

wargio · 2024-11-13T05:53:57Z

librz/bin/format/pe/pe_relocs.c

+		rz_buf_read(b, block_buf, sizeof(block_buf));
+		RzBinPeRelocBlock block;
+		PE_READ_STRUCT_FIELD_L(block_buf, block, RzBinPeRelocBlock, page_rva, 32);
+		PE_READ_STRUCT_FIELD_L(block_buf, block, RzBinPeRelocBlock, block_size, 32);


you can do the same here using rz_buf_read_bleXX_offset and checking the return value.
you can do this in a static function like this and fail when returns false.

wargio · 2024-11-13T05:55:13Z

librz/bin/format/pe/pe_relocs.c

+
+	do {
+		bytes_read += 2;
+		RzBinPeRelocEnt reloc;


always init this to 0.

Suggested change

RzBinPeRelocEnt reloc;

RzBinPeRelocEnt reloc = 0;

wargio · 2024-11-13T05:55:48Z

librz/bin/format/pe/pe_relocs.c

+	do {
+		ut8 block_buf[sizeof(RzBinPeRelocBlock)];
+		rz_buf_read(b, block_buf, sizeof(block_buf));
+		RzBinPeRelocBlock block;


Suggested change

RzBinPeRelocBlock block;

RzBinPeRelocBlock block = { 0 };

wargio

Please always initialize the variables to 0 or NULL so we can avoid random crashes and weird unintentional behavior.

* Remove old code of setting import entries to PE relocations * Parse PE base relocations * Implement converting from the specific RzBinPeRelocEnt type to the general RzBinReloc type * Implement corrent type naming of PE relocations

* Removed a.exe and b.exe tests, as they don't contain base relocations and swapped them with good tests * Corrected the olds tests 1. Need to add MDMP 2. Need to implement PE relocs patching to get the other tests to work

wargio · 2024-11-14T04:21:44Z

librz/bin/format/pe/pe.h

+// encoding is 12 bits for type and 4 bits for offset. 
+// See: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-reloc-section-image-only
+#define PE_RELOC_ENT_TYPE(val)   ((val) >> 12)
+#define PE_RELOC_ENT_OFFSET(val) ((val)&0b111111111111)


i'm not sure how 0b111111111111 is portable. use hex (0xfff).

You're right, totally forgot about that. I'll fix it

* Fix error caused when there are no relocations. Solved by adding a function checking whether relocation vector is empty or not

* Added freeing relocation vector and fixed correct setting of bin->relocs in case there has been an error

Roeegg2 · 2024-11-16T20:04:34Z

Sorry for the ugly commit history. I could rebase interactive + squash with the old ones so it's easier to review.

Roeegg2 · 2024-11-16T20:14:26Z

All tests are green except flagspace tests. Because there is no symbol associated with base relocations, they have no name associated with them, so I'm not sure how we can use flags here

librz/bin/p/bin_pe.inc

librz/bin/format/pe/pe.h

wargio · 2024-11-19T04:47:29Z

librz/bin/p/bin_pe.inc

+	rz_vector_foreach (bin->relocs, pe_reloc) {
+		RzBinReloc *general_reloc = RZ_NEW0(RzBinReloc);
+		if (!general_reloc) {
+			RZ_LOG_WARN("Failed to convert PE reloc to general reloc\n");


Suggested change

RZ_LOG_WARN("Failed to convert PE reloc to general reloc\n");

RZ_LOG_ERROR("Failed to allocate PE RzBinReloc\n");

wargio · 2024-11-19T04:48:40Z

librz/bin/p/bin_pe.inc

+		if (!general_reloc) {
+			RZ_LOG_WARN("Failed to convert PE reloc to general reloc\n");
+			rz_bin_reloc_free(general_reloc);
+			continue;


if alloc fails, then there is no memory left.
You should probably do the following instead of calling continue:

rz_vector_free(bin->relocs); return ret;

Add missing PE relocation format specs

166bcc8

* Add missing `PE_IMAGE_FILE_MACHINE` types * Add PE base relocation types Everything is based off of Microsoft's PE format documentation [found here](https://learn.microsoft.com/en-us/windows/win32/debug/pe-format)

Roeegg2 requested review from ret2libc, wargio, XVilka, kazarmy and thestr4ng3r as code owners November 11, 2024 14:55

github-actions bot added rz-test RzBin PE labels Nov 11, 2024

XVilka requested changes Nov 11, 2024

View reviewed changes

XVilka changed the title ~~Implement PE & MDMP base relocations. DO NOT MERGE YET!~~ [WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! Nov 11, 2024

Roeegg2 force-pushed the pe-reloc branch 2 times, most recently from 64455b5 to 7cac944 Compare November 11, 2024 16:52

wargio marked this pull request as draft November 12, 2024 02:29

XVilka requested changes Nov 12, 2024

View reviewed changes

wargio requested changes Nov 12, 2024

View reviewed changes

Roeegg2 force-pushed the pe-reloc branch from 7cac944 to b716edf Compare November 12, 2024 17:32

Roeegg2 added 2 commits November 13, 2024 01:39

Remove MDMP relocations

c31b234

Remove MDMP relocation tests

8ea009b

wargio reviewed Nov 13, 2024

View reviewed changes

wargio requested changes Nov 13, 2024

View reviewed changes

Roeegg2 added 4 commits November 13, 2024 19:36

Implement PE base relocations

2012869

* Remove old code of setting import entries to PE relocations * Parse PE base relocations * Implement converting from the specific RzBinPeRelocEnt type to the general RzBinReloc type * Implement corrent type naming of PE relocations

Clear PE format 'has_canary' function

f3cc97e

Remove 'RzBinPEObj.endian' and use 'RzBinPEObj.big_endian' instead

b9a2881

Update PE relocation tests

0040b1d

* Removed a.exe and b.exe tests, as they don't contain base relocations and swapped them with good tests * Corrected the olds tests 1. Need to add MDMP 2. Need to implement PE relocs patching to get the other tests to work

Roeegg2 force-pushed the pe-reloc branch from b716edf to 0040b1d Compare November 13, 2024 17:36

wargio reviewed Nov 14, 2024

View reviewed changes

Roeegg2 added 5 commits November 14, 2024 13:35

Correct formatting + PE_RELOC_ENT_OFFSET

b1900ab

Fix PE relocations no relocs error

2926b2e

* Fix error caused when there are no relocations. Solved by adding a function checking whether relocation vector is empty or not

Correct PE relocations implementation

3e45490

Correct PE relocation tests

2cdf8a6

Fix relocs allocation error

470c75a

* Added freeing relocation vector and fixed correct setting of bin->relocs in case there has been an error

wargio reviewed Nov 17, 2024

View reviewed changes

librz/bin/p/bin_pe.inc Outdated Show resolved Hide resolved

wargio reviewed Nov 17, 2024

View reviewed changes

librz/bin/p/bin_pe.inc Outdated Show resolved Hide resolved

wargio reviewed Nov 17, 2024

View reviewed changes

librz/bin/format/pe/pe.h Show resolved Hide resolved

Invert and change if

66c1e4a

wargio reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! #4711

[WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! #4711

Roeegg2 commented Nov 11, 2024 •

edited

Loading

Roeegg2 commented Nov 11, 2024

XVilka Nov 11, 2024

XVilka commented Nov 11, 2024

XVilka Nov 12, 2024

XVilka Nov 12, 2024

wargio left a comment

wargio Nov 13, 2024

wargio Nov 13, 2024

wargio Nov 13, 2024

wargio Nov 13, 2024 •

edited

Loading

wargio Nov 13, 2024

wargio left a comment

wargio Nov 14, 2024

Roeegg2 Nov 14, 2024

Roeegg2 commented Nov 16, 2024

Roeegg2 commented Nov 16, 2024

wargio Nov 19, 2024

wargio Nov 19, 2024

		rz_buf_read(b, ent_buf, sizeof(ent_buf));
		reloc = rz_read_ble16(ent_buf, big_endian);

	RZ_LOG_WARN("Failed to convert PE reloc to general reloc\n");
	RZ_LOG_ERROR("Failed to allocate PE RzBinReloc\n");

[WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! #4711

Are you sure you want to change the base?

[WIP] Implement PE & MDMP base relocations. DO NOT MERGE YET! #4711

Conversation

Roeegg2 commented Nov 11, 2024 • edited Loading

Roeegg2 commented Nov 11, 2024

Choose a reason for hiding this comment

XVilka commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roeegg2 commented Nov 16, 2024

Roeegg2 commented Nov 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roeegg2 commented Nov 11, 2024 •

edited

Loading

wargio Nov 13, 2024 •

edited

Loading