Add ability to mark memory area read only. Add new API uc_mem_map_ex t… #60

cseagle · 2015-08-26T20:38:46Z

Add ability to mark memory area read only. Add new API uc_mem_map_ex to allow permissions to be passed. Change MemoryBlock to track created MemoryRegions. Add regress/ro_mem_test.c

This also solves the memory leak issue on uc->ram because now all MemoryRegion allocations are tracked in the uc->mapped_blocks array.

…o allow permissions to be passed. Change MemoryBlock to track created MemoryRegions. Add regress/ro_mem_test.c

aquynh · 2015-08-27T04:06:50Z

you do not setup the stack in the sample ro_mem_test.c, but somehow this still works ??

cseagle · 2015-08-27T04:12:28Z

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Good point. Magic I guess. I will investigate.

On 8/26/2015 9:06 PM, Nguyen Anh Quynh wrote:

you do not setup the stack in the sample |ro_mem_test.c|, but
somehow this still works ??

— Reply to this email directly or view it on GitHub
<#60 (comment)
351>.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iEYEARECAAYFAlXejhcACgkQ9zL2pkgLDWmlYgCdG5Q6oRMXPN44Fzzf5BdxVvAA
CQAAn2HnLG20ZMHmTOSCnfkf2xNYUkHl
=yrE5
-----END PGP SIGNATURE-----

aquynh · 2015-08-27T04:29:10Z

few more things:

please sync with upstream, because there is a minor conflict at the moment in Makefile.
the main() function should return int value. current code either just return;, or do not return anything at the end.
there are few other warnings on printf() arguments, but apparently your compiler does not report them. can you find a way to turn on the warning messages?

thanks.

cseagle · 2015-08-27T04:31:03Z

Will you paste the warnings please? It would help me figure out what compiler options you may henabled that I do not.

aquynh · 2015-08-27T04:33:21Z

sure, there are the warnings generated by Clang on OSX: https://gist.github.com/aquynh/58f83e579f6d32695e58

cseagle · 2015-08-27T04:59:42Z

Note that you will need to decide what to do with the uc_struct.ram field as seems to no longer be needed, but I do not know what your design goal for that field was.

aquynh · 2015-08-27T05:50:40Z

a question: we can intercept invalid memory access events via hooking, like in hook_mem_invalid() in sample_x86.c. have you investigated if you can still do that now with this PR, after you write protect the memory with uc_mem_map_ex(handle, 0x400000, 0x4000, UC_PROT_READ | UC_PROT_EXEC)? do you still get the callback executed when writing to this memory?

thanks.

cseagle · 2015-08-27T06:04:50Z

Yes, you still receive the hook_mem_invalid callbacks for regions that are not mapped. I will change ro_mem_test to demonstrate by omitting the stack, then mapping it in in the callback.

aquynh · 2015-08-27T06:10:13Z

no, i mean that if you do not write protect the memory, the emulated code (with MOV instruction) works without any issue. now because you write protect it, MOV will fail. i am asking if you now can intercept this failure with UC_HOOK_MEM_INVALID, or not. if not, then we need to handle this situation now.

cseagle · 2015-08-27T06:23:30Z

Ah ok, let me test

cseagle · 2015-08-27T06:35:06Z

The answer is that no error condition is recognized in uc_emu_start. So, while the memory is in fact read only, no signal is propagating out of uc_emu_start, so it will need its own detection and error code

cseagle · 2015-08-27T07:45:53Z

This should probably be detected at the same places memory writes are detected in the softmmu. The big question is whether you want to overload the meaning of UC_ERR_MEM_WRITE which currently means that a write outside a mapped region occurred, or do you want to introduce a new error type or new uc_mem_type value. I think a new uc_mem_type such as UC_MEM_WRITE_RO might work.

aquynh · 2015-08-27T08:09:19Z

yes adding that new error code sounds OK.

radare · 2015-08-27T11:52:22Z

why use uc_mem_map_ex? what’s the meaning of that “ex” suffix suposed to be?

imho _prot or _perm sounds better to me for that purpose.

On 27 Aug 2015, at 10:09, Nguyen Anh Quynh notifications@github.com wrote:

