search-pattern: Don't stop searching when read_memory fails #675
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
|
I want to deliver one more patch regarding the last review by @Grazfather. I hope it is fine to put the commit together here. I will rebase if it must be in separated PR. |
ammarfaizi2
force-pushed
the
handle-search-pattern-special-case
branch
from
954dc94 to
63084b0
Compare
Test Output (Linux x86_64)Click to toggle contents |
ammarfaizi2
force-pushed
the
handle-search-pattern-special-case
branch
from
63084b0 to
030cb6d
Compare
ammarfaizi2
referenced
this pull request
in ammarfaizi2/gef
Jul 7, 2021
This commit introduces 2 changes:
1) Change "info registers" to "info registers all". This will track more
registers and make sure they are not changed due to syscall.
2) Change `gdb_start_silent_cmd` to `gdb_run_cmd`. We don't need to use
`gdb_start_silent_cmd` because our `before` commands have already
started the process. And we can't see the register before we do
`set-permission` command if the process has not been started yet.
Therefore, it makes sense not to append `"entry-break"` (calling
`gdb_start_silent_cmd`).
Before this commit the result in commands will be like this:
before = [
# These two do the entry-break job!
"starti",
"si",
"printf \"match_before\\n\"",
"info registers all",
"printf \"match_before\\n\"",
"gef config context.clear_screen False",
"gef config context.layout '-code -stack'",
# This is unnecessary, because we have `starti` and `si`.
# We can't reorder it because it is appended inside the
# `gdb_start_silent_cmd`.
"entry-break"
]
cmd = "set-permission $sp"
after = [
"printf \"match_after\\n\"",
"info registers all",
"printf \"match_after\\n\""
]
After this commit:
before = [
"entry-break",
"printf \"match_before\\n\"",
"info registers all",
"printf \"match_before\\n\""
]
cmd = "set-permission $sp"
after = [
"printf \"match_after\\n\"",
"info registers all",
"printf \"match_after\\n\""
]
Link: hugsy#673 (comment)
Fixes: 5eb3b24 ("x86-64: Preserve RCX and R11 when calling mprotect_asm (syscall)")
Cc: Grazfather <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
Grazfather
reviewed
Jul 7, 2021
| # | ||
| err(estr) | ||
| return [] | ||
| else: |
There was a problem hiding this comment.
What other errors can we get?
There was a problem hiding this comment.
@Grazfather I don't yet know. But I think it is fine to throw away the other errors to the higher call stack if there is any.
There was a problem hiding this comment.
It is, I was just wondering if you had seen others.
ammarfaizi2
added a commit
to ammarfaizi2/gef
that referenced
this pull request
Jul 7, 2021
This commit addresses code review from Grazfather. Fixes: 01002fb ("search-pattern: Don't stop searching when read_memory fails") Link: hugsy#675 (comment) Signed-off-by: Ammar Faizi <[email protected]>
|
Can you squash the first and third commit? I will rebase and merge since this does 2 different things. |
|
Cc: @komori-k |
Komori Kuzuyu <[email protected]> wrote: > search-pattern command stop finding string pattern after error "Cannot > access memory at address xxxxxxxxxxxx". Checking /proc/$pid/maps the > address mentioned in error is readable but cannot be read from gdb. > > The memory is a mapped file to /dev/dri/renderD128 > Do not assume virtual memory that has read bit is always directly readable from userspace. We have a special case where /proc/$pid/maps shows virtual memory address with a read bit, but it cannot be read from the GDB. This commit adds an exception handler for read_memory on search-pattern command when such a special case occurs. Before this commit, the search-pattern command stops when it meets the above case (unhandled exception). After this commit, the search-pattern command continues the scan when read_memory fails. We still of course, show the error message indicates that the read_memory fails. The special case after this commit looks like this: gef➤ search-pattern "However" [+] Searching 'However' in memory [+] In '/usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so'(0x7fffe5576000-0x7fffe59b6000), permission=r-- 0x7fffe55f8ec6 - 0x7fffe55f8efd → "However, if the abstract value is too large, the o[...]" 0x7fffe55ff01b - 0x7fffe55ff052 → "However, if the abstract value is too large, the o[...]" [!] Cannot access memory at address 0x7fffeb00b000 [!] Cannot access memory at address 0x7fffeb0d4000 [!] Cannot access memory at address 0x7fffef49f000 [+] In '/usr/lib/x86_64-linux-gnu/libbrotlicommon.so.1.0.9'(0x7ffff72ab000-0x7ffff72ca000), permission=r-- 0x7ffff72bb287 - 0x7ffff72bb2be → "However, compositionclear:both;cooperationwithin t[...]" 0x7ffff72bd4ae - 0x7ffff72bd4e5 → "However, inprogrammersat least inapproximatealthou[...]" 0x7ffff72bd834 - 0x7ffff72bd867 → "However thelead to the\t<a href="/was grantedpeople" 0x7ffff72be10f - 0x7ffff72be146 → "However, intelligence" tabindex="float:right;Commo[...]" 0x7ffff72c1c99 - 0x7ffff72c1cd0 → "However, the An example ofcompared withquantities [...]" 0x7ffff72c1f4a - 0x7ffff72c1f81 → "However, thisDepartment ofthe remainingeffect on t[...]" 0x7ffff72c2451 - 0x7ffff72c2488 → "However, manythe presidentHowever, someis thought [...]" 0x7ffff72c246b - 0x7ffff72c24a2 → "However, someis thought tountil the endwas announc[...]" 0x7ffff72c2ff8 - 0x7ffff72c302a → "However, theand eventuallyAt the end of because of" 0x7ffff72c3c36 - 0x7ffff72c3c6d → "However, it isbecame part ofin relation topopular [...]" 0x7ffff72c66da - 0x7ffff72c670c → "However, there aresrc="http://staticsuggested that" 0x7ffff72c6c32 - 0x7ffff72c6c69 → "However, since the/div>\n</div>\n<div left; margin[...]" gef➤ Fixes: hugsy#674 Reported-by: Komori Kuzuyu <[email protected]> Signed-off-by: Ammar Faizi <[email protected]> Signed-off-by: Komori Kuzuyu <[email protected]>
This commit introduces 2 changes:
1) Change "info registers" to "info registers all". This will track more
registers and make sure they are not changed due to syscall.
2. Change `gdb_start_silent_cmd` to `gdb_run_cmd`. We don't need to use
`gdb_start_silent_cmd` because our `before` commands have already
started the process. And we can't see the register before we do
`set-permission` command if the process has not been started yet.
Therefore, it makes sense not to append `"entry-break"` by calling
`gdb_start_silent_cmd`.
Before this commit the result in commands will be like this:
before = [
# These two do the entry-break job!
"starti",
"si",
"printf \"match_before\\n\"",
"info registers all",
"printf \"match_before\\n\"",
"gef config context.clear_screen False",
"gef config context.layout '-code -stack'",
# This is unecessary, because we have `starti` and `si`.
# We can't reorder it because it is appended inside the
# `gdb_start_silent_cmd`.
"entry-break"
]
cmd = "set-permission $sp"
after = [
"printf \"match_after\\n\"",
"info registers all",
"printf \"match_after\\n\""
]
After this commit:
before = [
"gef config context.clear_screen False",
"gef config context.layout '-code -stack'",
"entry-break",
"printf \"match_before\\n\"",
"info registers all",
"printf \"match_before\\n\""
]
cmd = "set-permission $sp"
after = [
"printf \"match_after\\n\"",
"info registers all",
"printf \"match_after\\n\""
]
Fixes: 5eb3b24 ("x86-64: Preserve RCX and R11 when calling mprotect_asm (syscall)")
Cc: Grazfather <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
komori-k
force-pushed
the
handle-search-pattern-special-case
branch
from
4e8b9a7 to
73f41db
Compare
|
@ammarfaizi2 done |
|
Thanks! |
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.
search-pattern: Don't stop searching when read_memory fails
How Has This Been Tested?
make testsChecklist
devbranch, notmaster.Fixes: #674