does unicorn have exec-only page?

[HyperSine]

Hi, considering the following code:

import unicorn

uc = unicorn.Uc(unicorn.UC_ARCH_X86, unicorn.UC_MODE_64)
uc.mem_map(0x400000, 0x1000, unicorn.UC_PROT_EXEC)

# mov al, byte ptr[rip]
# nop
# nop
# ...
uc.mem_write(0x400000, b'\x8a\x05\x00\x00\x00\x00'.ljust(0x1000, b'\x90'))

uc.emu_start(0x400000, 0x400006)    # run until first `nop`
# expect a UcError, but no error is raised

print('al = 0x{:02x}'.format(uc.reg_read(unicorn.x86_const.UC_X86_REG_AL)))
# it prints: al = 0x90

The instruction mov al, byte ptr[rip] would have read access to the byte at 0x400006 which resides in a page that is mapped with only UC_PROT_EXEC flag. I would expect there is a UC_ERR_READ_PROT error but nothing happened. It seems UC_PROT_EXEC implies UC_PROT_READ. Is it by design? Or a bug?

[PhilippTakacs]

This is a bug, because if you map two pages and change the code to \x8a\x05\x00\x10\x00\x00 (mov al, byte ptr[rip+0x1000]) you get the expected UC_ERR_READ_PROT.

[PhilippTakacs]

I have looked a bit more at it and this isn't that easy to solve. The problem that the load helper is only called once and the result is cached. Because the virtual mmu doesn't know about the UCPROT* permissions and defaults to the x86 default permissions. So the cache is stored as read/write/exec. So I don't think there is a good solution for this bug.

As a workaround you can register a mem_read callback to all exec only pages. An alternative workaround would be to switch to UC_TLB_VIRTUAL and register a tlb callback for the exec-only pages with which sets these pages exec only[0].

[0] Register a tlb callback shouldn't be neccesary, I'll look for a fix.

[PhilippTakacs]

you can try following patch, than only switch to UC_TLB_VIRTUAL fix your problem:

diff --git a/qemu/softmmu/unicorn_vtlb.c b/qemu/softmmu/unicorn_vtlb.c
index 3b629bda..4ebade7e 100644
--- a/qemu/softmmu/unicorn_vtlb.c
+++ b/qemu/softmmu/unicorn_vtlb.c
@@ -52,6 +52,7 @@ bool unicorn_fill_tlb(CPUState *cs, vaddr address, int size,
     bool ret = false;
     struct uc_struct *uc = cs->uc;
     uc_tlb_entry e;
+    MemoryRegion *mr;
     struct hook *hook;
     HOOK_FOREACH_VAR_DECLARE;

@@ -74,7 +75,12 @@ bool unicorn_fill_tlb(CPUState *cs, vaddr address, int size,

     if (!handled) {
         e.paddr = address & TARGET_PAGE_MASK;
-        e.perms = UC_PROT_READ|UC_PROT_WRITE|UC_PROT_EXEC;
+        mr = uc->memory_mapping(uc, e.paddr);
+        if (mr) {
+            e.perms = mr->perms;
+        } else {
+            e.perms = UC_PROT_READ | UC_PROT_WRITE | PAGE_EXEC;
+        }
     }

     switch (rw) {

I'm currently not sure if this is sane for all cases, need to look a bit more in the code and write some tests.

[HyperSine]

Thanks for your patch. But I got Unhandled CPU exception (UC_ERR_EXCEPTION) rather than UC_ERR_READ_PROT when I run my code above after applying your patch. It kind of makes sense if unicorn returns UC_ERR_EXCEPTION in this circumstance. But that is too unfriendly. You know, UC_ERR_EXCEPTION is so generic and would be too hard to diagnose for those people like me.

[HyperSine]

After some investigation, maybe we should patch raise_mmu_exception?

diff --git a/qemu/softmmu/unicorn_vtlb.c b/qemu/softmmu/unicorn_vtlb.c
index 3b629bda..2de34f5f 100644
--- a/qemu/softmmu/unicorn_vtlb.c
+++ b/qemu/softmmu/unicorn_vtlb.c
@@ -9,7 +9,20 @@
 static void raise_mmu_exception(CPUState *cs, target_ulong address,
                                 int rw, uintptr_t retaddr)
 {
-    cs->uc->invalid_error = UC_ERR_EXCEPTION;
+    switch (rw) {
+    case MMU_DATA_LOAD:
+        cs->uc->invalid_error = UC_ERR_READ_PROT;
+        break;
+    case MMU_DATA_STORE:
+        cs->uc->invalid_error = UC_ERR_WRITE_PROT;
+        break;
+    case MMU_INST_FETCH:
+        cs->uc->invalid_error = UC_ERR_FETCH_PROT;
+        break;
+    default:
+        cs->uc->invalid_error = UC_ERR_EXCEPTION;
+        break;
+    }
     cs->uc->invalid_addr = address;
     cpu_exit(cs->uc->cpu);
     cpu_loop_exit_restore(cs, retaddr);

[PhilippTakacs]

Change error in raise_mmu_exception is not a solution, because this won't trigger the expected callbacks (also a problem with my first patch). Also it is misleading when the vtlb behaves different then an emulated tlb.

I have tough a bit about it. The best way would be to set the permission to the requested operation. Like this:

diff --git a/qemu/softmmu/unicorn_vtlb.c b/qemu/softmmu/unicorn_vtlb.c
index 3b629bda..a9634737 100644
--- a/qemu/softmmu/unicorn_vtlb.c
+++ b/qemu/softmmu/unicorn_vtlb.c
@@ -74,7 +74,20 @@ bool unicorn_fill_tlb(CPUState *cs, vaddr address, int size,

     if (!handled) {
         e.paddr = address & TARGET_PAGE_MASK;
-        e.perms = UC_PROT_READ|UC_PROT_WRITE|UC_PROT_EXEC;
+        switch (rw) {
+        case MMU_DATA_LOAD:
+            e.perms = UC_PROT_READ;
+            break;
+        case MMU_DATA_STORE:
+            e.perms = UC_PROT_WRITE;
+            break;
+        case MMU_INST_FETCH:
+            e.perms = UC_PROT_EXEC;
+            break;
+        default:
+            e.perms = 0;
+            break;
+        }
     }

     switch (rw) {