yes adding that new error code sounds OK.

—
Reply to this email directly or view it on GitHub #60 (comment).

aquynh · 2015-08-27T13:14:59Z

i think "ex" here means "extended". this is the typical way to name API in Windows platform, when they want to extend the existing API to add more features/functionalities.

since we do not need to worry backward compatibility, we can think about extending the original uc_mem_map() to have one more argument about permission, without having to add uc_mem_map_ex() like this.

radare · 2015-08-27T13:17:40Z

+1 for adding one parameter for it, but then we will need another function to change those permissions after the map is allocated.

On 27 Aug 2015, at 15:15, Nguyen Anh Quynh notifications@github.com wrote:

i think "ex" here means "extended". this is the typical way to name API in Windows platform, when they want to extend the existing API to add more features/functionalities.

since we do not need to worry backward compatibility, we can think about extending the original uc_mem_map() to have one more argument about permission, without having to add uc_mem_map_ex() like this.

—
Reply to this email directly or view it on GitHub #60 (comment).

cseagle · 2015-08-27T15:29:18Z

The general idea is that uc_mem_map_ex would supersede uc_mem_map, hence the nearly identical name. I need to get it all working cleanly first. Once that happens, I have every intention of adding a uc_mprotect and a uc_munmap.

radare · 2015-08-27T15:39:25Z

why not follow the standard naming as uc_mem_protect() and uc_mem_unmap() ?

On 27 Aug 2015, at 17:29, Chris Eagle notifications@github.com wrote:

The general idea is that uc_mem_map_ex would supersede uc_mem_map, hence the nearly identical name. I need to get it all working cleanly first. Once that happens, I have every intention of adding a uc_mprotect and a uc_munmap.

—
Reply to this email directly or view it on GitHub #60 (comment).

…lity on write to read only. Add new error type UC_ERR_MEM_WRITE_RO and new access type UC_MEM_WRITE_RO for use in callback

cseagle · 2015-08-28T01:06:10Z

That is fine with me I will use those names when I get over the basic read only memory implementation

aquynh · 2015-08-28T07:04:23Z

we also need to put mem_protect.c into samples/Makefile to compile it.

thanks.

aquynh · 2015-08-28T08:20:47Z

another issue with uc_mem_protect is that while we have "address" & "size" parameters, internally it seems to set permission for the whole region, but not only for the range [address, address + size]. any solution for this?

btw, it would be nice if you open another branch, and issue another PR for uc_mem_protect, since your work on setting the permission for uc_mem_map() seems to be ready to merge.

thanks.

cseagle · 2015-08-28T08:41:09Z

Regarding uc_mem_protect, it needs more research to figure out how to split MemoryRegions with the qemu api, until then, it would be necessary to force changing permissions on entire blocks only to at least stub out an API that people can work on

aquynh · 2015-08-28T08:53:38Z

two functions getMemoryBlock and memory_mapping are similar. please use one, and remove the other.

another thing: so far we can deal with the case of writing to read-only memory. do we need to also handle the case of reading from memory with no permission? is there such a case, when we map memory with 0 permission - no RWX at all?

thanks.

cseagle · 2015-08-28T09:16:52Z

I have no idea what happens when a region of memory is mapped with no read permission. I could code up a test to see how linux behaves, but this case makes no sense to me. it might make sense in the case of a memory mapped file that is write only, but the unicorn API does not deal with mapped files. How is mapped memory useful if it can't be read?

JonathonReinhart · 2015-08-28T09:34:33Z

@cseagle Well normally I would say it is useful to trigger a trap when that memory is accessed, so you can do different things. However in this case, we already have the memory read hook, so allowing non-readable memory might not be useful in unicorn.

aquynh · 2015-08-28T09:44:51Z

this is just a question to make sure we handle all the corner cases. it is trivial to implement, anyway.

if you do not want this situation, then uc_mem_map() must reject permission without UC_PROT_READ. then we might come back to this in the future if this should be handled differently.

cseagle · 2015-08-28T10:44:03Z

Ok now it rejects reads from non-readable regions

aquynh · 2015-08-28T10:57:47Z

