Mtrr fixes #329
Merged
Mtrr fixes #329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Increase the maximum number of MTRRs stored in the EPT state from arbitrary 100 to the architecturally defined maximum possible number of combined variable and fixed range MTRRs (343)
Also fix invalid UTF-8 characters in comment block re: fixed range MTRRs
1. 'TargetMemoryType' is used in this function both as the variable that holds the eventual return value (i.e. it is normally assigned to at least once), but it is simultaneously also read from in the same loop in order to determine whether the current or some previous MTRR entry should take precedence for the address. Therefore, initializing this variable to any value that also happens to be a valid memory caching type can actually have subtle unintended side effects because makes the initial value biased. Initialize to a sentinel value of -1 to signify 'not found' instead, and only return the default memory type if no applicable MTRR was found for the address. 2. Related to above: use stricter checks w.r.t. memory type precedence rules before actually assigning a value to TargetMemoryType. This allows us to *tentatively* assign write-through in the case of overlapping MTRRs where one specifies write-through and the other write-back. However, continue searching in this case because fixed range and uncacheable type MTRRs both still take precedence. 3. Simplify check for whether an MTRR describes a page or not. We do not need the 'end address' of the requested page to determine this; an MTRR already has a base and end address, so use these to determine whether the page is contained within its range instead.
1. For the same reasons mentioned in 500d0b0 (correctly applying precedence rules when determining the memory caching type for some page), we actually need a complete collection of *all* valid MTRRs, not just those specifying a memory type other than the default. The reasoning for this is that while the MTRR's caching type is obviously what EptGetMemoryType() is looking for, the other fields of the MTRRs are still required in order to determine the order of precedence to apply to the MTRRs themselves. Fixed range MTRRs take precedence over variable range MTRRs if enabled, and UC (which is normally also the default non-MTRR type!) takes precedence over other memory types. Even the base and end addresses are required, as these are used to determine whether any MTRRs with conflicting memory types exist with overlapping ranges that both contain the requested page. 2. The last such check removed here was applying similar logic to skip certain MTRRs, but in a way that was more critically broken: because *our* default type (but probably not the CPU's, as reported by IA32_MTRR_DEF_TYPE MSR!) is write-back caching for EPTs(?), any MTRRs specifying write-back were *also* being ignored here and never committed to the global MTRR map. This commit fixes an issue where 'load vmm' was reliably causing my Intel 13th gen system to become completely unresponsive, requiring a hard shutdown. Reference: HyperDbg#323
|
Thanks a lot for the PR and the detailed explanation. I'm gonna test it with my 12 gen processor and if it is fine, we gonna release a new version (v0.7.2) as this is a critical bug. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a number of issues in the MTRR-related code in
EptBuildMtrrMap()andEptGetMemoryType(). The changes themselves are thoroughly documented in the commit messages, so please refer to those for more details.I think most of the things I (hopefully...) fixed were probably medium severity issues at worst, and I wouldn't normally have even noticed them, let alone made a PR, except for commit 580907e, which fixes a critical issue where
load vmmwas reliably causing my Z790 + i5 13600K machine to become completely unresponsive (i.e. requiring a hard shutdown/power cycle).Re: the other commits: I am pretty sure these are real fixes for real bugs, but these were mostly just things I noticed while going through the code and attempting to narrow down the cause of the system hang, so I can't speak to their severity. I can't honestly say I've noticed any difference (for better or worse) from them.
Fixes: #..? Possibly #323, but only if that is in fact the same issue I was seeing. There are some discrepancies with this bug description compared to what this PR is fixing that make me doubt this - see my comment for details.
This PR definitely fixes the full system hang in
load vmmon my machine, as well as for at least one other person I found who was running into the same issue (also on a Z790 + 13th gen machine).Type of change