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.
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
Linux: Address limitations in determining KASLR shifts by introducing VMCoreInfo support #1332
base: develop
Are you sure you want to change the base?
Linux: Address limitations in determining KASLR shifts by introducing VMCoreInfo support #1332
Changes from 3 commits
512c1ab
b4b7834
6610779
5ab9756
00b528a
41a9347
4c0ddca
4a88a74
f2a2f8a
d80ef55
13729ac
96f5fda
9f6d194
41d2e92
bbd2f9b
03aba8d
d14c777
d56f3a5
7858af3
7ed8b99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the rest of the automagic ever validate these values in any way? If not, perhaps they should (checking for an ELF signature or mapping the virtual kernel to the physical one and checking a number of bytes match, just something to make sure the map works correctly)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. But, if it already validates these values for
find_aslr_classic()
, it will also do so forfind_aslr_vmcoreinfo()
and vice versa.To validate these values and, if it fails, proceed with a fallback action like trying the next potential candidate, we should convert the find_aslr_*() to return a generator and make a loop. Once the layer is created here try to do something with it to test if it works.
The
find_aslr_vmcoreinfo()
could easily test if the ASLR address is correct even without creating a layer. The VMCOREINFO provides the virtual address (with the aslr shift already applied) of many symbols.For instance, we can check that the address provided by get_symbol("init_uts_ns"), which is the System.map value (without the shift applied) be equal to the address provided in VMCOREINFO -> SYMBOL(init_uts_ns) minus the KERNELOFFSET. If that doesn't match there's something wrong with that VMCOREINFO table and it will have to find the next one. We can validate one, some or all of them, for instance:
However, this approach makes the VMCOREINFO implementation dependent on a symbol table, which IMO is a mistake. It would prevent future capabilities, such as retrieving symbols from kallsyms. VMCOREINFO should be the first step and must remain completely independent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, I think that we could also move the find_aslr_vmcoreinfo() to be executed before and outside the layer.scan() banner loop. Since it's independent of the ISF, I think it should be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, at the moment the classes are just trusted to have returned the right ones, but you're right it makes sense to do the double checking for either of them. Care to knock something up for that (you may have already, I'm just trying to get through all the PRs so I may have missed it, I feel one recently involved this bit of code...) 5;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of single letter variable names... 5:S Could I tempt you to use
char
(or evenchr
) instead ofc
, pretty please? 5:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, still hoping to get this changed). 5;)