| Message ID | 20220808171022.49439-2-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Fix siginfo_t contents when jumping to non-readable pages | expand |
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 <iii@linux.ibm.com> > --- > 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~
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 <iii@linux.ibm.com> > > --- > > 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.
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~
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?
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~
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) { tb_invalidate_phys_page(addr, 0); } if (reset_target_data) {
After mprotect(addr, PROT_NONE), addr can still be executed if there are cached translations. Drop them. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- accel/tcg/translate-all.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)