cpu-target.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
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(-)
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;
--
2.43.0On 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~
© 2016 - 2025 Red Hat, Inc.