[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
make MDMP use the new base relocation
add COFF relocations for object files

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


XVilka · 2024-11-12T02:42:24Z

librz/bin/format/pe/pe.h

+	ut32 block_size;
+} RzBinPeRelocBlock;
+
+typedef struct rz_bin_pe_reloc_ent_t {


Bitfields aren't portable. Please use masks instead. It's ok for keeping them in definition, but they should not be used for reading. It's especially important for reading files of different endianess from the host Rizin compiled for.

use the le/be methods from rz_util/rz_buf.h

use the le/be methods from rz_util/rz_buf.h

I've used ble. Is there something I did wrong?

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

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

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

Roeegg2 commented Nov 11, 2024

XVilka Nov 11, 2024

XVilka commented Nov 11, 2024

XVilka Nov 12, 2024

XVilka Nov 12, 2024

XVilka Nov 12, 2024

wargio Nov 12, 2024

Roeegg2 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

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

[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

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

Choose a reason for hiding this comment

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

wargio Nov 13, 2024 •

edited

Loading