Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC
On 8/12/22 08:02, Ilya Leoshkevich wrote: tb_lookup() skips get_page_addr_code() if tb is found in tb_jmp_cache. I assume it's a bug? Yes, I think so. I've rearranged that for other reasons, and so may have inadvertently fix this. I'll post the in-progress work in a moment. r~
Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC
On Thu, 2022-08-11 at 08:42 -0700, Richard Henderson wrote: > On 8/11/22 02:28, Ilya Leoshkevich wrote: > > How is qemu-user's get_page_addr_code() involved here? > > > > I tried to experiment with it, and while I agree that it looks > > buggy, > > it's called only from translation code paths. If we already have a > > translation block, these code paths are not used. > > It's called from tb_lookup too, when we're trying to find an existing > TB. > > > r~ > Oh, I see. I was first worried about direct block chaining with goto_tb, but it turned out that translator_use_goto_tb() prevented it. tb_lookup() skips get_page_addr_code() if tb is found in tb_jmp_cache. I assume it's a bug?
Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC
On 8/11/22 02:28, Ilya Leoshkevich wrote: How is qemu-user's get_page_addr_code() involved here? I tried to experiment with it, and while I agree that it looks buggy, it's called only from translation code paths. If we already have a translation block, these code paths are not used. It's called from tb_lookup too, when we're trying to find an existing TB. r~
Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC
On Wed, 2022-08-10 at 13:29 -0700, Richard Henderson wrote:
> On 8/8/22 10:10, Ilya Leoshkevich wrote:
> > After mprotect(addr, PROT_NONE), addr can still be executed if
> > there
> > are cached translations. Drop them.
> >
> > Signed-off-by: Ilya Leoshkevich
> > ---
> > accel/tcg/translate-all.c | 17 -
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index ef62a199c7..32ea5f0adf 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start,
> > target_ulong end, int flags)
> > len != 0;
> > len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
> > PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS,
> > 1);
> > + bool write_set, exec_cleared;
> >
> > - /* If the write protection bit is set, then we invalidate
> > - the code inside. */
> > - if (!(p->flags & PAGE_WRITE) &&
> > - (flags & PAGE_WRITE) &&
> > - p->first_tb) {
> > + /*
> > + * If the write protection bit is set, then we invalidate
> > the code
> > + * inside.
> > + */
> > + write_set = !(p->flags & PAGE_WRITE) && (flags &
> > PAGE_WRITE);
> > + /*
> > + * If PAGE_EXEC is cleared, we also need to invalidate the
> > code in
> > + * order to force a fault when trying to run it.
> > + */
> > + exec_cleared = (p->flags & PAGE_EXEC) && !(flags &
> > PAGE_EXEC);
> > + if ((write_set || exec_cleared) && p->first_tb) {
>
> I believe the bug you're trying to fix is in get_page_addr_code,
> which for USER_ONLY is
> currently a no-op. It ought to be checking the page permissions
> there, as we do for softmmu.
>
> I have a patch for get_page_addr_code in the works, because I was
> working on pther stuff
> in the area.
How is qemu-user's get_page_addr_code() involved here?
I tried to experiment with it, and while I agree that it looks buggy,
it's called only from translation code paths. If we already have a
translation block, these code paths are not used.
Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC
On 8/8/22 10:10, Ilya Leoshkevich wrote:
After mprotect(addr, PROT_NONE), addr can still be executed if there
are cached translations. Drop them.
Signed-off-by: Ilya Leoshkevich
---
accel/tcg/translate-all.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ef62a199c7..32ea5f0adf 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong
end, int flags)
len != 0;
len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
+bool write_set, exec_cleared;
-/* If the write protection bit is set, then we invalidate
- the code inside. */
-if (!(p->flags & PAGE_WRITE) &&
-(flags & PAGE_WRITE) &&
-p->first_tb) {
+/*
+ * If the write protection bit is set, then we invalidate the code
+ * inside.
+ */
+write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
+/*
+ * If PAGE_EXEC is cleared, we also need to invalidate the code in
+ * order to force a fault when trying to run it.
+ */
+exec_cleared = (p->flags & PAGE_EXEC) && !(flags & PAGE_EXEC);
+if ((write_set || exec_cleared) && p->first_tb) {
I believe the bug you're trying to fix is in get_page_addr_code, which for USER_ONLY is
currently a no-op. It ought to be checking the page permissions there, as we do for softmmu.
I have a patch for get_page_addr_code in the works, because I was working on pther stuff
in the area.
r~
