| Message ID | 20231215232908.1040209-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] linux-user: Allow gdbstub to ignore page protection | expand |
On 12/16/23 10:24, Ilya Leoshkevich wrote: > @@ -377,22 +379,42 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, > flags = page_get_flags(page); > if (!(flags & PAGE_VALID)) > return -1; > + prot = ((flags & PAGE_READ) ? PROT_READ : 0) | > + ((flags & PAGE_WRITE) ? PROT_WRITE : 0) | > + ((flags & PAGE_EXEC) ? PROT_EXEC : 0); See target_to_host_prot where guest PROT_EXEC is mapped to host PROT_READ. This should be flags & (PAGE_READ | PAGE_EXEC) ? PROT_READ > + if (!(prot & PROT_WRITE)) { > + if (target_mprotect(page, TARGET_PAGE_SIZE, > + prot | PROT_WRITE)) { > + return -1; > + } > + } > /* XXX: this code should not depend on lock_user */ > - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) > + if (!(p = lock_user(VERIFY_NONE, addr, l, 0))) > return -1; > memcpy(p, buf, l); > unlock_user(p, addr, l); > + if (!(prot & PROT_WRITE)) { > + if (target_mprotect(page, TARGET_PAGE_SIZE, prot)) { > + return -1; > + } > + } Is the mmap lock held here? You're opening up a window in which a page may be modified but we don't catch the modification to translation blocks. > + if (!(prot & PROT_READ)) { > + if (target_mprotect(page, TARGET_PAGE_SIZE, prot | PROT_READ)) { > + return -1; > + } > + } I really really doubt you need to do this. The page is not accessible, so why expose it at all? If you want to do this, I wonder if we should instead be interacting with ptrace? r~
On Thu, Dec 21, 2023 at 10:33:51AM +1100, Richard Henderson wrote: > On 12/16/23 10:24, Ilya Leoshkevich wrote: > > @@ -377,22 +379,42 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, > > flags = page_get_flags(page); > > if (!(flags & PAGE_VALID)) > > return -1; > > + prot = ((flags & PAGE_READ) ? PROT_READ : 0) | > > + ((flags & PAGE_WRITE) ? PROT_WRITE : 0) | > > + ((flags & PAGE_EXEC) ? PROT_EXEC : 0); > > See target_to_host_prot where guest PROT_EXEC is mapped to host PROT_READ. > This should be > > flags & (PAGE_READ | PAGE_EXEC) ? PROT_READ Oh, right. PROT_EXEC is rather set on code_gen_buffer. > > + if (!(prot & PROT_WRITE)) { > > + if (target_mprotect(page, TARGET_PAGE_SIZE, > > + prot | PROT_WRITE)) { > > + return -1; > > + } > > + } > > /* XXX: this code should not depend on lock_user */ > > - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) > > + if (!(p = lock_user(VERIFY_NONE, addr, l, 0))) > > return -1; > > memcpy(p, buf, l); > > unlock_user(p, addr, l); > > + if (!(prot & PROT_WRITE)) { > > + if (target_mprotect(page, TARGET_PAGE_SIZE, prot)) { > > + return -1; > > + } > > + } > > Is the mmap lock held here? You're opening up a window in which a page may > be modified but we don't catch the modification to translation blocks. I don't think so, I'll need to add it. > > + if (!(prot & PROT_READ)) { > > + if (target_mprotect(page, TARGET_PAGE_SIZE, prot | PROT_READ)) { > > + return -1; > > + } > > + } > > I really really doubt you need to do this. The page is not accessible, so > why expose it at all? If you want to do this, I wonder if we should instead > be interacting with ptrace? Wouldn't this break when QEMU itself is being debugged? We would also need mounted /proc or a lot of peek calls. So I thought that if we could transparently remove and restore protection, it would be better. > r~
diff --git a/cpu-target.c b/cpu-target.c index 508013e23d2..8dd7342705b 100644 --- a/cpu-target.c +++ b/cpu-target.c @@ -27,6 +27,7 @@ #include "migration/vmstate.h" #ifdef CONFIG_USER_ONLY #include "qemu.h" +#include "user-mmap.h" #else #include "hw/core/sysemu-cpu-ops.h" #include "exec/address-spaces.h" @@ -368,6 +369,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, vaddr l, page; void * p; uint8_t *buf = ptr; + int prot; while (len > 0) { page = addr & TARGET_PAGE_MASK; @@ -377,22 +379,42 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, flags = page_get_flags(page); if (!(flags & PAGE_VALID)) return -1; + prot = ((flags & PAGE_READ) ? PROT_READ : 0) | + ((flags & PAGE_WRITE) ? PROT_WRITE : 0) | + ((flags & PAGE_EXEC) ? PROT_EXEC : 0); if (is_write) { - if (!(flags & PAGE_WRITE)) - return -1; + if (!(prot & PROT_WRITE)) { + if (target_mprotect(page, TARGET_PAGE_SIZE, + prot | PROT_WRITE)) { + return -1; + } + } /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) + if (!(p = lock_user(VERIFY_NONE, addr, l, 0))) return -1; memcpy(p, buf, l); unlock_user(p, addr, l); + if (!(prot & PROT_WRITE)) { + if (target_mprotect(page, TARGET_PAGE_SIZE, prot)) { + return -1; + } + } } else { - if (!(flags & PAGE_READ)) - return -1; + if (!(prot & PROT_READ)) { + if (target_mprotect(page, TARGET_PAGE_SIZE, prot | PROT_READ)) { + return -1; + } + } /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_READ, addr, l, 1))) + if (!(p = lock_user(VERIFY_NONE, addr, l, 1))) return -1; memcpy(buf, p, l); unlock_user(p, addr, 0); + if (!(prot & PROT_READ)) { + if (target_mprotect(page, TARGET_PAGE_SIZE, prot)) { + return -1; + } + } } len -= l; buf += l;
Hi, I've noticed that gdbstub behaves differently from gdbserver in that it doesn't allow reading non-readable pages. The below patch works for me, but I'm not sure if I haven't introduced layering violations or race conditions here, so I'm sending this as an RFC. Please let me know if the code can be improved. Best regards, Ilya gdbserver ignores page protection (by virtue of using /proc/$pid/mem), so qemu gdbstub should too. One option would be to use the same mechanism, but this will not work if /proc is not mounted. So temporarily set the necessary protection bits instead. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- cpu-target.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)