* [PATCH RFC] linux-user: Allow gdbstub to ignore page protection
@ 2023-12-15 23:24 Ilya Leoshkevich
2023-12-20 23:33 ` Richard Henderson
0 siblings, 1 reply; 3+ messages in thread
From: Ilya Leoshkevich @ 2023-12-15 23:24 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Alex Bennée, philmd
Cc: qemu-devel, Ilya Leoshkevich
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.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RFC] linux-user: Allow gdbstub to ignore page protection
2023-12-15 23:24 [PATCH RFC] linux-user: Allow gdbstub to ignore page protection Ilya Leoshkevich
@ 2023-12-20 23:33 ` Richard Henderson
2023-12-21 12:26 ` Ilya Leoshkevich
0 siblings, 1 reply; 3+ messages in thread
From: Richard Henderson @ 2023-12-20 23:33 UTC (permalink / raw)
To: Ilya Leoshkevich, Paolo Bonzini, Alex Bennée, philmd; +Cc: qemu-devel
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~
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RFC] linux-user: Allow gdbstub to ignore page protection
2023-12-20 23:33 ` Richard Henderson
@ 2023-12-21 12:26 ` Ilya Leoshkevich
0 siblings, 0 replies; 3+ messages in thread
From: Ilya Leoshkevich @ 2023-12-21 12:26 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Alex Bennée, philmd; +Cc: qemu-devel
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~
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-21 12:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-15 23:24 [PATCH RFC] linux-user: Allow gdbstub to ignore page protection Ilya Leoshkevich
2023-12-20 23:33 ` Richard Henderson
2023-12-21 12:26 ` Ilya Leoshkevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).