PE: Add parser for dynamic relocation (DVRT) #489
Conversation
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
This looks pretty good, with some nits; this is a large amount of code though! We should also begin to be careful putting Debug on all structs, as this can lead to an increase in compile times, fwiw. Unavoidable if something higher up requires it.
I'm curious, are portions of this ai generated? If yes, that's fine, I'm just wondering how we can better test that some of the e.g., bit masking logic and other details that I wouldn't have the semantic background to know is correct, is correct.
| let size_baserel = bytes.gread_with::<u32>(&mut offset, scroll::LE)?; | ||
| let bytes = bytes.gread_with::<&'a [u8]>(&mut offset, size_baserel as usize)?; |
There was a problem hiding this comment.
this pattern is so common, len,array I am idly wondering if it should be abstracted, e.g.
let arr = bytes.gread::<LArray>(&mut offset)?;
let bytes = arr.0;It could even be worth a breaking change in scroll for &[u8] ctx to have something like:
let arr: &[u8] = bytes.gread_with(&mut offset, SliceCtx::PrefixLen(Endian::LE))?;similar to StrCtx or something. Although not clear how the u32/u16/whatever len gets communicated in the ctx.
anyway just thinking out loud
There was a problem hiding this comment.
Actually it just occured to me we could use a tuple for this too in theory.
|
|
||
| if self.symbol == IMAGE_DYNAMIC_RELOCATION_FUNCTION_OVERRIDE { | ||
| self.bytes = &[]; | ||
| return None; // Unimplemented |
| Ok(x) => Ok(DynRelocBlock { | ||
| symbol: self.symbol, | ||
| rva, | ||
| bytes: &x[8..], // Skip sizeof(rva) + sizeof(size) |
There was a problem hiding this comment.
in general try to avoid magic constants like this, even sizeof_val(rva) + sizeof_val(size) is better imho
| u8::from(((self.0 >> 12) & 0x3) as u8) | ||
| } | ||
|
|
||
| /// Sets the Type field (bits 12-13) |
There was a problem hiding this comment.
doc comment same as above, intended?
| /// Sets the Type field (bits 12-13) | ||
| pub fn set_fixup_type(&mut self, fixup_type: u8) { | ||
| let type_bits = (fixup_type as u16) & 0x3; | ||
| self.0 = (self.0 & !(0x3 << 12)) | (type_bits << 12); |
There was a problem hiding this comment.
this is very hard to read, but all bit masking logic generally is :P
| } | ||
|
|
||
| /// Sets the CfgCheck field (bit 14) | ||
| pub fn set_cfg_check(&mut self, cfg_check: bool) { |
There was a problem hiding this comment.
I might opt for the builder pattern here, but I don't know how usage patterns are generally, do you normally mutate it a few times occasionally in different situations or is it generally constructed with these flags once? If latter I'd suggest to use builder pattern since it's more convenient. If it's mutated in different situations/conditions, set style is probably better
| } | ||
| IMAGE_DYNAMIC_RELOCATION_FUNCTION_OVERRIDE => { | ||
| // This is unreachable. See impl Iterator for DynRelocBlockIterator. | ||
| unimplemented!() |
There was a problem hiding this comment.
you're 100% sure this is not reachable? To be clear, I mean even with malformed, fuzzer generated binaries etc., this should truly be impossible to reach.
|
this probably needs a rebase/my comments still need some clarification. we can focus on the other outstanding PR for delay import parser in the meantime though :) |
Ok, I will try to strip Debug derives as possible as it can. It helps me easily debug while writing code and perhaps we should impl Display instead..? Not really generated by AI. AI sucks for this kind of work and is very trivial to mistake. I think I'd use it for doc comments and some trivial tests. It would definitely do better when it comes to generating tests for bit masking helpers. |
2 participants
This PR adds support for dynamic value relocation table (DVRT) for PE32/64. DVRT info is provided in load config directory. Generally speaking, DVRT is a feature to mitigate Spectre variant 2 (branch target injection), for example, by dynamically relocating indirect jumps or calls to ”retpoline” pages. DVRT contains where such indirect call targets and jumps are located within the executable.
There are 8 types of dynamic relocation as of now, which this PR adds 5 of them. DVRT has 2 versions as of now (
1and2) and this PR currently only supports1.IMAGE_DYNAMIC_RELOCATION_GUARD_RF_PROLOGUEIMAGE_DYNAMIC_RELOCATION_GUARD_RF_EPILOGUEIMAGE_DYNAMIC_RELOCATION_GUARD_IMPORT_CONTROL_TRANSFERIMAGE_DYNAMIC_RELOCATION_GUARD_INDIR_CONTROL_TRANSFERIMAGE_DYNAMIC_RELOCATION_GUARD_SWITCHTABLE_BRANCHIMAGE_DYNAMIC_RELOCATION_ARM64XIMAGE_DYNAMIC_RELOCATION_FUNCTION_OVERRIDEIMAGE_DYNAMIC_RELOCATION_ARM64_KERNEL_IMPORT_CALL_TRANSFERIMAGE_DYNAMIC_RELOCATION_GUARD_RF_PROLOGUEIMAGE_DYNAMIC_RELOCATION_GUARD_RF_EPILOGUEIMAGE_DYNAMIC_RELOCATION_GUARD_IMPORT_CONTROL_TRANSFERIMAGE_DYNAMIC_RELOCATION_GUARD_INDIR_CONTROL_TRANSFERIMAGE_DYNAMIC_RELOCATION_GUARD_SWITCHTABLE_BRANCHIMAGE_DYNAMIC_RELOCATION_ARM64XIMAGE_DYNAMIC_RELOCATION_FUNCTION_OVERRIDEIMAGE_DYNAMIC_RELOCATION_ARM64_KERNEL_IMPORT_CALL_TRANSFERDVRT V2 and other relocation types are probably going to be implemented in follow-up PRs.
Reference: