diff mbox

[v3] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB

Message ID 1467880392-1043630-1-git-send-email-snarpix@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stanislav Shmarov July 7, 2016, 8:33 a.m. UTC
In user-mode emulation Translation Block can consist of 2 guest pages.
In that case QEMU also mprotects 2 host pages that are dedicated for
guest memory, containing instructions. QEMU detects self-modifying code
with SEGFAULT signal processing.

In case if instruction in 1st page is modifying memory of 2nd
page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
invalidate TB, generate new TB contatining 1 guest instruction and
exit to CPU loop. QEMU won't call mprotect, and new TB will cause
same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
flags, so QEMU will handle the signal as guest binary problem,
and exit with guest SEGFAULT.

Solution is to do following: In case if current TB was invalidated
continue to invalidate TBs from remaining guest pages and mark pages
as PAGE_WRITE. After that disable host page protection with mprotect.
If current tb was invalidated longjmp to main loop. That is more
efficient, since we won't get SEGFAULT when executing new TB.

Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
---
  v3: Now mprotect is called on first SEGFAULT. (Significant changes)
  v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
      pointed by Sergey Fedorov.
 translate-all.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

sergey.fedorov@linaro.org July 7, 2016, 12:05 p.m. UTC | #1
On 07/07/16 11:33, Stanislav Shmarov wrote:
> In user-mode emulation Translation Block can consist of 2 guest pages.
> In that case QEMU also mprotects 2 host pages that are dedicated for
> guest memory, containing instructions. QEMU detects self-modifying code
> with SEGFAULT signal processing.
>
> In case if instruction in 1st page is modifying memory of 2nd
> page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
> invalidate TB, generate new TB contatining 1 guest instruction and
> exit to CPU loop. QEMU won't call mprotect, and new TB will cause
> same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
> flags, so QEMU will handle the signal as guest binary problem,
> and exit with guest SEGFAULT.
>
> Solution is to do following: In case if current TB was invalidated
> continue to invalidate TBs from remaining guest pages and mark pages
> as PAGE_WRITE. After that disable host page protection with mprotect.
> If current tb was invalidated longjmp to main loop. That is more
> efficient, since we won't get SEGFAULT when executing new TB.
>
> Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>

Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>

> ---
>   v3: Now mprotect is called on first SEGFAULT. (Significant changes)
>   v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
>       pointed by Sergey Fedorov.
>  translate-all.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index eaa95e4..0d47c1c 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -2000,6 +2000,7 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
>  int page_unprotect(target_ulong address, uintptr_t pc)
>  {
>      unsigned int prot;
> +    bool current_tb_invalidated;
>      PageDesc *p;
>      target_ulong host_start, host_end, addr;
>  
> @@ -2021,6 +2022,7 @@ int page_unprotect(target_ulong address, uintptr_t pc)
>          host_end = host_start + qemu_host_page_size;
>  
>          prot = 0;
> +        current_tb_invalidated = false;
>          for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
>              p = page_find(addr >> TARGET_PAGE_BITS);
>              p->flags |= PAGE_WRITE;
> @@ -2028,10 +2030,7 @@ int page_unprotect(target_ulong address, uintptr_t pc)
>  
>              /* and since the content will be modified, we must invalidate
>                 the corresponding translated code. */
> -            if (tb_invalidate_phys_page(addr, pc)) {
> -                mmap_unlock();
> -                return 2;
> -            }
> +            current_tb_invalidated |= tb_invalidate_phys_page(addr, pc);
>  #ifdef DEBUG_TB_CHECK
>              tb_invalidate_check(addr);
>  #endif
> @@ -2040,7 +2039,8 @@ int page_unprotect(target_ulong address, uintptr_t pc)
>                   prot & PAGE_BITS);
>  
>          mmap_unlock();
> -        return 1;
> +        /* If current TB was invalidated return to main loop */
> +        return current_tb_invalidated ? 2 : 1;
>      }
>      mmap_unlock();
>      return 0;
Richard Henderson July 7, 2016, 7 p.m. UTC | #2
On 07/07/2016 01:33 AM, Stanislav Shmarov wrote:
> In user-mode emulation Translation Block can consist of 2 guest pages.
> In that case QEMU also mprotects 2 host pages that are dedicated for
> guest memory, containing instructions. QEMU detects self-modifying code
> with SEGFAULT signal processing.
>
> In case if instruction in 1st page is modifying memory of 2nd
> page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
> invalidate TB, generate new TB contatining 1 guest instruction and
> exit to CPU loop. QEMU won't call mprotect, and new TB will cause
> same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
> flags, so QEMU will handle the signal as guest binary problem,
> and exit with guest SEGFAULT.
>
> Solution is to do following: In case if current TB was invalidated
> continue to invalidate TBs from remaining guest pages and mark pages
> as PAGE_WRITE. After that disable host page protection with mprotect.
> If current tb was invalidated longjmp to main loop. That is more
> efficient, since we won't get SEGFAULT when executing new TB.
>
> Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
> ---
>   v3: Now mprotect is called on first SEGFAULT. (Significant changes)
>   v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
>       pointed by Sergey Fedorov.
>  translate-all.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
diff mbox

Patch

diff --git a/translate-all.c b/translate-all.c
index eaa95e4..0d47c1c 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -2000,6 +2000,7 @@  int page_check_range(target_ulong start, target_ulong len, int flags)
 int page_unprotect(target_ulong address, uintptr_t pc)
 {
     unsigned int prot;
+    bool current_tb_invalidated;
     PageDesc *p;
     target_ulong host_start, host_end, addr;
 
@@ -2021,6 +2022,7 @@  int page_unprotect(target_ulong address, uintptr_t pc)
         host_end = host_start + qemu_host_page_size;
 
         prot = 0;
+        current_tb_invalidated = false;
         for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
             p = page_find(addr >> TARGET_PAGE_BITS);
             p->flags |= PAGE_WRITE;
@@ -2028,10 +2030,7 @@  int page_unprotect(target_ulong address, uintptr_t pc)
 
             /* and since the content will be modified, we must invalidate
                the corresponding translated code. */
-            if (tb_invalidate_phys_page(addr, pc)) {
-                mmap_unlock();
-                return 2;
-            }
+            current_tb_invalidated |= tb_invalidate_phys_page(addr, pc);
 #ifdef DEBUG_TB_CHECK
             tb_invalidate_check(addr);
 #endif
@@ -2040,7 +2039,8 @@  int page_unprotect(target_ulong address, uintptr_t pc)
                  prot & PAGE_BITS);
 
         mmap_unlock();
-        return 1;
+        /* If current TB was invalidated return to main loop */
+        return current_tb_invalidated ? 2 : 1;
     }
     mmap_unlock();
     return 0;