i merged this with some minor fixes into a new branch "mem_map_ex". will look closer before merging it to the "master" branch.

thanks.

aquynh · 2015-08-28T14:20:18Z

i feel that UC_ERR_MEM_WRITE_NW & UC_ERR_MEM_READ_NR are not very intuitive. it is hard to figre out what "N" means here.

how about these names instead: UC_ERR_MEM_WRITE_RO & UC_ERR_MEM_READ_WO ?

thanks.

cseagle · 2015-08-28T14:58:07Z

If we add an execute but in the future then the memory is not nessarily RO
or WO. I borrowed from NX for not executable. Feel free to change if you
wish but RO and WO will become misnomers.
On Aug 28, 2015 7:20 AM, "Nguyen Anh Quynh" notifications@github.com
wrote:

i feel that UC_ERR_MEM_WRITE_NW & UC_ERR_MEM_READ_NR are not very
intuitive. it is hard to figre out what "N" means here.

how about these names instead: UC_ERR_MEM_WRITE_RO & UC_ERR_MEM_READ_WO ?

thanks.

—
Reply to this email directly or view it on GitHub
#60 (comment)
.

JonathonReinhart · 2015-08-28T15:19:57Z

Why not follow the mmap(2) convention:

The prot argument describes the desired memory protection of the
   mapping (and must not conflict with the open mode of the file).  It
   is either PROT_NONE or the bitwise OR of one or more of the following
   flags:

   PROT_EXEC  Pages may be executed.

   PROT_READ  Pages may be read.

   PROT_WRITE Pages may be written.

   PROT_NONE  Pages may not be accessed.

cseagle · 2015-08-28T15:24:36Z

I followed those conventions exactly. This is in respect to what to name the error values that are used to indicate that execution terminated due to, for example, a read from a read only location. I elected UC_ERR_MEM_WRITE_NR for now, but the suggestion is to change that to UC_ERR_MEM_WRITE_WO which I countered earlier. The permissions themselves are UC_PROT_READ, UC_PROT_WRITE, and soon hopefully UC_PROT_EXEC.

aquynh · 2015-08-28T15:26:37Z

regress/ro_mem_test.c

+    printf("BEGIN execution - 2\n");
+    uint32_t eax = 0x40002C;
+    uc_reg_write(handle, UC_X86_REG_EAX, &eax); 
+    err = uc_emu_start(handle, 0x400015, 0x400000 + sizeof(PROGRAM), 0, 2);


why is this 0x400015 here? do you really mean 0x400000 ?

it seems you want to start from mov dword ptr [eax], 0x87654321. some more comments in the source should be helpful here.

cseagle · 2015-08-28T15:36:36Z

To amplify, when UC_PROT_EXEC becomes possible, there will be 8 possible combinations of permissions. Exactly 1 of those 8 will be RO, 1 will be WO and one will be XO, yet 4 will be NX, 4 will be NR, and 4 will be NW. Since you tend to be pedantic, NX, NR, and NW are the correct ways to refer to missing X, R, or W permissions.

aquynh · 2015-08-28T15:37:14Z

these error code are starting to get ugly. perhaps we should keep them simple to be just UC_ERR_MEM_WRITE & UC_ERR_MEM_READ ?

JonathonReinhart · 2015-08-28T15:40:18Z

@cseagle I missed the ERR part. Sorry for the noise.

cseagle · 2015-08-28T16:04:28Z

I have no problem with that either. It does make it harder for the
programmer to determine exactly which read or write related issue occured.
With a robust enough set of apis that should still be possible.
On Aug 28, 2015 8:37 AM, "Nguyen Anh Quynh" notifications@github.com
wrote:

these error code are starting to get ugly. perhaps we should keep them
simple to be just UC_ERR_MEM_WRITE & UC_ERR_MEM_READ ?

—
Reply to this email directly or view it on GitHub
#60 (comment)
.

aquynh · 2015-08-29T00:56:41Z

merged this into master branch, thanks!

now lets rename uc_mem_map_ex() to uc_mem_map to support memory permission at mapping time?

cseagle · 2015-08-29T01:44:58Z

now lets rename |uc_mem_map_ex()| to |uc_mem_map| to support memory permission at mapping time?

Happy to do that, but I would socialize the change as it will break everyone's code the minute we do it.

aquynh · 2015-08-29T02:01:33Z

please go ahead doing that as soon as you can. i can fix the Python binding right after this change.

Add ability to mark memory are read only. Add new API uc_mem_map_ex t…

00944b6

…o allow permissions to be passed. Change MemoryBlock to track created MemoryRegions. Add regress/ro_mem_test.c

cseagle changed the title ~~Add ability to mark memory are read only. Add new API uc_mem_map_ex t…~~ Add ability to mark memory area read only. Add new API uc_mem_map_ex t… Aug 26, 2015

Free up all MemoryRegion* when uc is closed

4b529bc

setup stack pointer in ro_mem_test

dec793e

cseagle added 3 commits August 26, 2015 21:38

regress/Makefile conflict resolution

9442b46

Eliminate clang warnings in ro_mem_test.c

a5cc88d

Merge branch 'master' into mem_map_ex

5264fc7

Demonstrate continued correct behavior on invalid memory access

980ec8b

Improve status reporting in regress/ro_mem_test.c

f357f4d

cseagle added 2 commits August 27, 2015 18:03

Detect all occurences of write to read only page. Add callback capabi…

686acb7

…lity on write to read only. Add new error type UC_ERR_MEM_WRITE_RO and new access type UC_MEM_WRITE_RO for use in callback

fix perms on files

140e9f9

cseagle added 2 commits August 28, 2015 00:17

Makefile cleanup

4f11d88

Doc cleanup

71ddad9

Roll back uc_mem_protect changes

adc254c

delete redundant function

aa509cc

Add code to handle non-readable memory

3452b47

Add regress check to handle non-readable memory

c8d64cf

aquynh reviewed Aug 28, 2015
View reviewed changes

aquynh merged commit c8d64cf into unicorn-engine:master Aug 29, 2015

cseagle deleted the mem_map_ex branch August 29, 2015 06:14

chfl4gs added a commit to chfl4gs/unicorn that referenced this pull request Mar 6, 2020

cmd run unicorn-engine#60

8f95016

Add ability to mark memory area read only. Add new API uc_mem_map_ex t… #60

Add ability to mark memory area read only. Add new API uc_mem_map_ex t… #60

Conversation

cseagle commented Aug 26, 2015

aquynh commented Aug 27, 2015

cseagle commented Aug 27, 2015

aquynh commented Aug 27, 2015

cseagle commented Aug 27, 2015

aquynh commented Aug 27, 2015

cseagle commented Aug 27, 2015

aquynh commented Aug 27, 2015

cseagle commented Aug 27, 2015

aquynh commented Aug 27, 2015

cseagle commented Aug 27, 2015

cseagle commented Aug 27, 2015

cseagle commented Aug 27, 2015

aquynh commented Aug 27, 2015

radare commented Aug 27, 2015

aquynh commented Aug 27, 2015

radare commented Aug 27, 2015

cseagle commented Aug 27, 2015

radare commented Aug 27, 2015

cseagle commented Aug 28, 2015

aquynh commented Aug 28, 2015

aquynh commented Aug 28, 2015

cseagle commented Aug 28, 2015

aquynh commented Aug 28, 2015

cseagle commented Aug 28, 2015 via email

JonathonReinhart commented Aug 28, 2015

aquynh commented Aug 28, 2015

cseagle commented Aug 28, 2015

aquynh commented Aug 28, 2015

aquynh commented Aug 28, 2015

cseagle commented Aug 28, 2015

JonathonReinhart commented Aug 28, 2015

cseagle commented Aug 28, 2015 via email

aquynh Aug 28, 2015

Choose a reason for hiding this comment

aquynh Aug 28, 2015

Choose a reason for hiding this comment

cseagle commented Aug 28, 2015 via email

aquynh commented Aug 28, 2015

JonathonReinhart commented Aug 28, 2015

cseagle commented Aug 28, 2015

aquynh commented Aug 29, 2015

cseagle commented Aug 29, 2015 via email

aquynh commented Aug 29, 2015